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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Mar 06, 2015 at 04:25:54PM +0100, Olaf Hering wrote:
> 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?
> 

I didn't look into details. I had the impression that there were lots of
manual string parsing.

> > 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.

I think it would, because 1) it only requires me to focus on one
specific aspect, 2) it enables we commit work that is ready.

But I don't have very strong opinion on this, so you can keep using one
patch.

> 
> > > +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?
> 

No. I mean you need to modify this function to comply with locking
scheme and update JSON template.

libxl_internal.h documents how this supposes to work.  You can use
libxl__device_vtpm_add as an example to see how it is actually
implemented.

See

2052     if (aodev->update_json) {
2053         lock = libxl__lock_domain_userdata(gc, domid);
2054         if (!lock) {
2055             rc = ERROR_LOCK_FAIL;
2056             goto out;
2057         }
2058
2059         rc = libxl__get_domain_configuration(gc, domid, &d_config);
2060         if (rc) goto out;
2061
2062         DEVICE_ADD(vtpm, vtpms, domid, &vtpm_saved, COMPARE_DEVID, 
&d_config);
2063     }
2064

That handles JSON template update when you do device hotplug.

> > > +{
> > > +    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
> 

Sorry about the inconsistency in code.

Let's not introduce more code that violates coding style. :-)

> > > +    /* 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.
> 

I need to do some more research before commenting on this.

> > Please avoid using goto to retry transaction.
> 
> Just copy&paste from other code, just grep for "retry_transaction".
> 

New code should use for or while to do this. We would very much like to
fix those goto loop but just haven't got there.

Wei.

> > > +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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel