Re: new idl helper, append to Array

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2016-02-04 at 10:23 +0100, Olaf Hering wrote:
> Ian,
> 
> in my pvscsi code I have two copies of a helper function which appends
> yet another instance of something to an Array, as shown below. This is
> similar to the _copy variant. Is it worth to let gentypes generate such
> a helper, like libxl_device_vscsictrl_append_vscsidev()?

If something can be autogenerated without too much trouble then I see no
reason not to do so.

I'd go with libxl_<type>_list_append as the naming scheme, which fits in
with libxl_<type>_list_free (which probably ought to be autogenerated too,
but isn't). So:

libxl_device_vscsidev_list_append(libxl_ctx *ctx,
                                  libxl_device_vscsidev *dev, int nr,
				  libxl_device_vscsidev *new);
(if you intend for this to be internal then s/^libxl_/&_/ and
s/libxl_ctx \*ctx/libxl__gc *gc/)

Oh, I see you want it to take the type containing the array, that could
work to, you'd need to call it libxl_<type>_append_<field>, so

libxl_device_vscsictrl_append_vscsidevs

which looks a bit odd (since the field name is plural and the IDL has no
way to find the singular). We could live with that, or s/append/append_to/
or make it varargs and take perhaps multiple new entries and a NULL
terminator.

I think the append_to variant is probably least gross.

Looks like various places such as libxl__append_nic_list_of_type could make
use of this helper too. As could xl_cmdimpl.c for ARRAY_EXTEND_INIT perhaps
(using it everywhere isn't mandatory of course, but if you feel inclined it
would be nice)

> While writing this I realize that libxl__realloc will not return, so my
> helper can be converted from returning int to void, and all the locals
> can be removed.

Indeed.

> static int vscsi_append_dev(libxl__gc *gc, libxl_device_vscsictrl *ctrl,
>                             libxl_device_vscsidev *dev)
> {
>     int rc;
>     libxl_device_vscsidev *devs;
> 
>     devs = libxl__realloc(NOGC, ctrl->vscsidevs, sizeof(*dev) * (ctrl->num_vscsidevs + 1));
>     if (!devs) {
>         rc = ERROR_NOMEM;
>         goto out;
>     }
> 
>     ctrl->vscsidevs = devs;
>     libxl_device_vscsidev_init(ctrl->vscsidevs + ctrl->num_vscsidevs);
>     libxl_device_vscsidev_copy(CTX, ctrl->vscsidevs + ctrl->num_vscsidevs, dev);

Wei, is it necessary to init the dst before copy into it?

>     ctrl->num_vscsidevs++;
>     rc = 0;
> out:
>     return rc;
> }
> 
> 
> Olaf

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