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

[Daniel P. Berrange]

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.

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