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

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

On Wed, 2015-05-13 at 15:44 +0100, Ian Campbell wrote:
> >  #define COMPARE_PCI(a, b) ((a)->func == (b)->func &&    \
> >                             (a)->bus == (b)->bus &&      \
> >                             (a)->dev == (b)->dev)
> > libxl_vscsi_dev = Struct("vscsi_dev", [
> > +    ("vscsi_dev_id",     libxl_devid),
> > +    ("remove",           bool),
> 
> What is this remove field?
> 
> A libxl_vscsi_dev describes a device, not the actions which can be
> performed on the device. Those are in the names of the functions.

I started reviewing the code and came across the usage of this, and I'm
afraid we need to reconsider how this stuff fits together.

The underlying issue is that vscsi, like pci (and maybe pvusb?), is a
bit different to other devices, in that the xenstore front/back actually
represent a bus and not individual devices, which are represented as
subdevices.

You've represented this as a libxl_vscsi_dev which is actually a bus
containing an array of devices, which leads to an API where to remove a
device you have to provide libxl_device_scsi_remove with a
libxl_device_vscsi where the individual devices are tagged for remove or
not, using a field in the dev descriptor. Really the remove API should
be taking something like what you've called libxl_vscsi_dev.

There are also issues around updating the json state etc which I think
are made more complicated because of this (DEVICE_ADD called in dev_rm,
and a new version of DEVICE_REMOVE etc).

Is it important/useful that the user be able to configure/control the
number (and addresses) of the buses themselves and which devices are on
which, or can we get away with the pvpci model where the libxl user just
gives the individual devices and the library internally takes care of
what buses need to be created?

If that is not an option then we may need to follow the pvusb model
(Chunyan and George CC-d in case I've got it wrong) which is to have
explicit/separate host and device structs in the libxl API and
associated separate commands to add/remove buses vs add/remove devices
on those buses.

I don't think splitting the structs but not the API functions as you hae
here is something we want to go with.

Going the PCI route would almost certainly be less work at both the API
and implementation side.

Ian.