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

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Feb 12, 2016 at 07:24:59PM +0100, Olaf Hering wrote:
[...]
> 
> > > +    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.
>

OK.

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

I'm confused. You're joking, right? "Has to be used somewhere" is not
a justification for having it in this particular place.

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

If it is mandated by hardware that empty scsi controller doesn't
exist, that's of course fine. But I don't think it is mandated in
reality? I can have no disk attached to a controller and that should
be fine.

> While toying around I noticed that removing all vscsidevs and
> leaving the vscsictrl in xenstore the frontend preserved its SCSI
> controller entry in sysfs.
>

That's what I would expect, too.

Wei.

> Olaf