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

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.