[libvirt] [PATCHv2 3/7] interface: implement public APIs for libvirt transactional network changes

[Daniel Veillard]

On Thu, May 19, 2011 at 04:51:25PM -0400, Laine Stump wrote:
> From: Michal Privoznik <mprivozn at redhat.com>
> 
> ---
>  src/libvirt.c |  141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index ff16c48..786ce15 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -8143,7 +8143,9 @@ error:
>   * @xml: the XML description for the interface, preferably in UTF-8
>   * @flags: and OR'ed set of extraction flags, not used yet
>   *
> - * Define an interface (or modify existing interface configuration)
> + * Define an interface (or modify existing interface configuration).
> + * This can, however be affected by virInterfaceChangeBegin
> + * and/or friends.

  Small nit, I would suggest making the 4 extra comments a bit more
  explicit that it's about transaction support, something along the
  lines of:

    * This can however be affected by the transaction support for
    * interface configuration change, see virInterfaceChangeBegin(),
    * virInterfaceChangeCommit() and related functions.

[...]
> @@ -8374,6 +8382,135 @@ virInterfaceFree(virInterfacePtr iface)
>      return 0;
>  }
>  
> +/**
> + * virInterfaceChangeBegin:
> + * @conn: pointer to hypervisor connection
> + * @flags: flags, not used yet
> + *
> + * This functions creates a restore point to which one can return
> + * later by calling virInterfaceChangeRollback. This function should
> + * be called before any transaction with interface configuration.
> + * Once knowing, new configuration works, it can be commited via
> + * virInterfaceChangeCommit, which frees restore point.

   which frees the restore point.

I would like the description of what happens ona  sequence of
   virInterfaceChangeBegin()
   virInterfaceChangeBegin()

calls for the same connection, is that an error ? Will netcf implement
this, this behaviour should probably be documented.

> + *
> + * Returns 0 in case of success and -1 in case of failure.
> + */
> +int
> +virInterfaceChangeBegin(virConnectPtr conn, unsigned int flags)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("conn=%p, flags=%d", conn, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        goto end;
> +    }
> +
> +    if (conn->flags & VIR_CONNECT_RO) {
> +        virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        virDispatchError(conn);
> +        goto end;
> +    }
> +
> +    if (conn->interfaceDriver && conn->interfaceDriver->interfaceChangeBegin) {
> +        ret = conn->interfaceDriver->interfaceChangeBegin(conn, flags);
> +    } else {
> +        virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +    }
> +
> +end:
> +    VIR_DEBUG("returning: %d", ret);
> +    return ret;
> +}
> +
> +/**
> + * virInterfaceChangeCommit:
> + * @conn: pointer to hypervisor connection
> + * @flags: flags, not used yet
> + *
> + * This commits the changes made to interfaces and frees restore point

  "frees the restore point"

> + * created by virInterfaceChangeBegin.

  Same thing behaviour of virInterfaceChangeCommit() in case there was
  no Begin() associated should be documented.

> + * Returns 0 in case of success and -1 in case of failure.
> + */
> +int
> +virInterfaceChangeCommit(virConnectPtr conn, unsigned int flags)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("conn=%p, flags=%d", conn, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        goto end;
> +    }
> +
> +    if (conn->flags & VIR_CONNECT_RO) {
> +        virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        virDispatchError(conn);
> +        goto end;
> +    }
> +
> +    if (conn->interfaceDriver && conn->interfaceDriver->interfaceChangeCommit) {
> +        ret = conn->interfaceDriver->interfaceChangeCommit(conn, flags);
> +    } else {
> +        virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +    }
> +
> +end:
> +    VIR_DEBUG("returning: %d", ret);
> +    return ret;
> +}
> +
> +/**
> + * virInterfaceChangeRollback:
> + * @conn: pointer to hypervisor connection
> + * @flags: flags, not used yet
> + *
> + * This cancels changes made to interfaces settings by restoring previous
> + * state created by virInterfaceChangeBegin.
> + *

  Same thing behaviour of virInterfaceChangeRollback() in case there was
  no Begin() associated should be documented.

> + * Returns 0 in case of success and -1 in case of failure.
> + */
> +int
> +virInterfaceChangeRollback(virConnectPtr conn, unsigned int flags)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("conn=%p, flags=%d", conn, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECT(conn)) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        goto end;
> +    }
> +
> +    if (conn->flags & VIR_CONNECT_RO) {
> +        virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        virDispatchError(conn);
> +        goto end;
> +    }
> +
> +    if (conn->interfaceDriver &&
> +        conn->interfaceDriver->interfaceChangeRollback) {
> +        ret = conn->interfaceDriver->interfaceChangeRollback(conn, flags);
> +    } else {
> +         virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +    }
> +
> +end:
> +    VIR_DEBUG("returning: %d", ret);
> +    return ret;
> +}

[...]

  The partial code checking entry points seems fine but the real stuff
happens somewhere else in netcf.
  I would like to have documented the behaviour in corner cases like
I described but it would be good to clarify too what happens when
2 libvirt users/program/agents use simultanously those entry points
targetting the same node. It seems to me virInterfaceChangeBegin()
should be allowed only once on the full host before a Commit(),
a Rollback() or a reboot happens, and if it's the case it should
be documented in virInterfaceChangeBegin comment too.

  So with clarifications on semantic, ACK :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/