Re: [Xen-devel] [PATCH v3 04/11] libxl: add generic function to add device

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
>> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>
>> Add libxl__device_add functio.
>
> function
>
>> Almost all devices have similar libxl__device_xxxx_add function.
>> This generic function implements same functionality but
>> using the device handling framework. The device specific
>> part this is setting xen store configuration. This part
>> is moved to set_xenstore_config callback of the device framework.
>>
>
> Right. I think this is a good idea in general.
>
> I don't see exiting device ported to the new framework, why?

Good question. I think it is a little dangerous and may introduce regression.
But definitely it should be done. I can do these changes but I don't have
visibility how to check each device.

>
> We really don't want two sets of code that does the same thing in libxl.
> That's a recipe for bugs.



>
>> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
> [...]
>>  /*
>> diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c
>> index a628adc..c79bcda 100644
>> --- a/tools/libxl/libxl_vdispl.c
>> +++ b/tools/libxl/libxl_vdispl.c
>> @@ -14,6 +14,21 @@
>>
>>  #include "libxl_internal.h"
>>
>> +static int libxl__device_vdispl_setdefault(libxl__gc *gc, uint32_t domid,
>> +                                           libxl_device_vdispl *vdispl)
>> +{
>> +    int rc;
>> +
>> +    rc = libxl__resolve_domid(gc, vdispl->backend_domname,
>> +                              &vdispl->backend_domid);
>> +
>> +    if (vdispl->devid == -1) {
>> +        vdispl->devid = libxl__device_nextid(gc, domid, "vdispl");
>> +    }
>> +
>
> No need to have {}.
>
>> +    return rc;
>> +}
>> +
>>  static int libxl__device_from_vdispl(libxl__gc *gc, uint32_t domid,
>>                                       libxl_device_vdispl *vdispl,
>>                                       libxl__device *device)
>> @@ -47,7 +62,7 @@ static void libxl__device_vdispl_add(libxl__egc *egc, 
>> uint32_t domid,
>>                                       libxl_device_vdispl *vdispl,
>>                                       libxl__ao_device *aodev)
>>  {
>> -
>> +    libxl__device_add(egc, domid, &libxl__vdispl_devtype, vdispl, aodev);
>>  }
>>
>>  libxl_device_vdispl *libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t 
>> domid,
>> --
>> 2.7.4
>>



-- 
Best Regards,
Oleksandr Grytsov.

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