[libvirt] [PATCH] libxl: support vscsi
Jim Fehlig
jfehlig at suse.com
Fri May 8 18:09:46 UTC 2015
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 at aepfle.de>
> ---
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
>
>
More information about the libvir-list
mailing list