On Wed, 28 Jul 2010, Gianni Tedesco (3P) wrote:
> 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?
>
Yep
> > > +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.
>
Yes but libxl_device_pci_list_assignable is a public function, we don't
know what libxl users are going to do with it.
In particular it seems reasonable to call
libxl_device_pci_list_assignable before assigning a device even if xl
doesn't do it at the moment.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home