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

[Olaf Hering <olaf@aepfle.de>]

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?

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

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

> > +    ])
> > +
> > +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.

Olaf