[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