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@xxxxxxxxxx> > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list