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

[Olaf Hering <olaf@xxxxxxxxx>]

On Wed, May 13, Ian Campbell wrote:

> Just looking at the API functions on this pass.

> > @@ -3628,6 +3643,7 @@ static inline void 
> > libxl__update_config_vtpm(libxl__gc *gc,
> >   * devices have same identifier. */
> >  #define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid)
> >  #define COMPARE_DISK(a, b) (!strcmp((a)->vdev, (b)->vdev))
> > +#define COMPARE_VSCSI(a, b) ((a)->v_hst == (b)->v_hst)
> 
> Does this end up doing the correct thing given that vscsi is a bit
> different and the libxl_device_vscsi is actually a bus? Shouldn't
> something somewhere be taking care that vscsi->vscsi_devs is up to date,
> not just the list of the vscsi busses?

In the usecases I have seen comparing just the index number ->v_hst is
ok IMO. The tricky part is to keep xenstore and the json in sync. Given
your comment below, if ->remove will be dropped it will be required to
always keep xenstore and json in sync. I will see what needs to be done.

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

This is also used by the JSON to not readd an already removed device.

But perhaps the code should just use a dummy vhost->vdev and pass that
into libxl. Which in turn searches in the list of active vdevs for the
requested vdev and removes it. Sounds reasonable to do it that way.

> > +    ("pdev",             libxl_vscsi_pdev),
> > +    ("vdev",             libxl_vscsi_hctl),
> > +    ])
> > +
> > +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")),
> > +    ("next_vscsi_dev_id", libxl_devid),
> 
> This one also seems odd.
> 
> If you need to store state internally within libxl then you will have to
> arrange to store it somewhere else, the public API structs are not
> appropriate.

Makes sense. I will rearrange the code which makes use of that member.

Olaf

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