Re: new idl helper, append to Array

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Feb 04, 2016 at 09:58:14AM +0000, Ian Campbell wrote:
> 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?
> 

I don't think it is absolutely necessary because every field inside dst
will be overwritten, but better safe than sorry.

Wei.

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