[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 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);

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]