From: Jim Fehlig <jfehlig@suse.com>
To: Olaf Hering <olaf@aepfle.de>
Cc: libvir-list@redhat.com, xen-devel@lists.xen.org
Subject: Re: [libvirt] [PATCH] libxl: support vscsi
Date: Fri, 08 May 2015 12:09:46 -0600 [thread overview]
Message-ID: <554CFBEA.2020201__26378.0797984966$1431108790$gmane$org@suse.com> (raw)
In-Reply-To: <1430203306-28283-1-git-send-email-olaf@aepfle.de>
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@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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>
>
>
prev parent reply other threads:[~2015-05-08 18:09 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-28 6:41 [PATCH] libxl: support vscsi Olaf Hering
2015-05-08 18:09 ` Jim Fehlig [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='554CFBEA.2020201__26378.0797984966$1431108790$gmane$org@suse.com' \
--to=jfehlig@suse.com \
--cc=libvir-list@redhat.com \
--cc=olaf@aepfle.de \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.