Re: [Xen-devel] [libvirt] [PATCH] libxl: support vscsi

[Jim Fehlig <jfehlig@xxxxxxxx>]

Olaf Hering wrote:
> This uses the API version of the proposed vscsi support in libxl (v4):
> http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg01949.html
>   

We'll need to wait for that to land before committing this work to
libvirt.git, but thanks posting it for early review and feedback!

> Is there anything else that needs to be done in libvirt?

You'll need to add support for domXML <-> xl config conversion in
src/xenconfig/, along with tests for the conversion code. As you're
probably aware, src/xenconfig supports (among other things) 'virsh
domxml-{to,from}-native xen-{xm,xl,sxpr} ...'.

>  Right now libvirt scsi
> support is very simple minded, no support at all to describe host devices with
> persistant names.
>   

I'm not that familiar with the internals of libvirt's scsi support.
Hopefully others can point out anything else you might have missed.

> This patch got very little runtime testing up to now...
>   

Seems very little compile testing, at least with -Werror and
LIBXL_HAVE_VSCSI not defined... :)

> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> ---
>  src/libxl/libxl_conf.c   |  59 +++++++++++++++++
>  src/libxl/libxl_conf.h   |   1 +
>  src/libxl/libxl_domain.c |   2 +-
>  src/libxl/libxl_driver.c | 167 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 228 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index f9bb5ed..7964e8b 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1591,6 +1591,61 @@ libxlMakePCIList(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>  }
>  
>  static int
> +libxlMakeVscsiList(libxl_ctx *ctx,
> +                   XLU_Config *xlu,
> +                   virDomainDefPtr def,
> +                   libxl_domain_config *d_config)
>   

Unused parameters when LIBXL_HAVE_VSCSI is not defined.

