Re: [Xen-devel] [PATCH 0/2] libxl: add PV display device driver interface

["al1img ." <al1img@xxxxxxxxx>]

On Thu, Mar 23, 2017 at 4:58 PM, Juergen Gross <jgross@xxxxxxxx> wrote:
> On 23/03/17 15:23, al1img . wrote:
>> This example is clear. But still wrapper macro is required to make it
>> visible for libxen client (xl):
>>
>> #define LIBXL_DEFINE_DEVICE_LIST_FREE(type)
>>     void libxl_device_##type##_list_free(libxl_device_##type* list, int nr)
>>     {
>>         libxl__device_list_free(libxl__##type##_devtype, list, nr);
>>     }
>
> Either this or we could switch to a more generic way avoiding the need
> to add multiple very similar functions to libxl:
>
> #define LIBXL_DEVTYPE_VTPM "vtpm"
>
> int libxl_device_list_free(const char *type, void *list, int nr)
> {
>     int i;
>
>     for (i = 0; device_type_tbl[i]; i++) {
>         if (!strcmp(type, device_type_tbl[i]->type)) {
>            libxl__device_list_free(device_type_tbl[i], list, nr);
>            return 0;
>         }
>     }
>     return ERROR_INVAL;
> }
>
> and let xl call it via libxl_device_list_free(LIBXL_DEVTYPE_VTPM, ...)
>
> This is subject to an Ack from the tools maintainers, of course.
>
>>
>> Also where should this function (libxl__device_list_free) be located?
>> libxl_device.c?
>
> I think so.
>
>>
>> Another case is getting this list:
>>
>> From one side each device should have its own implementation, from
>> other side for PV devices
>> there is common part like getting devId and backend domId and unique
>> part like getting
>> device specific parameters (uuid for vtpm). In this case I can do following:
>>
>> Implement void *libxl__device_list(struct libxl_device_type *dt,
>> libxl_ctx *ctx, uint32_t domid, int *num)
>> where I will get devId and backend domId. Then add get_params callback
>> to libxl_device_type to get device specific
>> parameters:
>>
>> void *libxl__device_pv_list(struct libxl_device_type *dt, libxl_ctx
>> *ctx, uint32_t domid, int *num)
>> {
>>     ...
>>     if (dt->get_patams) {
>>          dt->get_params(...);
>>     }
>> }
>>
>> And vtpm get list may look like:
>>
>> libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t
>> domid, int *num)
>> {
>>     return libxl__device_list(&libxl__vtpm_devtype, ctx, domid, num);
>> }
>>
>> DEFINE_DEVICE_TYPE_STRUCT(vtpm,
>>     .update_config = libxl_device_vtpm_update_config
>>     .get_params = libxl_device_vtpm_get_params
>> );
>>
>> Correct?
>
> Right. Possibly using the same trick as above.
>
> BTW: Please don't top-post.
>
>
> Juergen
>
>>
>>
>> 2017-03-23 14:08 GMT+02:00 Juergen Gross <jgross@xxxxxxxx>:
>>> On 23/03/17 12:32, al1img . wrote:
>>>> Hi Juergen,
>>>>
>>>> I've checked the mentioned commits. And I don't see how it can be done.
>>>> The duplication I see it is in libxl_device_type.add and
>>>> libxl_device_type.list functions
>>>> for different PV devices. These functions have a lot of common code
>>>> which I've tried
>>>> to move to macros in my patches.
>>>
>>> Okay, here an example for replacement of LIBXL_DEFINE_DEVICE_LIST_FREE()
>>>
>>> void libxl_device_list_free(struct libxl_device_type *dt,
>>>                             void *list, int nr)
>>> {
>>>     int i;
>>>
>>>     for (i = 0; i < nr; i++)
>>>         dt->dispose(list + i * dt->dev_elem_size);
>>>     free(list);
>>> }
>>>
>>> BTW: exactly this pattern is to be found at the very end of
>>> libxl_retrieve_domain_configuration().
>>>
>>> The others should be doable, too.
>>>
>>>
>>> Juergen
>>>
>>>>
>>>> 2017-03-23 12:21 GMT+02:00 Juergen Gross <jgross@xxxxxxxx>:
>>>>> On 23/03/17 11:10, Oleksandr Grytsov wrote:
>>>>>> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> We are working on series of PV drivers (display, sound, input etc.) and
>>>>>> would like to add their support to libxl and xl. These patches add PV 
>>>>>> display
>>>>>> device. PV display is based on [1] protocol.
>>>>>>
>>>>>> During implementation I see a lot of code duplication. This problem was
>>>>>> mentioned in [2]. One of the solutions was to implement common parts in 
>>>>>> IDL
>>>>>> and make them autogenerated. On my side, to minimize the copy/pasting
>>>>>> I've moved common parts into macro functions: LIBXL_DEFINE_DEVICE_COMMIT,
>>>>>> LIBXL_DEFINE_DEVICE_LIST_GET, LIBXL_DEFINE_DEVICE_GETINFO etc.
>>>>>> Existing PV devices implementations can be reworked to use these macros 
>>>>>> as
>>>>>> well. Any other proposals to avoid the duplications are welcome.
>>>>>
>>>>> Did you look into the device type framework I introduced with commit
>>>>> 74e857c6c7f9 and some followups? Maybe it is possible to expand this
>>>>> framework by adding more callbacks to struct libxl_device_type and
>>>>> have some common function(s) in libxl_device.c?
>>>>>
>>>>> Juergen
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>

For me the drawback of more generic way is to cast device_type to
void* and back which
may be used by client in improper way.

Thanks.

-- 
Best Regards,
Oleksandr Grytsov.

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