[Xen-devel] Re: [PATCH 2 of 3] xl, implement pci-list-assignable-devices

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Wed, 2010-07-28 at 18:39 +0100, Stefano Stabellini wrote:
> > +                }
> > +                libxl_free(ctx, bdf);
> > +                libxl_free(ctx, devpath);
> > +            }
> > +            libxl_free(ctx, num_devs);
> > +        }
> > +        libxl_free(ctx, path);
> > +    }
> > +    libxl_free(ctx, domlist);
> > +
> > +    return pcidevs;
> > +}
> 
> You shouldn't use libxl_free explicitly on variables allocated in the
> context. Actually I like explicit free's but we have to keep the style
> consistent, so the memory management refactoring should come in as a
> separate patch, and here there shouldn't be any libxl_free's.
> The realloc is correct because it is used on a variable that is going
> to be returned to the caller.

OK, this was written before the idea for refactoring memory management
and just done out of habit. Are you saying just remove the libxl_free's
and re-submit it like that?

> Also I would prefer this function to return an integer and take the
> libxl_device_pci array as a parameter.

Hmm, OK.

> > +libxl_device_pci *libxl_device_pci_list_assignable(struct libxl_ctx *ctx, 
> > int *num)
> > +{
> > +    libxl_device_pci *pcidevs = NULL;
> > +    libxl_device_pci *assigned;
> > +    int num_assigned;
> > +
> > +    *num = 0;
> > +
> > +    assigned = get_all_assigned_devices(ctx, &num_assigned);
> > +
> > +    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> > +                              "/sys/bus/pci/drivers/pciback", num);
> > +    pcidevs = scan_sys_pcidir(pcidevs, assigned, num_assigned,
> > +                              "/sys/bus/pci/drivers/pcistub", num);
> > +
> > +    free(assigned);
> > +    return pcidevs;
> > +}
> > +
> 
> Same comment about returning an integer, also I don't like the way you
> are "assuming" scan_sys_pcidir is going to use realloc on pcidevs: that
> should be an implementation detail of scan_sys_pcidir that callers
> shouldn't rely upon.

Actually it's a static function with only one caller, this makes the
implementation of list_assignable as simple as possible.

> Consider that devices bound to pcistub cannot be assigned to PV guests,
> so I would remove scan_sys_pcidir on pcistub from here.
> Please define "/sys/bus/pci/drivers/pciback" in a macro.

But they can be assigned to HVM guests? Nothing in the add-device path
uses this list of devices anyway. What actually happens is a check
whether a device appears in the get_all_assigned_devices() list. Even
after this patch-set the user can always specify BDF's which aren't
really assignable (eg. the Host's PCI-to-ISA bridge or PCI root for
maximum amusement) That is to be addressed in a future patch.

Whether-or-not a specific device can be added is an issue for the
add-device path. I think that the best approach there would then be just
to stat the relevant sysfs paths to make sure the device is available.

As for the macro, of course, I will change that.

Thanks for the comments.

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel