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

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Wed, Sep 6, 2017 at 12:36 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxx> wrote:
>> > On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote:
>> >> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>> >>
>> >> 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@xxxxxxxx>
>> >> ---
>> >>  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.

Ok, I will use the macro in every device which need update devid in this patch.
Also I will call this function in add device function.

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