new idl helper, append to Array

new idl helper, append to Array

[Olaf Hering]

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()?

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.



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);
    ctrl->num_vscsidevs++;
    rc = 0;
out:
    return rc;
}


Olaf

Re: new idl helper, append to Array

[Ian Campbell]

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

Re: new idl helper, append to Array

[Olaf Hering]

On Thu, Feb 04, Ian Campbell wrote:

> I think the append_to variant is probably least gross.

libxl_device_vscsidev_append_to_vscsictrl() would work too.

This is what I will use for the time being (modulo introduced runtime bugs):

void libxl_device_vscsictrl_append_vscsidev(libxl_ctx *ctx,
                                            libxl_device_vscsictrl *ctrl,
                                            libxl_device_vscsidev *dev)
{
    GC_INIT(ctx);
    ctrl->vscsidevs = libxl__realloc(NOGC, ctrl->vscsidevs, sizeof(*dev) * (ctrl->num_vscsidevs + 1));
    libxl_device_vscsidev_init(ctrl->vscsidevs + ctrl->num_vscsidevs);
    libxl_device_vscsidev_copy(CTX, ctrl->vscsidevs + ctrl->num_vscsidevs, dev);
    ctrl->num_vscsidevs++;
    GC_FREE;
}


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

It would clear the padding between members, a plain copy will leave them
uninitialized.

Olaf

Re: new idl helper, append to Array

[Wei Liu]

On Thu, Feb 04, 2016 at 11:07:58AM +0100, Olaf Hering wrote:
> On Thu, Feb 04, Ian Campbell wrote:
> 
> > I think the append_to variant is probably least gross.
> 
> libxl_device_vscsidev_append_to_vscsictrl() would work too.
> 
> This is what I will use for the time being (modulo introduced runtime bugs):
> 
> void libxl_device_vscsictrl_append_vscsidev(libxl_ctx *ctx,
>                                             libxl_device_vscsictrl *ctrl,
>                                             libxl_device_vscsidev *dev)
> {
>     GC_INIT(ctx);
>     ctrl->vscsidevs = libxl__realloc(NOGC, ctrl->vscsidevs, sizeof(*dev) * (ctrl->num_vscsidevs + 1));
>     libxl_device_vscsidev_init(ctrl->vscsidevs + ctrl->num_vscsidevs);
>     libxl_device_vscsidev_copy(CTX, ctrl->vscsidevs + ctrl->num_vscsidevs, dev);
>     ctrl->num_vscsidevs++;
>     GC_FREE;
> }
> 
> 
> > Wei, is it necessary to init the dst before copy into it?
> 
> It would clear the padding between members, a plain copy will leave them
> uninitialized.
> 

There is that. Yes, _init memset the whole structure to zeros.

Wei.

> Olaf

Re: new idl helper, append to Array

[Olaf Hering]

On Thu, Feb 04, Olaf Hering wrote:

> On Thu, Feb 04, Ian Campbell wrote:
> 
> > I think the append_to variant is probably least gross.
> 
> libxl_device_vscsidev_append_to_vscsictrl() would work too.

While looking at the MERGE macro in libxl.c, a _remove_from could be
added as well. I have not checked if there are other users beside the
the MERGE function.

Olaf

Re: new idl helper, append to Array

[Ian Campbell]

On Fri, 2016-02-05 at 10:55 +0100, Olaf Hering wrote:
> On Thu, Feb 04, Olaf Hering wrote:
> 
> > On Thu, Feb 04, Ian Campbell wrote:
> > 
> > > I think the append_to variant is probably least gross.
> > 
> > libxl_device_vscsidev_append_to_vscsictrl() would work too.
> 
> While looking at the MERGE macro in libxl.c, a _remove_from could be
> added as well. I have not checked if there are other users beside the
> the MERGE function.

If you have the tuits please feel free to arrange to autogenerate as much
as you like ;-)

(If you are unsure about the general utility you could always make them
internal only for now)

Ian.

Re: new idl helper, append to Array

[Wei Liu]

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