|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 4/5] libxl: add support for vscsi
On 22/03/16 09:23, Olaf Hering wrote:
> Port pvscsi support from xend to libxl:
>
> vscsi=['pdev,vdev{,options}']
> xl scsi-attach
> xl scsi-detach
> xl scsi-list
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> docs/man/xl.cfg.pod.5 | 56 ++
> docs/man/xl.pod.1 | 18 +
> tools/libxl/Makefile | 2 +
> tools/libxl/libxl.c | 9 +
> tools/libxl/libxl.h | 42 ++
> tools/libxl/libxl_create.c | 41 +-
> tools/libxl/libxl_device.c | 2 +
> tools/libxl/libxl_internal.h | 9 +
> tools/libxl/libxl_types.idl | 55 ++
> tools/libxl/libxl_types_internal.idl | 1 +
> tools/libxl/libxl_vscsi.c | 1184
> ++++++++++++++++++++++++++++++++++
> tools/libxl/libxlu_vscsi.c | 684 ++++++++++++++++++++
> tools/libxl/libxlutil.h | 18 +
> tools/libxl/xl.h | 3 +
> tools/libxl/xl_cmdimpl.c | 225 ++++++-
> tools/libxl/xl_cmdtable.c | 15 +
> 16 files changed, 2360 insertions(+), 4 deletions(-)
>
Some minor comments. With those addressed:
Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
And also:
Tested-by: Juergen Gross <jgross@xxxxxxxx>
...
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 4ced9b6..6d12a5c 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -543,6 +543,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
> * The following functions are defined:
> * libxl__add_disks
> * libxl__add_nics
> + * libxl__add_vscsictrls
> * libxl__add_vtpms
> * libxl__add_usbctrls
> * libxl__add_usbs
> @@ -564,6 +565,7 @@ void libxl__multidev_prepared(libxl__egc *egc,
>
> DEFINE_DEVICES_ADD(disk)
> DEFINE_DEVICES_ADD(nic)
> +DEFINE_DEVICES_ADD(vscsictrl)
> DEFINE_DEVICES_ADD(vtpm)
> DEFINE_DEVICES_ADD(usbctrl)
> DEFINE_DEVICES_ADD(usbdev)
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 345a764..3dcab80 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2581,6 +2581,10 @@ _hidden void libxl__device_nic_add(libxl__egc *egc,
> uint32_t domid,
> libxl_device_nic *nic,
> libxl__ao_device *aodev);
>
> +_hidden void libxl__device_vscsictrl_add(libxl__egc *egc, uint32_t domid,
> + libxl_device_vscsictrl *vscsictrl,
> + libxl__ao_device *aodev);
> +
> _hidden void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
> libxl_device_vtpm *vtpm,
> libxl__ao_device *aodev);
> @@ -3346,6 +3350,10 @@ _hidden void libxl__add_nics(libxl__egc *egc,
> libxl__ao *ao, uint32_t domid,
> libxl_domain_config *d_config,
> libxl__multidev *multidev);
>
> +_hidden void libxl__add_vscsictrls(libxl__egc *egc, libxl__ao *ao, uint32_t
> domid,
> + libxl_domain_config *d_config,
> + libxl__multidev *multidev);
> +
> _hidden void libxl__add_vtpms(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> libxl_domain_config *d_config,
> libxl__multidev *multidev);
> @@ -4032,6 +4040,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)->devid == (b)->devid)
Is this really needed? I'd prefer using COMPARE_DEVID() instead.
> #define COMPARE_PCI(a, b) ((a)->func == (b)->func && \
> (a)->bus == (b)->bus && \
> (a)->dev == (b)->dev)
...
> diff --git a/tools/libxl/libxl_vscsi.c b/tools/libxl/libxl_vscsi.c
> new file mode 100644
> index 0000000..fbc7a7e
> --- /dev/null
> +++ b/tools/libxl/libxl_vscsi.c
> @@ -0,0 +1,1184 @@
> +/*
> + * Copyright (C) 2016 SUSE Linux GmbH
> + * Author Olaf Hering <olaf@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"
> +
> +typedef struct vscsidev_rm {
> + libxl_device_vscsictrl *ctrl;
> + char *be_path;
> + int dev_wait;
> + libxl__device dev;
> +} vscsidev_rm_t;
> +
> +typedef void (*vscsictrl_add)(libxl__egc *egc,
> + libxl__ao_device *aodev,
> + libxl_device_vscsictrl *vscsictrl,
> + libxl_domain_config *d_config);
> +
> +#define XLU_WWN_LEN 16
> +
> +static int vscsi_parse_hctl(char *str, libxl_vscsi_hctl *hctl)
> +{
> + unsigned int hst, chn, tgt;
> + unsigned long long lun;
> +
> + if (sscanf(str, "%u:%u:%u:%llu", &hst, &chn, &tgt, &lun) != 4)
> + return ERROR_INVAL;
> +
> + hctl->hst = hst;
> + hctl->chn = chn;
> + hctl->tgt = tgt;
> + hctl->lun = lun;
> + return 0;
> +}
> +
> +static bool vscsi_wwn_valid(const char *p)
> +{
> + bool ret = true;
> + int i = 0;
> +
> + for (i = 0; i < XLU_WWN_LEN; i++, p++) {
> + if (*p >= '0' && *p <= '9')
> + continue;
> + if (*p >= 'a' && *p <= 'f')
> + continue;
> + if (*p >= 'A' && *p <= 'F')
> + continue;
What about using isxdigit() here?
Or even: omit the whole function and use "%16[0-9a-fA-F]" as format of
sscanf().
> + ret = false;
> + break;
> + }
> + return ret;
> +}
> +
> +/* Translate p-dev back into pdev.type */
> +static bool vscsi_parse_pdev(libxl__gc *gc, libxl_device_vscsidev *dev,
> + char *c, char *p, char *v)
> +{
> + libxl_vscsi_hctl hctl;
> + unsigned long long lun;
> + char wwn[XLU_WWN_LEN + 1];
> + bool parsed_ok = false;
> +
> + libxl_vscsi_hctl_init(&hctl);
> +
> + dev->pdev.p_devname = libxl__strdup(NOGC, c);
> +
> + if (strncmp(p, "naa.", 4) == 0) {
> + /* WWN as understood by pvops */
> + memset(wwn, 0, sizeof(wwn));
> + if (sscanf(p, "naa.%16c:%llu", wwn, &lun) == 2 &&
> vscsi_wwn_valid(wwn)) {
> + libxl_vscsi_pdev_init_type(&dev->pdev,
> LIBXL_VSCSI_PDEV_TYPE_WWN);
> + dev->pdev.u.wwn.m = libxl__strdup(NOGC, p);
> + parsed_ok = true;
> + }
> + } else if (vscsi_parse_hctl(p, &hctl) == 0) {
> + /* Either xenlinux, or pvops with properly configured alias in sysfs
> */
> + libxl_vscsi_pdev_init_type(&dev->pdev, LIBXL_VSCSI_PDEV_TYPE_HCTL);
> + libxl_vscsi_hctl_copy(CTX, &dev->pdev.u.hctl.m, &hctl);
> + parsed_ok = true;
> + }
> +
> + if (parsed_ok && vscsi_parse_hctl(v, &dev->vdev) != 0)
> + parsed_ok = false;
> +
> + libxl_vscsi_hctl_dispose(&hctl);
> +
> + return parsed_ok;
> +}
...
> diff --git a/tools/libxl/libxlu_vscsi.c b/tools/libxl/libxlu_vscsi.c
> new file mode 100644
> index 0000000..86c78b4
> --- /dev/null
> +++ b/tools/libxl/libxlu_vscsi.c
> @@ -0,0 +1,684 @@
> +/*
> + * libxlu_vscsi.c - xl configuration file parsing: setup and helper functions
> + *
> + * Copyright (C) 2016 SUSE Linux GmbH
> + * Author Olaf Hering <olaf@xxxxxxxxx>
> + * Author Ondřej Holeček <aaannz@xxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include <unistd.h>
> +#include <ctype.h>
> +#include <dirent.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include "libxlu_internal.h"
...
> +static bool xlu__vscsi_wwn_valid(const char *p)
> +{
> + bool ret = true;
> + int i = 0;
> +
> + for (i = 0; i < XLU_WWN_LEN; i++, p++) {
> + if (*p >= '0' && *p <= '9')
> + continue;
> + if (*p >= 'a' && *p <= 'f')
> + continue;
> + if (*p >= 'A' && *p <= 'F')
> + continue;
Same as above: isxdigit() or completely omit the function.
> + ret = false;
> + break;
> + }
> + return ret;
> +}
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |