Re: [PATCH v4 01/13] libxl: add generic function to add device

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Sep 05, 2017 at 07:44:34PM +0300, Oleksandr Grytsov wrote:
> On Tue, Sep 5, 2017 at 2:47 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote:
> >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >>
> >> Add libxl__device_add to simple write XenStore device conifg
> >> and libxl__device_add_async to update domain configuration
> >> and write XenStore device config asynchroniously.
> >> Almost all devices have similar libxl__device_xxxx_add function.
> >> This generic functions implement same functionality but
> >> using the device handling framework. Th device specific
> >> part such as setting xen store configurationis moved
> >> to set_xenstore_config callback of the device framework.
> >>
> >
> > The two add functions look correct.
> >
> > Some comments below.
> >
> >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >> ---
> >>  tools/libxl/libxl_create.c   |   3 +
> >>  tools/libxl/libxl_device.c   | 198 +++++++++++++++++++++++++++++++++++++++++++
> >>  tools/libxl/libxl_disk.c     |   2 +
> >>  tools/libxl/libxl_internal.h |  36 ++++++++
> >>  tools/libxl/libxl_nic.c      |   2 +
> >>  tools/libxl/libxl_pci.c      |   2 +
> >>  tools/libxl/libxl_usb.c      |   6 ++
> >>  tools/libxl/libxl_vtpm.c     |   2 +
> >>  8 files changed, 251 insertions(+)
> >>
> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> >> index bffbc45..b2163cd 100644
> >> --- a/tools/libxl/libxl_create.c
> >> +++ b/tools/libxl/libxl_create.c
> >> @@ -1430,6 +1430,9 @@ out:
> >>
> >>  #define libxl_device_dtdev_list NULL
> >>  #define libxl_device_dtdev_compare NULL
> >> +#define libxl__device_from_dtdev NULL
> >> +#define libxl__device_dtdev_setdefault NULL
> >> +#define libxl__device_dtdev_update_devid NULL
> >>  static DEFINE_DEVICE_TYPE_STRUCT(dtdev);
> >>
> >>  const struct libxl_device_type *device_type_tbl[] = {
> >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> >> index 00356af..07165f0 100644
> >> --- a/tools/libxl/libxl_device.c
> >> +++ b/tools/libxl/libxl_device.c
> >> @@ -1793,6 +1793,204 @@ out:
> >>      return AO_CREATE_FAIL(rc);
> >>  }
> >>
> >> +static void device_add_domain_config(libxl__gc *gc,
> >> +                                     libxl_domain_config *d_config,
> >> +                                     const struct libxl_device_type *dt,
> >> +                                     void *type)
> >> +{
> >> +    int *num_dev;
> >> +    int i;
> >
> > unsigned int please.
> 
> For "i" counter only or for num_dev as well?
> For "i" is ok but num_dev better to keep int.
> 

For i only.

> >>   * mode: C
> >> diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
> >> index 63de75c..f2f3635 100644
> >> --- a/tools/libxl/libxl_disk.c
> >> +++ b/tools/libxl/libxl_disk.c
> >> @@ -1244,6 +1244,8 @@ static int libxl_device_disk_dm_needed(void *e, unsigned domid)
> >>             elem->backend_domid == domid;
> >>  }
> >>
> >> +#define libxl__device_disk_update_devid NULL
> >> +
> >
> > Is this correct for disk (and other device types as well)?
> 
> What exactly is correct? libxl__device_disk_update_devid NULL or
> libxl__device_add_async function?
> 

Defining all the update_devid functions to be NULL. They should be
defined with the macros now, right?

I notice in later patches they are changed, so I'm not too fuss either
way. If you want to keep them to be defined as  NULL please say so in
commit message.

> >
> > Since you've defined LIBXL_DEFINE_UPDATE_DEVID, you should be able to
> > use that immediately?
> 
> Actually disk doesn't call update dev ID. So assigning it to NULL I
> guess is ok here.
> 

Yes. I think so. Please consider other device types then.

> -- 
> Best Regards,
> Oleksandr Grytsov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel