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

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

On Fri, 2015-03-13 at 14:44 +0100, Olaf Hering wrote:
> On Thu, Mar 12, Ian Campbell wrote:
> 
> > 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.
> > > 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.
> 
> NOPARAVIRT looked like a perfect fit, there are no PV drivers for BSD.

Which has no impact on the ability to parse a pdev or not.

(Also this error code is used when it is not possible to speak the pv
control protocol, but you weren't to know that)

> I will use something else then.

Thanks.

> > > > > +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?
> 
> Its not clear to me how a Union would look like. I will study the other
> usages. From a quick look the layout may look like:
> 
> DEV { pdev, vdev }
> WWN { string, vdev }
> HCTL { pdev, vdev }

I don't think vdev should be in the union.

I don't know what the keys are so I can't comment on what should be in
them.

> 
> > > > > +    ])
> > > > > +
> > > > > +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.
> 
> Ok, will rename it to feature_noemul or feature_xenlinux_raw_scsicmd or
> whatever.

Neither of those appear to be a good description of what you say quoted
above.

It sounds somewhat analogous to the pciback permissive mode, but I don't
know that for sure.

Also, should this be a defbool or not?

Ian.