From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: new idl helper, append to Array Date: Thu, 4 Feb 2016 10:45:41 +0000 Message-ID: <20160204104541.GL23178@citrix.com> References: <20160204092353.GA17709@aepfle.de> <1454579894.25207.144.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <1454579894.25207.144.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Olaf Hering , Wei Liu , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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__list_append as the naming scheme, which fits in > with libxl__list_free (which probably ought to be autogenerated too, > but isn't). So: > = > libxl_device_vscsidev_list_append(libxl_ctx *ctx, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 libxl= _device_vscsidev *dev, int nr, > =A0 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__append_, 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 ma= ke > use of this helper too. As could xl_cmdimpl.c for ARRAY_EXTEND_INIT perha= ps > (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, > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0libxl_device_vscsidev *dev) > > { > > =A0=A0=A0=A0int rc; > > =A0=A0=A0=A0libxl_device_vscsidev *devs; > > = > > =A0=A0=A0=A0devs =3D libxl__realloc(NOGC, ctrl->vscsidevs, sizeof(*dev)= * (ctrl->num_vscsidevs + 1)); > > =A0=A0=A0=A0if (!devs) { > > =A0=A0=A0=A0=A0=A0=A0=A0rc =3D ERROR_NOMEM; > > =A0=A0=A0=A0=A0=A0=A0=A0goto out; > > =A0=A0=A0=A0} > > = > > =A0=A0=A0=A0ctrl->vscsidevs =3D devs; > > =A0=A0=A0=A0libxl_device_vscsidev_init(ctrl->vscsidevs + ctrl->num_vscs= idevs); > > =A0=A0=A0=A0libxl_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. > > =A0=A0=A0=A0ctrl->num_vscsidevs++; > > =A0=A0=A0=A0rc =3D 0; > > out: > > =A0=A0=A0=A0return rc; > > } > > = > > = > > Olaf