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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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