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