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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

Just looking at the API functions on this pass.

General comment though: Please go through and rewrap any lines longer
than 75-80, I saw quite a few as I was scrolling.

> @@ -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?


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

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



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