> +{
> +    virDomainHostdevDefPtr *l_hostdevs = def->hostdevs;
> +    size_t i, nhostdevs = def->nhostdevs;
> +    virDomainHostdevDefPtr hostdev;
> +    virDomainHostdevSubsysSCSIPtr scsisrc;
> +    char *str;
> +    int rc = 0;
>   

Unused variables when LIBXL_HAVE_VSCSI is not defined. Should this whole
function be wrapped in LIBXL_HAVE_VSCSI? E.g.

#if defined(LIBXL_HAVE_VSCSI)
static int
libxlMakeVscsiList(libxl_ctx *ctx,
XLU_Config *xlu,
virDomainDefPtr def,
libxl_domain_config *d_config)
{
...
}
#else
static int
libxlMakeVscsiList(libxl_ctx *ctx,
XLU_Config *xlu,
virDomainDefPtr def,
libxl_domain_config *d_config)
{
error not supported
}


> +
> +    if (nhostdevs == 0)
> +        return 0;
> +
> +    for (i = 0; i < nhostdevs; i++) {
> +        hostdev = l_hostdevs[i];
> +        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> +            continue;
> +        if (hostdev->source.subsys.type != 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> +            continue;
> +        scsisrc = &hostdev->source.subsys.u.scsi;
> +        if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +            continue;
> +#if defined(LIBXL_HAVE_VSCSI)
> +        if (virAsprintf(&str, "%s:%u:%u:%u,%u:%u:%u:%u%s",
> +                     scsisrc->u.host.adapter + strlen("scsi_host"),
> +                     scsisrc->u.host.bus,
> +                     scsisrc->u.host.target,
> +                     scsisrc->u.host.unit,
> +                     hostdev->info->addr.drive.controller,
> +                     hostdev->info->addr.drive.bus,
> +                     hostdev->info->addr.drive.target,
> +                     hostdev->info->addr.drive.unit,
> +                     scsisrc->rawio == VIR_TRISTATE_BOOL_YES ? 
> ",feature-host" : "") < 0) {
> +            goto error;
> +        };
> +        rc = xlu_vscsi_config_add(xlu, ctx, str, &d_config->num_vscsis, 
> &d_config->vscsis);
>   

You'll need to declare xlu_vscsi_config_add in libxl_conf.h, similar to
xlu_cfg_{destroy,init}.

> +        VIR_FREE(str);
> +        if (rc)
> +            goto error;
> +#else
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("This version of libxenlight does not support 
> vscsi"));
> +        goto error;
> +#endif
> +    }
> +
> +    return 0;
> +
> + error:
> +    return -1;
>   

Nothing is done here. Why not 'return -1' where the errors occur?

> +}
> +
> +static int
>  libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
>  
>  {
> @@ -1724,6 +1779,7 @@ int
>  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
>                         libxl_ctx *ctx,
> +                       XLU_Config *xlu,
>                         libxl_domain_config *d_config)
>   

I still have dreams of unit tests for this function

https://www.redhat.com/archives/libvir-list/2015-January/msg00924.html

But adding the XLU_Config parameter doesn't complicate that. The
invoking test can declare and init an XLU_Config object.

>  {
>      libxl_domain_config_init(d_config);
> @@ -1746,6 +1802,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr 
> graphicsports,
>      if (libxlMakePCIList(def, d_config) < 0)
>          return -1;
>  
> +    if (libxlMakeVscsiList(ctx, xlu, def, d_config) < 0)
> +        return -1;
> +
>      /*
>       * Now that any potential VFBs are defined, update the build info with
>       * the data of the primary display. Some day libxl might implicitely do
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index bdc68d4..94a3046 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -205,6 +205,7 @@ int
>  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
>                         libxl_ctx *ctx,
> +                       XLU_Config *xlu,
>                         libxl_domain_config *d_config);
>  
>  static inline void
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 3039427..537e370 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -950,7 +950,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
> virDomainObjPtr vm,
>      }
>  
>      if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
> -                               cfg->ctx, &d_config) < 0)
> +                               cfg->ctx, cfg->xlu, &d_config) < 0)
>          goto cleanup;
>  
>      if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0)
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index f915f91..424150f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -2943,6 +2943,97 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> driver,
>  }
>  
>  static int
> +libxlDomainAttachHostSCSIDevice(libxlDriverPrivatePtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainHostdevDefPtr hostdev)
> +{
> +#if defined(LIBXL_HAVE_VSCSI)
> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +    libxl_device_vscsi vscsidev;
> +    virDomainHostdevDefPtr found;
> +    virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +    virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> +    char *str = NULL;
> +    int ret = -1;
> +
> +    if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +        return -1;
> +
> +    libxl_device_vscsi_init(&vscsidev);
> +
> +    if (virDomainHostdevFind(vm->def, hostdev, &found) >= 0) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("target scsi device %u:%u:%u:%u already exists"),
> +                       hostdev->info->addr.drive.controller,
> +                       hostdev->info->addr.drive.bus,
> +                       hostdev->info->addr.drive.target,
> +                       hostdev->info->addr.drive.unit);
> +        goto cleanup;
> +    }
> +
> +    if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
> +        goto cleanup;
> +
> +    if (virHostdevPrepareSCSIDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
> +                                     vm->def->name, &hostdev, 1) < 0)
> +        goto cleanup;
> +
> +    if (virAsprintf(&str, "%s:%u:%u:%u,%u:%u:%u:%u%s",
> +                    scsisrc->u.host.adapter + strlen("scsi_host"),
> +                    scsisrc->u.host.bus,
> +                    scsisrc->u.host.target,
> +                    scsisrc->u.host.unit,
> +                    hostdev->info->addr.drive.controller,
> +                    hostdev->info->addr.drive.bus,
> +                    hostdev->info->addr.drive.target,
> +                    hostdev->info->addr.drive.unit,
> +                    scsisrc->rawio == VIR_TRISTATE_BOOL_YES ?
> +                    ",feature-host" : "") < 0) {
> +        goto error;
> +    };
> +
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", str);
> +
> +    if (xlu_vscsi_get_host(cfg->xlu, cfg->ctx, vm->def->id, str, &vscsidev) 
> < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("libxutil failed to parse scsi device %u:%u:%u:%u"),
> +                       hostdev->info->addr.drive.controller,
> +                       hostdev->info->addr.drive.bus,
> +                       hostdev->info->addr.drive.target,
> +                       hostdev->info->addr.drive.unit);
> +    }
> +
> +    if (libxl_device_vscsi_add(cfg->ctx, vm->def->id, &vscsidev, NULL) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("libxenlight failed to attach scsi device 
> %u:%u:%u:%u"),
> +                       hostdev->info->addr.drive.controller,
> +                       hostdev->info->addr.drive.bus,
> +                       hostdev->info->addr.drive.target,
> +                       hostdev->info->addr.drive.unit);
> +        goto error;
> +    }
> +
> +    vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> +    ret = 0;
> +    goto cleanup;
> +
> + error:
> +    virHostdevReAttachSCSIDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
> +                                  vm->def->name, &hostdev, 1);
> +
> + cleanup:
> +    VIR_FREE(str);
> +    virObjectUnref(cfg);
> +    libxl_device_vscsi_dispose(&vscsidev);
> +    return ret;
> +#else
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                   _("This version of libxenlight does not support vscsi"));
> +    return -1;
>   

Parameters are unused when LIBXL_HAVE_VSCSI is not defined.

> +#endif
> +}
> +
> +static int
>  libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
>                              virDomainObjPtr vm,
>                              virDomainHostdevDefPtr hostdev)
> @@ -2960,6 +3051,11 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr 
> driver,
>              return -1;
>          break;
>  
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> +        if (libxlDomainAttachHostSCSIDevice(driver, vm, hostdev) < 0)
> +            return -1;
> +        break;
> +
>      default:
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("hostdev subsys type '%s' not supported"),
> @@ -3284,6 +3380,74 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr 
> driver,
>  }
>  
>  static int
> +libxlDomainDetachHostSCSIDevice(libxlDriverPrivatePtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainHostdevDefPtr hostdev)
> +{
> +#if defined(LIBXL_HAVE_VSCSI)
> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +    virDomainHostdevDefPtr detach;
> +    int idx;
> +    virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +    virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> +    int ret = -1;
> +    char *str = NULL;
> +
> +    if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +        return -1;
> +
> +    idx = virDomainHostdevFind(vm->def, hostdev, &detach);
> +    if (idx < 0) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("target scsi device %u:%u:%u:%u not found"),
> +                       hostdev->info->addr.drive.controller,
> +                       hostdev->info->addr.drive.bus,
> +                       hostdev->info->addr.drive.target,
> +                       hostdev->info->addr.drive.unit);
> +        goto cleanup;
> +    }
> +
> +    if (virAsprintf(&str, "%u:%u:%u:%u",
> +                    hostdev->info->addr.drive.controller,
> +                    hostdev->info->addr.drive.bus,
> +                    hostdev->info->addr.drive.target,
> +                    hostdev->info->addr.drive.unit) < 0) {
> +        goto error;
> +    };
> +
>   

Maybe a comment here that xlu_vscsi_detach calls
libxl_device_vscsi_remove. The other libxlDomain{Attach,Detach}*Device
functions call libxl_device_*_{add,remove}, so I think a comment about
diverging from that pattern would be helpful.

> +    if (xlu_vscsi_detach(cfg->xlu, cfg->ctx, vm->def->id, str) < 0) {
>   
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("libxenlight failed to detach pci device 
> %u:%u:%u:%u"),
> +                       hostdev->info->addr.drive.controller,
> +                       hostdev->info->addr.drive.bus,
> +                       hostdev->info->addr.drive.target,
> +                       hostdev->info->addr.drive.unit);
> +        goto error;
> +    }
> +
> +
> +    virDomainHostdevRemove(vm->def, idx);
> +
> +    virHostdevReAttachPCIDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
> +                                 vm->def->name, &hostdev, 1, NULL);
> +
> +    ret = 0;
> +
> + error:
> +    VIR_FREE(str);
> +    virDomainHostdevDefFree(detach);
> +
> + cleanup:
> +    virObjectUnref(cfg);
> +    return ret;
> +#else
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                   _("This version of libxenlight does not support vscsi"));
> +    return -1;
>   

Parameters are unused when LIBXL_HAVE_VSCSI is not defined.

Regards,
Jim

> +#endif
> +}
> +
> +static int
>  libxlDomainDetachHostDevice(libxlDriverPrivatePtr driver,
>                              virDomainObjPtr vm,
>                              virDomainHostdevDefPtr hostdev)
> @@ -3301,6 +3465,9 @@ libxlDomainDetachHostDevice(libxlDriverPrivatePtr 
> driver,
>          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>              return libxlDomainDetachHostPCIDevice(driver, vm, hostdev);
>  
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> +            return libxlDomainDetachHostSCSIDevice(driver, vm, hostdev);
> +
>          default:
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("unexpected hostdev type %d"), subsys->type);
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
>
>   

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