[libvirt] [PATCH] Device reattach: Check if device is assigned to guest before reattaching

[Osier Yang]

于 2011年01月05日 21:54, Yufang Zhang 写道:
> 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. Note that the patch only works for Xen,
> for it just checks xenstore to get pci device information.

So the subject of the patch could be:
xen: Check if device is assigned to guest before reattaching

>
> Signed-off-by: Yufang Zhang<yuzhang at redhat.com>
> ---
>   src/xen/xen_driver.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 67 insertions(+), 0 deletions(-)
>
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 4c11b11..5773d3b 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -1891,11 +1891,70 @@ out:
>   }
>
>   static int
> +xenUnifiedNodeDeviceIsAssigned (virNodeDevicePtr dev)
> +{
> +    int numdomains;
> +    int ret = -1, i;
> +    int *ids = NULL;
> +    char *bdf = NULL;
> +    char *xref = NULL;
> +    unsigned domain, bus, slot, function;

unsigned int

> +    virConnectPtr conn = dev->conn;
> +    xenUnifiedPrivatePtr priv = conn->privateData;
> +
> +    /* Get active domains */
> +    numdomains = xenUnifiedNumOfDomains(conn);
> +    if (numdomains<  0) {
> +        return(ret);

Generally we use "return ret;"

> +    }
> +    if (numdomains>  0){
> +        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++ ){
> +        xenUnifiedLock(priv);
> +        xref = xenStoreDomainGetPCIID(conn, ids[i], bdf);
> +        xenUnifiedUnlock(priv);
> +        if (xref == NULL)
> +            continue;
> +        else {
> +            ret = ids[i];
> +            break;
> +        }
> +    }
> +
> +    VIR_FREE(xref);
> +    VIR_FREE(bdf);
> +out:
> +    VIR_FREE(ids);
> +
> +    return(ret);
> +}

Generally, a function "*Is*" will return a bool value, TRUE/FALSE,
altough seems you want to get the domain ID to use in following
function to print. But does it make sense to find a better method?
or a better function name?

> +
> +static int
>   xenUnifiedNodeDeviceReAttach (virNodeDevicePtr dev)
>   {
>       pciDevice *pci;
>       unsigned domain, bus, slot, function;
>       int ret = -1;
> +    int dom;

 From the code, it should be "domain id" here. so probly you need
a better variable name here.

>
>       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 ((dom = xenUnifiedNodeDeviceIsAssigned(dev))>= 0){
> +        xenUnifiedError(VIR_ERR_INTERNAL_ERROR,
> +                        _("Device %s has been assigned to guest %d"),
> +                        dev->name, dom);
> +        goto out;
> +    }
> +
>       if (pciReAttachDevice(pci, NULL)<  0)
>           goto out;
>

Anyway, I like the idea, we have bug (host will crash or reboot if
try to reattach a PCI device to host which is using by guest) of
qemu driver. Though we can't resolve the problem in the similar way,
as kvm doesn't record the PCI/domain pair like xenstore. It still
gives some inspiration.

Thanks
Osier