Re: [PATCH v3 4/4] libxl: add support for vscsi

[Olaf Hering <olaf@aepfle.de>]

On Fri, Mar 06, Wei Liu wrote:

> I think you need to fix some overly long lines. I won't mention them
> individually inline.

Some are just copy&paste from other places. I will check what can be
trimmed.

> Regarding all the parsing stuffs, you haven't defined vscsispec so I
> cannot review it. You might want to look at
> docs/misc/xl-disk-configuration.txt.

Its in pvscsi.txt, not complete yet.

> I'm not suggesting you have to write something like that, but
> considering all the compatibility issues you might have you might
> actually end up writing up a document like that.

I will check if its needed.

> Then after that, do you consider writing a lexer for vscsispec?
> We have one for diskspec, see libxlu_disk_l.l. That might result in a
> shorter patch?

The bulk is not the parser but using the result of the parsing. Or do
you spot something that is better done with flex?

> On Fri, Mar 06, 2015 at 10:45:56AM +0100, Olaf Hering wrote:
> > Port pvscsi support from xend to libxl. See pvscsi.txt for details.
> > Outstanding work is listed in the TODO section.
> There is no TODO section in this patch. :-)

See the line above, its in pvscsi.txt.

> Better split this patch into two. One for libxl and one for xl.
> 
> Or you can even split it into three, one for introducing libxl types,
> one for implementing functionalities in libxl and one for xl.

Would that actually improve things? In another thread it was suggested
to have it all in a single patch to avoid jumping around between mails.

> > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid,
> > +                             libxl_device_vscsi *vscsi,
> > +                             libxl__ao_device *aodev)
> You need to update this domain's JSON configuration. Cf.
> libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310.

So I need to add my function to the comment near DEFINE_DEVICE_ADD()?
Or do you mean something else?

> > +{
> > +    STATE_AO_GC(aodev->ao);
> > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    flexarray_t *front;
> > +    flexarray_t *back;
> > +    libxl__device *device;
> > +    char *be_path;
> > +    unsigned int be_dirs = 0, rc, i;
> > +
> > +    if (vscsi->devid == -1) {
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> > +    /* Prealloc key+value: 4 toplevel + 4 per device */
> > +    i = 2 * (4 + (4 * vscsi->num_vscsi_devs));
> > +    back = flexarray_make(gc, i, 1);
> > +    front = flexarray_make(gc, 2 * 2, 1);
> > +
> > +    GCNEW(device);
> > +    rc = libxl__device_from_vscsi(gc, domid, vscsi, device);
> > +    if ( rc != 0 ) goto out;
> > +
> Coding style. No space after ( and before ). You can even just use
>      if (!rc) goto out;

Copy&paste from some similar code from staging-4.4:
libxl__device_vtpm_add

> > +    /* Check if backend device path is already present */
> > +    be_path = libxl__device_backend_path(gc, device);
> > +    if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) {
> > +        /* backend does not exist, create a new one */
> > +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> > +        flexarray_append_pair(back, "online", "1");
> > +        flexarray_append_pair(back, "state", "1");
> > +        flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host));
> > +
> > +        flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> > +        flexarray_append_pair(front, "state", "1");
> > +    }
> > +
> > +    for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> > +        libxl_vscsi_dev *v = vscsi->vscsi_devs + i;
> > +        /* Trigger removal, otherwise create new device */
> Why do you want to trigger removal in "add" function?

Because the vscsi is a single_host:many_devices, contrary to
single_host:singe_device for other backends.

> Please avoid using goto to retry transaction.

Just copy&paste from other code, just grep for "retry_transaction".

> > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> > +                                   libxl_vscsi_dev *dev)
> > +{
> 
> Can this only be an internal function? I.e. use libxl__ namespace.

Ok, will add the underscore.

Olaf