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

[Olaf Hering <olaf@xxxxxxxxx>]

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. I
will use something else then.

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

Ok, will check what looks best, and where to bail out if vscsi= is used
on BSD.

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

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

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel