Re: [Xen-devel] [PATCH 0/3] libxl: add framework for device types

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"):
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.

I think something like this is a jolly good idea.  Thanks a lot!

The rough shape - a set of structs with what amount to method calls -
seems a good direction to be going in.

I have some comments/questions.

I saw this in 2/3:

> +    for (i = 0; i < d_config->num_pcidevs; i++) {
> +        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> +        if (rc < 0) {
> +            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> +            goto out;
> +        }
> +    }
> +

And there is similar code in 3/3 for dtdevs.  Could that be lifted
away somehow ?  (You'd have to take some care about the types, sadly;
ie, I think libxl__device_pci_add might have to start to take a
void*; maybe some macros could make things typesafe?)


In 1/3:

> +struct libxl_device_type libxl__usbctrl_devtype = {
> +    .type       = "usbctrl",
> +    .num_offset = offsetof(libxl_domain_config, num_usbctrls),
> +    .add        = libxl__add_usbctrls,
> +};

And then num_offset is used like this:

> +        if (*(int *)((void *)d_config + dt->num_offset) > 0) {

This is a fine approach but I would prefer it if the there were a bit
more type safety, particularly in the parts that have to occur once
for each device type.

For example, there is nothing stopping one using this pattern with a
num_* field which is not an int.

Perhaps there should be a macro for generating the libxl_device_type
contents ?  It could perhaps take `usbctrls' and make `num_usbctrls'
out of it using token pasting.


Also these structs should be static const, I think.

Thanks again!

Ian.

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