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

[Jim Fehlig]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01.patch
Type: text/x-patch
Size: 1315 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100125/75fab445/attachment-0001.bin>