[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 0/2] libxl: add PV display device driver interface
On 23.03.17 17:55, al1img . wrote:
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.
To summarize:
There are two ways to rework the patches:
1. Keep interface between xl an libxl as is and put duplicated code into
libxl_device_type specific functions.
2. Change interface to call libxl_device_type specific functions
directly from xl:
libxl_device_vtpm *vtpms;
vtpms = (libxl_device_vtpm*)libxl_device_list_get(LIBXL_DEVTYPE_VTPM, ...);
...
libxl__device_list_free(LIBXL_DEVTYPE_VTPM, vtpms, nr);
But I need feedback from maintainers.
Adding maintainers to CC.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
