* new idl helper, append to Array
@ 2016-02-04 9:23 Olaf Hering
2016-02-04 9:58 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Olaf Hering @ 2016-02-04 9:23 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: new idl helper, append to Array
2016-02-04 9:23 new idl helper, append to Array Olaf Hering
@ 2016-02-04 9:58 ` Ian Campbell
2016-02-04 10:07 ` Olaf Hering
2016-02-04 10:45 ` Wei Liu
0 siblings, 2 replies; 7+ messages in thread
From: Ian Campbell @ 2016-02-04 9:58 UTC (permalink / raw)
To: Olaf Hering, Wei Liu; +Cc: xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: new idl helper, append to Array
2016-02-04 9:58 ` Ian Campbell
@ 2016-02-04 10:07 ` Olaf Hering
2016-02-04 10:48 ` Wei Liu
2016-02-05 9:55 ` Olaf Hering
2016-02-04 10:45 ` Wei Liu
1 sibling, 2 replies; 7+ messages in thread
From: Olaf Hering @ 2016-02-04 10:07 UTC (permalink / raw)
To: Ian Campbell; +Cc: Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: new idl helper, append to Array
2016-02-04 9:58 ` Ian Campbell
2016-02-04 10:07 ` Olaf Hering
@ 2016-02-04 10:45 ` Wei Liu
1 sibling, 0 replies; 7+ messages in thread
From: Wei Liu @ 2016-02-04 10:45 UTC (permalink / raw)
To: Ian Campbell; +Cc: Olaf Hering, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: new idl helper, append to Array
2016-02-04 10:07 ` Olaf Hering
@ 2016-02-04 10:48 ` Wei Liu
2016-02-05 9:55 ` Olaf Hering
1 sibling, 0 replies; 7+ messages in thread
From: Wei Liu @ 2016-02-04 10:48 UTC (permalink / raw)
To: Olaf Hering; +Cc: Wei Liu, Ian Campbell, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: new idl helper, append to Array
2016-02-04 10:07 ` Olaf Hering
2016-02-04 10:48 ` Wei Liu
@ 2016-02-05 9:55 ` Olaf Hering
2016-02-05 10:06 ` Ian Campbell
1 sibling, 1 reply; 7+ messages in thread
From: Olaf Hering @ 2016-02-05 9:55 UTC (permalink / raw)
To: Ian Campbell; +Cc: Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: new idl helper, append to Array
2016-02-05 9:55 ` Olaf Hering
@ 2016-02-05 10:06 ` Ian Campbell
0 siblings, 0 replies; 7+ messages in thread
From: Ian Campbell @ 2016-02-05 10:06 UTC (permalink / raw)
To: Olaf Hering; +Cc: Wei Liu, xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-05 10:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-04 9:23 new idl helper, append to Array Olaf Hering
2016-02-04 9:58 ` Ian Campbell
2016-02-04 10:07 ` Olaf Hering
2016-02-04 10:48 ` Wei Liu
2016-02-05 9:55 ` Olaf Hering
2016-02-05 10:06 ` Ian Campbell
2016-02-04 10:45 ` Wei Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.