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

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-12 at 17:20 +0100, Olaf Hering wrote:
> On Wed, Mar 11, Ian Campbell wrote:
> 
> > On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> > > +int libxl_device_vscsi_parse_pdev(libxl__gc *gc, char *pdev, unsigned int *hst,
> > > +                                unsigned int *chn, unsigned int *tgt,
> > > +                                unsigned int *lun)
> > > +{
> > > +
> > > +    return ERROR_NOPARAVIRT;
> > That's a rather odd error code to use here.
> > 
> > (also, unnecessary blank line)
> 
> What error should be returned?

Take a look at the list and find one that fits, or look at other similar
stub functions. If there is no suitable existing error code then feel
free to add one.

> Looks like this function can be moved to xlu. Should I introduce a
> libxlu_$(OS).c or just use #ifdef in libxlu_vscsi.c?

I think we could live with either.

> > > +libxl_vscsi_hctl = Struct("vscsi_hctl", [
> > > +    ("hst", uint32),
> > > +    ("chn", uint32),
> > > +    ("tgt", uint32),
> > > +    ("lun", uint32),
> > > +    ])
> > > +
> > > +libxl_vscsi_dev = Struct("vscsi_dev", [
> > > +    ("vscsi_dev_id",     libxl_devid),
> > > +    ("remove",           bool),
> > > +    ("p_devname",        string),
> > > +    ("pdev_type",        libxl_vscsi_pdev_type),
> > > +    ("pdev",             libxl_vscsi_hctl),
> > > +    ("vdev",             libxl_vscsi_hctl),
> > 
> > Are these last two valid for LIBXL_VSCSI_PDEV_TYPE != HCTL?
> 
> At least for vdev, which is always in HCTL format.
> 
> pdev is kind of orphan in case of LIBXL_VSCSI_PDEV_TYPE_WWN. The code
> records pdev.lun somewhere, just in case. But in the end the LUN is
> already part of the cfg string so nothing needs to make use of pdev
> anymore.

Perhaps pdev should be in the !WWN part of a KeyedUnion then?

> 
> > > +    ])
> > > +
> > > +libxl_device_vscsi = Struct("device_vscsi", [
> > > +    ("backend_domid",    libxl_domid),
> > > +    ("devid",            libxl_devid),
> > > +    ("v_hst",            uint32),
> > > +    ("vscsi_devs",       Array(libxl_vscsi_dev, "num_vscsi_devs")),
> > > +    ("feature_host",     bool),
> > 
> > What is this feature thing? What does !host imply?
> 
> This enables raw SCSI command passthrough in xenlinux. If the flag is
> off then each command is checked. The check allows many commands to
> passthrough, one command needs emulation and unhandled commands are
> rejected. The flag is a noop in pvops because it doesnt look at such
> flag. I was told its not required, dont know the details.

Sounds like it at the very least needs a better name.

But if you don't really know what a thing is for or what the details are
then it would probably be best just to leave it out until someone who
does know comes along with a followup patch.

Ian.