[libvirt] [PATCH V2] xen: check if device is assigned to guest before reattaching

Yufang Zhang yuzhang at redhat.com
Wed Apr 27 12:14:41 UTC 2011


On 04/26/2011 10:26 PM, Eric Blake wrote:
> On 01/07/2011 12:15 AM, Yufang Zhang wrote:
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=664059
>>
>> This is the version2 patch for BZ#664059. Reattaching pci device back to
>> host without destroying guest or detaching device from guest would cause
>> host to crash. This patch adds a check before doing device reattach. If
>> the device is being assigned to guest, libvirt refuses to reattach device
>> to host. The patch only works for Xen, for it just checks xenstore to get
>> pci device information.
>>
>> Signed-off-by: Yufang Zhang<yuzhang at redhat.com>
>> ---
>>   src/xen/xen_driver.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 67 insertions(+), 0 deletions(-)
> Wow, just noticed that this hasn't had any review - does it still apply
> to the latest upstream?
>
The v3 patch has been sent which is based on latest upstream.
>> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
>> index 4c11b11..318bb6a 100644
>> --- a/src/xen/xen_driver.c
>> +++ b/src/xen/xen_driver.c
>> @@ -1891,11 +1891,70 @@ out:
>>   }
>>
>>   static int
>> +xenUnifiedNodeDeviceAssignedDomainId (virNodeDevicePtr dev)
>> +{
>> +    int numdomains;
>> +    int ret = -1, i;
>> +    int *ids = NULL;
>> +    char *bdf = NULL;
>> +    char *xref = NULL;
>> +    unsigned int domain, bus, slot, function;
>> +    virConnectPtr conn = dev->conn;
>> +    xenUnifiedPrivatePtr priv = conn->privateData;
>> +
>> +    /* Get active domains */
>> +    numdomains = xenUnifiedNumOfDomains(conn);
>> +    if (numdomains<  0) {
>> +        return ret;
>> +    }
>> +    if (numdomains>  0){
>> +        if (VIR_ALLOC_N(ids, numdomains)<  0){
> Formatting nit - space before {, as in:
>
> if (VIR_ALLOC_N(ids, numdomains)<  0) {
>
>> +            virReportOOMError();
>> +            goto out;
>> +        }
>> +        if ((numdomains = xenUnifiedListDomains(conn,&ids[0], numdomains))<  0){
>> +            goto out;
>> +        }
>> +    }
>> +
>> +    /* Get pci bdf */
>> +    if (xenUnifiedNodeDeviceGetPciInfo(dev,&domain,&bus,&slot,&function)<  0)
>> +        goto out;
>> +
>> +    if (virAsprintf(&bdf, "%04x:%02x:%02x.%0x",
>> +                    domain, bus, slot, function)<  0) {
>> +        virReportOOMError();
>> +        goto out;
>> +    }
>> +
>> +    /* Check if bdf is assigned to one of active domains */
>> +    for (i = 0; i<  numdomains; i++ ){
> Again, a formatting nit:
>
> for (i = 0; i<  numdomains; i++) {
>
>> +        xenUnifiedLock(priv);
>> +        xref = xenStoreDomainGetPCIID(conn, ids[i], bdf);
>> +        xenUnifiedUnlock(priv);
> This locks and unlocks around every iteration of the loop.  Would it be
> better to hold the lock for the entire loop duration, so that the action
> is atomic (no other thread can interrupt things), or is it better to
> drop the lock each iteration?  I'm not as familiar with the xen
> architecture to give a good answer to this off the top of my head.
>
I think moving the lock-unlock pair out of loop should work.
>> +        if (xref == NULL)
>> +            continue;
>> +        else {
> Formatting - if one branch of an if-else has {}, then both need it:
>
> if (xref == NULL) {
>      continue;
> } else {
>
>> +            ret = ids[i];
>> +            break;
>> +        }
>> +    }
>> +
>> +    VIR_FREE(xref);
>> +    VIR_FREE(bdf);
>> +out:
>> +    VIR_FREE(ids);
>> +
>> +    return ret;
>> +}
>> +
>> +static int
>>   xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev)
>>   {
>>       pciDevice *pci;
>>       unsigned domain, bus, slot, function;
>>       int ret = -1;
>> +    int domid;
>>
>>       if (xenUnifiedNodeDeviceGetPciInfo(dev,&domain,&bus,&slot,&function)<  0)
>>           return -1;
>> @@ -1904,6 +1963,14 @@ xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev)
>>       if (!pci)
>>           return -1;
>>
>> +    /* Check if device is assigned to an active guest */
>> +    if ((domid = xenUnifiedNodeDeviceAssignedDomainId(dev))>= 0){
>> +        xenUnifiedError(VIR_ERR_INTERNAL_ERROR,
>> +                        _("Device %s has been assigned to guest %d"),
>> +                        dev->name, domid);
>> +        goto out;
>> +    }
>> +
>>       if (pciReAttachDevice(pci, NULL)<  0)
>>           goto out;
>>




More information about the libvir-list mailing list