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

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

On Fri, 2015-03-06 at 10:45 +0100, Olaf Hering wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bbc52d..1ad52e3 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h

Needs a LIBXL_HAVE define too.

> @@ -1224,6 +1224,35 @@ int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
>                                   libxl_device_channel *channel,
>                                   libxl_channelinfo *channelinfo);
>  
> +/* Virtual SCSI */
> +int libxl_device_vscsi_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vscsi *vscsi,
> +                           const libxl_asyncop_how *ao_how)
> +                           LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vscsi_remove(libxl_ctx *ctx, uint32_t domid,
> +                              libxl_device_vscsi *vscsi,
> +                              const libxl_asyncop_how *ao_how)
> +                              LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_vscsi_destroy(libxl_ctx *ctx, uint32_t domid,
> +                               libxl_device_vscsi *vscsi,
> +                               const libxl_asyncop_how *ao_how)
> +                               LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_vscsi *libxl_device_vscsi_list(libxl_ctx *ctx, uint32_t domid, int *num);
> +int libxl_device_vscsi_getinfo(libxl_ctx *ctx,
> +                               uint32_t domid,
> +                               libxl_device_vscsi *vscsi_host,
> +                               libxl_vscsi_dev *vscsi_dev,
> +                               libxl_vscsiinfo *vscsiinfo);

> +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst,
> +                                   libxl_vscsi_dev *dev);
> +int libxl_device_vscsi_get_host(libxl_ctx *ctx,
> +                                uint32_t domid,
> +                                const char *cfg,
> +                                libxl_device_vscsi **vscsi_host);

What do these two non-standard functions do?

In general the caller would be expected to provide a libxl_device_vscsi,
which will be filled in, rather than having the function allocate one.

> +int libxl_device_vscsi_parse(libxl_ctx *ctx, const char *cfg,
> +                             libxl_device_vscsi *vscsi_host,
> +                             libxl_vscsi_dev *vscsi_dev);

Like with disk, this is xend/xl specific but might be of use to other
toolstacks, therefore it belongs in libxlu not in libxl proper.

(this might apply to get_host too, depending on what it does)

> +
>  /* Virtual TPMs */
>  int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
>                            const libxl_asyncop_how *ao_how)

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

> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
> index 898e160..b8972f0 100644
> --- a/tools/libxl/libxl_netbsd.c
> +++ b/tools/libxl/libxl_netbsd.c
> @@ -95,3 +95,11 @@ libxl_device_model_version libxl__default_device_model(libxl__gc *gc)
>  {
>      return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
>  }
> +
> +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;

As before.

> +}
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 47af340..e56f231 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -542,6 +542,37 @@ libxl_device_channel = Struct("device_channel", [
>             ])),
>  ])
>  
> +libxl_vscsi_pdev_type = Enumeration("vscsi_pdev_type", [
> +    (0, "INVALID"),
> +    (1, "DEV"),
> +    (2, "WWN"),
> +    (3, "HCTL"),
> +    ], init_val = "LIBXL_VSCSI_PDEV_TYPE_INVALID")

You don't need init_val if it happens to be zero.

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

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

> diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
> new file mode 100644
> index 0000000..ab4cb5f
> --- /dev/null
> +++ b/tools/libxl/libxl_vscsi.c

Needs a copyright and a license.