[libvirt] [PATCH 4/9] Public API Implementation

[Daniel P. Berrange]

On Thu, Jan 14, 2010 at 10:42:41AM -0700, Jim Fehlig wrote:
> Implementation of public API for virDomain{Attach,Detach}DeviceFlags.
> ---
>  src/libvirt.c |  106 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 98 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 1145561..77f76bc 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -5129,7 +5129,6 @@ error:
>  int
>  virDomainAttachDevice(virDomainPtr domain, const char *xml)
>  {
> -    virConnectPtr conn;
>      DEBUG("domain=%p, xml=%s", domain, xml);
>  
>      virResetLastError();
> @@ -5147,17 +5146,63 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml)
>          virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__);
>          goto error;
>      }
> +
> +    return virDomainAttachDeviceFlags(domain, xml,
> +                                      VIR_DOMAIN_DEVICE_MODIFY_LIVE);
> +
> +error:
> +    virDispatchError(domain->conn);
> +    return -1;
> +}

This looks safe, but there's a subtle problem with changing the existing
virDomainAttachDevice() entry point to call virDomainAttachDeviceFlags().
It will break compatability with old libvirtd, even though the old
libvirtd supports the virDomainAttachDevice() code. So we need to keep
the distinct paths in the public API & driver definitions. The eventual
low level hypervisor drivers can of course just turn their existing
impl into a thin wrapper to the new method..

> +
> +/**
> + * virDomainAttachDeviceFlags:
> + * @domain: pointer to domain object
> + * @xml: pointer to XML description of one device
> + * @flags: an OR'ed set of virDomainDeviceModifyFlags
> + *
> + * Attach a virtual device to a domain, using the flags parameter
> + * to control how the device is attached.  VIR_DOMAIN_DEVICE_MODIFY_CURRENT
> + * specifies that the device allocation is made based on current domain
> + * state.  VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be
> + * allocated to the active domain instance only and is not added to the
> + * persisted domain configuration.  VIR_DOMAIN_DEVICE_MODIFY_CONFIG
> + * specifies that the device shall be allocated to the persisted domain
> + * configuration only.  Note that the target hypervisor must return an
> + * error if unable to satisfy flags.  E.g. the hypervisor driver will
> + * return failure if LIVE is specified but it only supports modifying the
> + * persisted device allocation.
> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + */
> +int
> +virDomainAttachDeviceFlags(virDomainPtr domain,
> +                           const char *xml, unsigned int flags)
> +{
> +    virConnectPtr conn;
> +    DEBUG("domain=%p, xml=%s, flags=%d", domain, xml, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        return (-1);
> +    }
> +    if (domain->conn->flags & VIR_CONNECT_RO) {
> +        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
>      conn = domain->conn;
>  
> -    if (conn->driver->domainAttachDevice) {
> +    if (conn->driver->domainAttachDeviceFlags) {
>          int ret;
> -        ret = conn->driver->domainAttachDevice (domain, xml);
> +        ret = conn->driver->domainAttachDeviceFlags(domain, xml, flags);
>          if (ret < 0)
>              goto error;
>          return ret;
>      }
>  
> -    virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +    virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
>  
>  error:
>      virDispatchError(domain->conn);
> @@ -5177,7 +5222,6 @@ error:
>  int
>  virDomainDetachDevice(virDomainPtr domain, const char *xml)
>  {
> -    virConnectPtr conn;
>      DEBUG("domain=%p, xml=%s", domain, xml);
>  
>      virResetLastError();
> @@ -5195,17 +5239,63 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml)
>          virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__);
>          goto error;
>      }
> +
> +    return virDomainDetachDeviceFlags(domain, xml,
> +                                      VIR_DOMAIN_DEVICE_MODIFY_LIVE);
> +
> +error:
> +    virDispatchError(domain->conn);
> +    return -1;
> +}
> +
> +/**
> + * virDomainDetachDeviceFlags:
> + * @domain: pointer to domain object
> + * @xml: pointer to XML description of one device
> + * @flags: an OR'ed set of virDomainDeviceModifyFlags
> + *
> + * Detach a virtual device from a domain, using the flags parameter
> + * to control how the device is detached.  VIR_DOMAIN_DEVICE_MODIFY_CURRENT
> + * specifies that the device allocation is removed based on current domain
> + * state.  VIR_DOMAIN_DEVICE_MODIFY_LIVE specifies that the device shall be
> + * deallocated from the active domain instance only and is not from the
> + * persisted domain configuration.  VIR_DOMAIN_DEVICE_MODIFY_CONFIG
> + * specifies that the device shall be deallocated from the persisted domain
> + * configuration only.  Note that the target hypervisor must return an
> + * error if unable to satisfy flags.  E.g. the hypervisor driver will
> + * return failure if LIVE is specified but it only supports removing the
> + * persisted device allocation.
> + *
> + * Returns 0 in case of success, -1 in case of failure.
> + */
> +int
> +virDomainDetachDeviceFlags(virDomainPtr domain,
> +                           const char *xml, unsigned int flags)
> +{
> +    virConnectPtr conn;
> +    DEBUG("domain=%p, xml=%s, flags=%d", domain, xml, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        return (-1);
> +    }
> +    if (domain->conn->flags & VIR_CONNECT_RO) {
> +        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
>      conn = domain->conn;
>  
> -    if (conn->driver->domainDetachDevice) {
> +    if (conn->driver->domainDetachDeviceFlags) {
>          int ret;
> -        ret = conn->driver->domainDetachDevice (domain, xml);
> +        ret = conn->driver->domainDetachDeviceFlags(domain, xml, flags);
>          if (ret < 0)
>              goto error;
>          return ret;
>      }
>  
> -    virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +    virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
>  
>  error:
>      virDispatchError(domain->conn);
> -- 


The same with the detach operation. Can you re-post this one such that it
just adds the new methods, but leaves the current ones alone.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|