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

[Juergen Gross <jgross@suse.com>]

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@aepfle.de>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  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@suse.com>

And also:

Tested-by: Juergen Gross <jgross@suse.com>

...

> 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@aepfle.de>
> + *
> + * 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@aepfle.de>
> + * Author Ondřej Holeček <aaannz@gmail.com>
> + *
> + * 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@lists.xen.org
http://lists.xen.org/xen-devel