Re: [libvirt] [PATCH 1/9] Restrict virDomain{Attach, Detach}Device to active domains

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(a)novell.com&gt; 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. */