Re: [PATCH v8 3/5] libxl: add support for vscsi

[Olaf Hering <olaf@aepfle.de>]

On Fri, Feb 12, Wei Liu wrote:

> There seems to be a lot of places in this patch where the lines are
> longer than 80 characters. I'm not going to point them out one by one.
> Can you fix as many of those as you can where sensible?

Many lines are slightly longer than 80 chars already, as can be seen
with 'set colorcolumn=80'. I already wrapped most of them in my patch.

> Because VSCSI is similar to PVUSB and PVUSB has mostly been reviewed, I
> basically did a side by side comparison between these two. My goal is to
> keep them as close as possible.

Unfortnuately they are not comparable. While I have now split the logic
into ctrl and dev, the code using a ctrl is just the one doing the
xenstore device work.

> > +    flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> > +    flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
> Wrap long lines please.

Done. I dont like the result.


> > +    if (libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs)) {
> > +        rc = libxl__device_vscsi_reconfigure_add(egc, aodev, &vscsi_saved, &d_config, be_path);
> > +        if (rc)
> > +            goto out;
> > +        /* Notify that this is done */
> > +        aodev->callback(egc, aodev);
> 
> Wouldn't it be better if you call aodev->callback unconditionally at the
> end of this function? BTW you seem to have forgotten to set aodev->rc.

I have to rework this code, its still using libxl__wait_for_backend.
This is already fixed in the detach case. Somehow I was only
concentrating on the detach case this week, and did not notice this.


> > +/* Remove vscsidev connected to vscsictrl */
> > +int libxl_device_vscsidev_remove(libxl_ctx *ctx, uint32_t domid,
> > +                                 libxl_device_vscsidev *dev,
> > +                                 const libxl_asyncop_how *ao_how)
> > +                                 LIBXL_EXTERNAL_CALLERS_ONLY;
> 
> Where is libxl_device_vscsidev_add?

This is done in scsi-attach, via libxl_device_vscsictrl_add. Its not
even, compared to the scsi-detach case. I will see if that can be
changed.


> > +    libxl__add_vscsictrls(egc, ao, domid, d_config, &dcs->multidev);
> 
> This is because you need vscsi disk early?

I think yes, DEFINE_DEVICES_ADD has to be used somewhere.


> > +        case XenbusStateClosing:
> > +        case XenbusStateClosed:
> > +            LOG(DEBUG, "unexpected state in %s: %s", path, s);
> > +            break;

> This doesn't seem useful in the context of this function.

A few lines up "state" is already printed. Looks like the whole state
checking can be removed.

> > +    return true;
> > +}
> > +
> > +static bool libxl__vscsi_fill_ctrl(libxl__gc *gc,
> > +                                   xs_transaction_t t,
> > +                                   const char *fe_path,
> > +                                   const char *dir,
> > +                                   libxl_device_vscsictrl *ctrl)
> > +{
> > +    libxl_device_vscsidev dev;
> > +    char *tmp, *be_path, *devs_path;
> > +    char **dev_dirs;
> > +    unsigned int ndev_dirs, dev_dir;
> > +    bool parsed_ok;
> > +
> > +    ctrl->devid = atoi(dir);
> > +
> > +    be_path = libxl__xs_read(gc, t, GCSPRINTF("%s/%s/backend", fe_path, dir));
> > +    /* FIXME what if xenstore is broken? */
> 
> I'm not sure what kind of "broken" you referred to, but it is most
> likely we can't recover from such state.
> 
> It's reasonable to assume xenstore is robust.

I will remove the FIXME comments ...

> > +    if (!be_path) {
> > +        libxl_defbool_set(&ctrl->scsi_raw_cmds, false);
> > +        return false;
> > +    }
> > +
> > +    tmp = libxl__xs_read(gc, t, GCSPRINTF("%s/%s/backend-id", fe_path, dir));
> > +    /* FIXME what if xenstore is broken? */

... and I will return failure here as well.

> > +    if (tmp)
> > +        ctrl->backend_domid = atoi(tmp);


> > +int main_vscsiattach(int argc, char **argv);
> > +int main_vscsilist(int argc, char **argv);
> > +int main_vscsidetach(int argc, char **argv);
> 
> What about other commands? Looking at PVUSB series:
> 
> +int main_usbctrl_attach(int argc, char **argv);
> +int main_usbctrl_detach(int argc, char **argv);
> +int main_usbdev_attach(int argc, char **argv);
> +int main_usbdev_detach(int argc, char **argv);
> +int main_usblist(int argc, char **argv);
> 
> so we should be able to attach / detach both controller and specific
> device?

There are no empty scsi controllers, just controllers with at least one
device. IMO its not useful to define empty controllers, what would be
the point? While toying around I noticed that removing all vscsidevs and
leaving the vscsictrl in xenstore the frontend preserved its SCSI
controller entry in sysfs.

Olaf