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
|
Home