From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Fehlig Subject: Re: [libvirt] [PATCH] libxl: support vscsi Date: Fri, 08 May 2015 12:09:46 -0600 Message-ID: <554CFBEA.2020201__26378.0797984966$1431108790$gmane$org@suse.com> References: <1430203306-28283-1-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: <1430203306-28283-1-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: libvir-list@redhat.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 > --- > 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@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > > > > >