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

[Juergen Gross <jgross@suse.com>]

On 06/07/16 13:04, Ian Jackson wrote:
> 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?)

I thought about this idea already. I think we would end up with more
code which would be rather unpleasant to read. Main reason is the
need for a dtdev wrapper function and the pci backend creation.

> 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.

I think 'usbctrl' as the macro parameter would be just fine: it would
allow to generate the complete struct:

#define DEFINE_DEVICE_STRUCT(name) \
  const libxl_device_type libxl__ ## name ## _devtype = { \
    .type       = # name , \
    .num_offset = offsetof(libxl_domain_config, num_ ## name ## s), \
    .add        = libxl__add_ ## name ## s, \
}

DEFINE_DEVICE_STRUCT(usbctrl);

> Also these structs should be static const, I think.

static: sometimes, const: yes.


Juergen

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