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

[Daniel P. Berrange]

On Mon, Jan 25, 2010 at 04:22:12PM -0700, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> >>>> 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.
> >>>>         
> >> Ah, yes - good catch.  Thanks.
> >>
> >>     
> >>>>  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..
> >>>>     
> >>>>         
> >>> There's one other option actually - we could put compatability code in
> >>> the remote driver client instead. Either, make it always invoke the old
> >>> RPC call if flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE, or have it invoke
> >>> the new RPC call & fallback to the old RPC call if it gets an error
> >>> indicating the new one doesn't exist.
> >>>   
> >>>       
> >> Do you prefer the latter option?  After a quick look, I didn't spot any
> >> existing compatibility code in the remote driver client.  The first
> >> option might be slightly better wrt maintenance.
> >>     
> >
> > Yes, the first option is certainly simpler / clearer code todo, so lets
> > just do that first. We can easily revisit adding compat code in the
> > remote driver client at a later date if we find it to be neccessary.
> >   
> 
> Revised patch below.
> 
> Thanks,
> Jim
> 

> commit 487b2434403d520027957ed623354b398984af31
> Author: Jim Fehlig <jfehlig at novell.com>
> Date:   Wed Jan 13 18:34:23 2010 -0700
> 
>     Public API Implementation
>     
>     Implementation of public API for virDomain{Attach,Detach}DeviceFlags.
>     
>     V2: Don't break remote compatibility with older libvirtd
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 188b991..de802c4 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -5146,14 +5146,68 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml)
>      conn = domain->conn;
>  
>      if (conn->driver->domainAttachDevice) {
> +       int ret;
> +       ret = conn->driver->domainAttachDevice (domain, xml);
> +       if (ret < 0)
> +          goto error;
> +       return ret;
> +    }
> +
> +    virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    virDispatchError(domain->conn);
> +    return -1;
> +}
> +
> +/**
> + * 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->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);
> @@ -5192,12 +5246,66 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml)
>      if (conn->driver->domainDetachDevice) {
>          int ret;
>          ret = conn->driver->domainDetachDevice (domain, xml);
> +         if (ret < 0)
> +             goto error;
> +         return ret;
> +     }
> +
> +    virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +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->domainDetachDeviceFlags) {
> +        int ret;
> +        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);

ACK


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 :|