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

[Daniel P. Berrange]

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