On Mon, Jan 25, 2010 at 04:16:59PM -0700, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Thu, Jan 14, 2010 at 10:42:38AM -0700, Jim Fehlig wrote:
virDomain{Attach,Detach}Device is now only permitted on active domains. Explicitly state this restriction in the API documentation and enforce it in libvirt frontend. --- src/libvirt.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index d973907..1145561 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5121,7 +5121,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Create a virtual device attachment to backend. + * Create a virtual device attachment to backend. This function, + * having hotplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5142,6 +5143,10 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } + if (!virDomainIsActive(domain)) { + virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); + goto error; + } conn = domain->conn;
if (conn->driver->domainAttachDevice) { @@ -5164,7 +5169,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Destroy a virtual device attachment to backend. + * Destroy a virtual device attachment to backend. This function, + * having hot-unplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5185,6 +5191,10 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml) virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; } + if (!virDomainIsActive(domain)) { + virLibDomainError(domain, VIR_ERR_OPERATION_INVALID, __FUNCTION__); + goto error; + } conn = domain->conn;
if (conn->driver->domainDetachDevice) { --
Agreed with the added doc comments, but I think I prefer that we do the check for active in the drivers themselves, at time of use. Doing the check at the top level is open to race conditions, since no locks are held on the objects in question at this point. Drivers will thus have to do extra checks themselves, making this top level one redundant. So I think we should just add the docs here.
Ok. Modified patch below.
Thanks, Jim
commit d8ec244c6513b7c44956a547e56c228a4c38fbbe Author: Jim Fehlig <jfehlig@novell.com> Date: Wed Jan 13 18:24:51 2010 -0700
doc: restrict virDomain{Attach,Detach}Device to active domains
virDomain{Attach,Detach}Device is now only permitted on active domains. Explicitly state this restriction in the API documentation.
V2: Only change doc, dropping the hunk that forced the restriction in libvirt frontend.
diff --git a/src/libvirt.c b/src/libvirt.c index d973907..188b991 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5121,7 +5121,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Create a virtual device attachment to backend. + * Create a virtual device attachment to backend. This function, + * having hotplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */ @@ -5164,7 +5165,8 @@ error: * @domain: pointer to domain object * @xml: pointer to XML description of one device * - * Destroy a virtual device attachment to backend. + * Destroy a virtual device attachment to backend. This function, + * having hot-unplug semantics, is only allowed on an active domain. * * Returns 0 in case of success, -1 in case of failure. */
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 :|