From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v3 4/4] libxl: add support for vscsi Date: Wed, 11 Mar 2015 15:33:26 +0000 Message-ID: <1426088006.21353.282.camel@citrix.com> References: <1425635156-2357-1-git-send-email-olaf@aepfle.de> <1425635156-2357-5-git-send-email-olaf@aepfle.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1425635156-2357-5-git-send-email-olaf@aepfle.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Olaf Hering Cc: Wei Liu , Stefano Stabellini , Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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.