[libvirt] [PATCH 0/2 v4] qemu: Don't fail if the SCSI host device is shareable between domains
*** BLURB HERE *** Osier Yang (2): qemu: Don't fail if the SCSI host device is shareable between domains tests: Add tests for scsi utils docs/formatdomain.html.in | 5 ++ src/libvirt_private.syms | 2 +- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 12 ++- src/qemu/qemu_hostdev.c | 86 ++++++++++-------- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 +- src/security/security_selinux.c | 6 +- src/util/virscsi.c | 70 +++++++++++---- src/util/virscsi.h | 16 ++-- tests/Makefile.am | 14 +++ tests/testutilsqemu.c | 3 +- tests/virscsitest.c | 188 +++++++++++++++++++++++++++++++++++++++ 15 files changed, 344 insertions(+), 76 deletions(-) create mode 100644 tests/virscsitest.c -- 1.8.1.4
It doesn't make sense to fail if the SCSI host device is specified as "shareable" explicitly between domains (NB, it works if and only if the device is specified as "shareable" for *all* domains, otherwise it fails). To fix the problem, this patch introduces an array for virSCSIDevice struct, which records all the names of domain which are using the device (note that the recorded domains must specifiy the device as shareable). And the change on the data struct brings on many subsequent changes in the code. Since prior to this patch, the "shareable" tag didn't work as expected, it actually work like "non-shareable". If there was a live domain using the SCSI device with "shareable" specified, then after upgrading (assuming the live domain keep running during upgrading) the older libvirt (without this patch) to newer libvirt (with this patch), it may cause some confusion when user tries to start later domains as "shareable". So this patch also added notes in formatdomain.html to declare the fact. * src/util/virscsi.h: - Remove virSCSIDeviceGetUsedBy - Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel - Add virSCSIDeviceIsAvailable * src/util/virscsi.c: - struct virSCSIDevice: Change "used_by" to be an array; Add "n_used_by" as the array count - virSCSIDeviceGetUsedBy: Removed - virSCSIDeviceFree: frees the "used_by" array - virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential memory corruption - virSCSIDeviceIsAvailable: New - virSCSIDeviceListDel: Change the logic, for device which is already in the list, just remove the corresponding entry in "used_by". And since it's only used in one place, we can safely removing the code to find out the dev in the list first. - Copyright updating * src/libvirt_private.sys: - virSCSIDeviceGetUsedBy: Remove - virSCSIDeviceIsAvailable: New * src/qemu/qemu_hostdev.c: - qemuUpdateActiveScsiHostdevs: Check if the device existing before adding it to the list; - qemuPrepareHostdevSCSIDevices: Error out if the not all domains use the device as "shareable"; Also don't try to add the device to the activeScsiHostdevs list if it already there; And make more sensible error w.r.t the current "shareable" value in driver->activeScsiHostdevs. - qemuDomainReAttachHostScsiDevices: Change the logic according to the changes on helpers. Signed-off-by: Osier Yang <jyang@redhat.com> --- docs/formatdomain.html.in | 5 +++ src/libvirt_private.syms | 2 +- src/qemu/qemu_hostdev.c | 77 ++++++++++++++++++++++++++--------------------- src/util/virscsi.c | 44 +++++++++++++++++++++------ src/util/virscsi.h | 7 +++-- 5 files changed, 88 insertions(+), 47 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ff50214..20b3f5a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2798,6 +2798,11 @@ between domains (assuming the hypervisor and OS support this). Only supported by SCSI host device. <span class="since">Since 1.0.6</span> + <p> + Note: Although <code>shareable</code> was introduced + <span class="since">in 1.0.6</span>, but it did not work as + as expected until <span class="since">1.2.2</span>. + </p> </dd> </dl> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..4dbba17 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1687,7 +1687,7 @@ virSCSIDeviceGetSgName; virSCSIDeviceGetShareable; virSCSIDeviceGetTarget; virSCSIDeviceGetUnit; -virSCSIDeviceGetUsedBy; +virSCSIDeviceIsAvailable; virSCSIDeviceListAdd; virSCSIDeviceListCount; virSCSIDeviceListDel; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 86a463a..2b9d274 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev = NULL; size_t i; int ret = -1; + virSCSIDevicePtr scsi = NULL; + virSCSIDevicePtr tmp = NULL; if (!def->nhostdevs) return 0; virObjectLock(driver->activeScsiHostdevs); for (i = 0; i < def->nhostdevs; i++) { - virSCSIDevicePtr scsi = NULL; hostdev = def->hostdevs[i]; if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || @@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, hostdev->shareable))) goto cleanup; - virSCSIDeviceSetUsedBy(scsi, def->name); - - if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) { + if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { + if (virSCSIDeviceSetUsedBy(tmp, def->name) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } virSCSIDeviceFree(scsi); - goto cleanup; + } else { + if (virSCSIDeviceSetUsedBy(scsi, def->name) < 0 || + virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } } } ret = 0; @@ -1118,24 +1126,29 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, for (i = 0; i < count; i++) { virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i); if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { - const char *other_name = virSCSIDeviceGetUsedBy(tmp); + bool scsi_shareable = virSCSIDeviceGetShareable(scsi); + bool tmp_shareable = virSCSIDeviceGetShareable(tmp); - if (other_name) - virReportError(VIR_ERR_OPERATION_INVALID, - _("SCSI device %s is in use by domain %s"), - virSCSIDeviceGetName(tmp), other_name); - else + if (!(scsi_shareable && tmp_shareable)) { virReportError(VIR_ERR_OPERATION_INVALID, - _("SCSI device %s is already in use"), + _("SCSI device %s is already in use by " + "other domain(s) as '%s'"), + tmp_shareable ? "shareable" : "non-shareable", virSCSIDeviceGetName(tmp)); - goto error; - } + goto error; + } - virSCSIDeviceSetUsedBy(scsi, name); - VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi)); + if (virSCSIDeviceSetUsedBy(tmp, name) < 0) + goto error; + } else { + if (virSCSIDeviceSetUsedBy(scsi, name) < 0) + goto error; - if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) - goto error; + VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi)); + + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) + goto error; + } } virObjectUnlock(driver->activeScsiHostdevs); @@ -1380,8 +1393,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, virObjectLock(driver->activeScsiHostdevs); for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; - virSCSIDevicePtr scsi, tmp; - const char *used_by = NULL; + virSCSIDevicePtr scsi; + virSCSIDevicePtr tmp; virDomainDeviceDef dev; dev.type = VIR_DOMAIN_DEVICE_HOSTDEV; @@ -1411,30 +1424,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, /* Only delete the devices which are marked as being used by @name, * because qemuProcessStart could fail on the half way. */ - tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi); - virSCSIDeviceFree(scsi); - - if (!tmp) { + if (!(tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { VIR_WARN("Unable to find device %s:%d:%d:%d " "in list of active SCSI devices", hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit); + virSCSIDeviceFree(scsi); continue; } - used_by = virSCSIDeviceGetUsedBy(tmp); - if (STREQ_NULLABLE(used_by, name)) { - VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs", - hostdev->source.subsys.u.scsi.adapter, - hostdev->source.subsys.u.scsi.bus, - hostdev->source.subsys.u.scsi.target, - hostdev->source.subsys.u.scsi.unit, - name); + VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + name); - virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp); - } + virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp, name); + virSCSIDeviceFree(scsi); } virObjectUnlock(driver->activeScsiHostdevs); } diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 58d9e25..b150299 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -1,6 +1,7 @@ /* * virscsi.c: helper APIs for managing host SCSI devices * + * Copyright (C) 2013 - 2014 Red Hat, Inc. * Copyright (C) 2013 Fujitsu, Inc. * * This library is free software; you can redistribute it and/or @@ -55,7 +56,8 @@ struct _virSCSIDevice { char *name; /* adapter:bus:target:unit */ char *id; /* model:vendor */ char *sg_path; /* e.g. /dev/sg2 */ - const char *used_by; /* name of the domain using this dev */ + char **used_by; /* name of the domains using this dev */ + size_t n_used_by; /* how many domains are using this dev */ bool readonly; bool shareable; @@ -256,26 +258,37 @@ cleanup: void virSCSIDeviceFree(virSCSIDevicePtr dev) { + size_t i; + if (!dev) return; VIR_FREE(dev->id); VIR_FREE(dev->name); VIR_FREE(dev->sg_path); + for (i = 0; i < dev->n_used_by; i++) { + VIR_FREE(dev->used_by[i]); + } + VIR_FREE(dev->used_by); VIR_FREE(dev); } -void +int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name) { - dev->used_by = name; + char *copy = NULL; + + if (VIR_STRDUP(copy, name) < 0) + return -1; + + return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy); } -const char * -virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev) +bool +virSCSIDeviceIsAvailable(virSCSIDevicePtr dev) { - return dev->used_by; + return dev->n_used_by == 0; } const char * @@ -406,10 +419,23 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list, void virSCSIDeviceListDel(virSCSIDeviceListPtr list, - virSCSIDevicePtr dev) + virSCSIDevicePtr dev, + const char *name) { - virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev); - virSCSIDeviceFree(ret); + virSCSIDevicePtr tmp = NULL; + size_t i; + + for (i = 0; i < dev->n_used_by; i++) { + if (STREQ_NULLABLE(dev->used_by[i], name)) { + if (dev->n_used_by > 1) { + VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); + } else { + tmp = virSCSIDeviceListSteal(list, dev); + virSCSIDeviceFree(tmp); + } + break; + } + } } virSCSIDevicePtr diff --git a/src/util/virscsi.h b/src/util/virscsi.h index b2e98ca..aff7e5a 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -50,8 +50,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter, bool shareable); void virSCSIDeviceFree(virSCSIDevicePtr dev); -void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name); -const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev); +int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name); +bool virSCSIDeviceIsAvailable(virSCSIDevicePtr dev); const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); @@ -83,7 +83,8 @@ size_t virSCSIDeviceListCount(virSCSIDeviceListPtr list); virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list, virSCSIDevicePtr dev); void virSCSIDeviceListDel(virSCSIDeviceListPtr list, - virSCSIDevicePtr dev); + virSCSIDevicePtr dev, + const char *name); virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list, virSCSIDevicePtr dev); -- 1.8.1.4
On 01/29/2014 10:22 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is specified as "shareable" explicitly between domains (NB, it works if and only if the device is specified as "shareable" for *all* domains, otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice struct, which records all the names of domain which are using the device (note that the recorded domains must specifiy the device as
s/specifiy/specify/
shareable). And the change on the data struct brings on many subsequent changes in the code.
Since prior to this patch, the "shareable" tag didn't work as expected, it actually work like "non-shareable". If there was a live domain using the SCSI device with "shareable" specified, then after upgrading (assuming the live domain keep running during upgrading) the older libvirt (without this patch) to newer libvirt (with this patch), it may cause some confusion when user tries to start later domains as "shareable". So this patch also added notes in formatdomain.html to declare the fact.
I'm not sure I follow what you're trying to warn about here - when you upgrade libvirtd, doesn't the state reloading figure out which domains have a device in use, as part of the reload machinery? That is, while the old code mishandled shareable with regards to which domain is using the device, we don't actually store the array of domains using a device in XML, but recompute it on libvirtd restart. Thus, upgrading to a newer libvirtd (whether release 1.2.2 or to a version with this patch backported), then starting yet another guest with a shareable request, should work correctly, right?
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ff50214..20b3f5a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2798,6 +2798,11 @@ between domains (assuming the hypervisor and OS support this). Only supported by SCSI host device. <span class="since">Since 1.0.6</span> + <p> + Note: Although <code>shareable</code> was introduced + <span class="since">in 1.0.6</span>, but it did not work as
s/but //
+++ b/src/util/virscsi.c @@ -1,6 +1,7 @@ /* * virscsi.c: helper APIs for managing host SCSI devices * + * Copyright (C) 2013 - 2014 Red Hat, Inc.
s/ - /-/ At any rate, the changes look reasonable to me; ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 30/01/14 03:31, Eric Blake wrote:
On 01/29/2014 10:22 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is specified as "shareable" explicitly between domains (NB, it works if and only if the device is specified as "shareable" for *all* domains, otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice struct, which records all the names of domain which are using the device (note that the recorded domains must specifiy the device as s/specifiy/specify/
shareable). And the change on the data struct brings on many subsequent changes in the code.
Since prior to this patch, the "shareable" tag didn't work as expected, it actually work like "non-shareable". If there was a live domain using the SCSI device with "shareable" specified, then after upgrading (assuming the live domain keep running during upgrading) the older libvirt (without this patch) to newer libvirt (with this patch), it may cause some confusion when user tries to start later domains as "shareable". So this patch also added notes in formatdomain.html to declare the fact. I'm not sure I follow what you're trying to warn about here - when you upgrade libvirtd, doesn't the state reloading figure out which domains have a device in use, as part of the reload machinery? That is, while the old code mishandled shareable with regards to which domain is using the device, we don't actually store the array of domains using a device in XML, but recompute it on libvirtd restart. Thus, upgrading to a newer libvirtd (whether release 1.2.2 or to a version with this patch backported), then starting yet another guest with a shareable request, should work correctly, right?
I think both John and I were confused and forgot upgrading will triger configuration reloading. :-) Will reword the commit log a bit when pushing. Thanks. Osier
To support to pass the path of the test data to the utils, one more argument is added to virSCSIDeviceGetSgName, virSCSIDeviceGetDevName, and virSCSIDeviceNew, and the related code is changed accordingly. Signed-off-by: Osier Yang <jyang@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 12 ++- src/qemu/qemu_hostdev.c | 9 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 +- src/security/security_selinux.c | 6 +- src/util/virscsi.c | 26 +++--- src/util/virscsi.h | 9 +- tests/Makefile.am | 14 +++ tests/testutilsqemu.c | 3 +- tests/virscsitest.c | 188 +++++++++++++++++++++++++++++++++++++++ 13 files changed, 256 insertions(+), 29 deletions(-) create mode 100644 tests/virscsitest.c diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index de20f2d..a97f184 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -291,7 +291,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: - if ((scsi = virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + if ((scsi = virSCSIDeviceNew(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 96b8825..e1ff287 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5785,7 +5785,8 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, virBuffer buf = VIR_BUFFER_INITIALIZER; char *sg = NULL; - sg = (callbacks->qemuGetSCSIDeviceSgName)(dev->source.subsys.u.scsi.adapter, + sg = (callbacks->qemuGetSCSIDeviceSgName)(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index de7683d..82ca4b3 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -57,7 +57,8 @@ typedef struct _qemuBuildCommandLineCallbacks qemuBuildCommandLineCallbacks; typedef qemuBuildCommandLineCallbacks *qemuBuildCommandLineCallbacksPtr; struct _qemuBuildCommandLineCallbacks { - char * (*qemuGetSCSIDeviceSgName) (const char *adapter, + char * (*qemuGetSCSIDeviceSgName) (const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit); diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ac53f6d..0c246c0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -761,7 +761,8 @@ qemuCheckSharedDevice(virHashTablePtr sharedDevices, } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev; - if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + if (!(hostdev_name = virSCSIDeviceGetDevName(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit))) @@ -949,7 +950,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver, if (!(key = qemuGetSharedDeviceKey(disk->src))) goto cleanup; } else { - if (!(dev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + if (!(dev_name = virSCSIDeviceGetDevName(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit))) @@ -1053,7 +1055,8 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver, if (!(key = qemuGetSharedDeviceKey(disk->src))) goto cleanup; } else { - if (!(dev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + if (!(dev_name = virSCSIDeviceGetDevName(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit))) @@ -1137,7 +1140,8 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) return 0; - if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + if (!(hostdev_name = virSCSIDeviceGetDevName(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit))) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 2b9d274..2b11d6c 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -264,7 +264,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) continue; - if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + if (!(scsi = virSCSIDeviceNew(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit, @@ -1102,7 +1103,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, goto cleanup; } - if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + if (!(scsi = virSCSIDeviceNew(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit, @@ -1406,7 +1408,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) continue; - if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + if (!(scsi = virSCSIDeviceNew(NULL, + hostdev->source.subsys.u.scsi.adapter, hostdev->source.subsys.u.scsi.bus, hostdev->source.subsys.u.scsi.target, hostdev->source.subsys.u.scsi.unit, diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 86a033f..257e3dd 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -829,7 +829,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virSCSIDevicePtr scsi = - virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + virSCSIDeviceNew(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0952df9..09b5e57 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -532,7 +532,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virSCSIDevicePtr scsi = - virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + virSCSIDeviceNew(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit, @@ -650,7 +651,8 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virSCSIDevicePtr scsi = - virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + virSCSIDeviceNew(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7b6d8e3..aa47667 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1350,7 +1350,8 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr def, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virSCSIDevicePtr scsi = - virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + virSCSIDeviceNew(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit, @@ -1542,7 +1543,8 @@ virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: { virSCSIDevicePtr scsi = - virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + virSCSIDeviceNew(NULL, + dev->source.subsys.u.scsi.adapter, dev->source.subsys.u.scsi.bus, dev->source.subsys.u.scsi.target, dev->source.subsys.u.scsi.unit, diff --git a/src/util/virscsi.c b/src/util/virscsi.c index b150299..49e19a1 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -101,7 +101,8 @@ virSCSIDeviceGetAdapterId(const char *adapter, } char * -virSCSIDeviceGetSgName(const char *adapter, +virSCSIDeviceGetSgName(const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit) @@ -111,13 +112,14 @@ virSCSIDeviceGetSgName(const char *adapter, char *path = NULL; char *sg = NULL; unsigned int adapter_id; + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) return NULL; if (virAsprintf(&path, - SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/scsi_generic", - adapter_id, bus, target, unit) < 0) + "%s/%d:%d:%d:%d/scsi_generic", + prefix, adapter_id, bus, target, unit) < 0) return NULL; if (!(dir = opendir(path))) { @@ -144,7 +146,8 @@ cleanup: * on failure. */ char * -virSCSIDeviceGetDevName(const char *adapter, +virSCSIDeviceGetDevName(const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit) @@ -154,13 +157,14 @@ virSCSIDeviceGetDevName(const char *adapter, char *path = NULL; char *name = NULL; unsigned int adapter_id; + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) return NULL; if (virAsprintf(&path, - SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/block", - adapter_id, bus, target, unit) < 0) + "%s/%d:%d:%d:%d/block", + prefix, adapter_id, bus, target, unit) < 0) return NULL; if (!(dir = opendir(path))) { @@ -184,7 +188,8 @@ cleanup: } virSCSIDevicePtr -virSCSIDeviceNew(const char *adapter, +virSCSIDeviceNew(const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit, @@ -197,6 +202,7 @@ virSCSIDeviceNew(const char *adapter, char *model_path = NULL; char *vendor = NULL; char *model = NULL; + const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; if (VIR_ALLOC(dev) < 0) return NULL; @@ -207,7 +213,7 @@ virSCSIDeviceNew(const char *adapter, dev->readonly = readonly; dev->shareable = shareable; - if (!(sg = virSCSIDeviceGetSgName(adapter, bus, target, unit))) + if (!(sg = virSCSIDeviceGetSgName(prefix, adapter, bus, target, unit))) goto cleanup; if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) @@ -226,9 +232,9 @@ virSCSIDeviceNew(const char *adapter, } if (virAsprintf(&vendor_path, - SYSFS_SCSI_DEVICES "/%s/vendor", dev->name) < 0 || + "%s/%s/vendor", prefix, dev->name) < 0 || virAsprintf(&model_path, - SYSFS_SCSI_DEVICES "/%s/model", dev->name) < 0) + "%s/%s/model", prefix, dev->name) < 0) goto cleanup; if (virFileReadAll(vendor_path, 1024, &vendor) < 0) diff --git a/src/util/virscsi.h b/src/util/virscsi.h index aff7e5a..1b685eb 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -33,16 +33,19 @@ typedef virSCSIDevice *virSCSIDevicePtr; typedef struct _virSCSIDeviceList virSCSIDeviceList; typedef virSCSIDeviceList *virSCSIDeviceListPtr; -char *virSCSIDeviceGetSgName(const char *adapter, +char *virSCSIDeviceGetSgName(const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit); -char *virSCSIDeviceGetDevName(const char *adapter, +char *virSCSIDeviceGetDevName(const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit); -virSCSIDevicePtr virSCSIDeviceNew(const char *adapter, +virSCSIDevicePtr virSCSIDeviceNew(const char *sysfs_prefix, + const char *adapter, unsigned int bus, unsigned int target, unsigned int unit, diff --git a/tests/Makefile.am b/tests/Makefile.am index 0930073..cfc0905 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -111,6 +111,7 @@ EXTRA_DIST = \ virsh-uriprecedence \ virfiledata \ virpcitestdata \ + virscsidata \ vmx2xmldata \ xencapsdata \ xmconfigdata \ @@ -240,6 +241,10 @@ if WITH_STORAGE test_programs += storagevolxml2argvtest endif WITH_STORAGE +if WITH_LINUX +test_programs += virscsitest +endif WITH_LINUX + test_programs += storagevolxml2xmltest storagepoolxml2xmltest test_programs += nodedevxml2xmltest @@ -916,6 +921,15 @@ else ! WITH_LINUX EXTRA_DIST += fchosttest.c endif ! WITH_LINUX +if WITH_LINUX +virscsitest_SOURCES = \ + virscsitest.c testutils.h testutils.c +virscsitest_LDADD = $(LDADDS) + +else ! WITH_LINUX +EXTRA_DIST += virscsitest.c +endif ! WITH_LINUX + if WITH_CIL CILOPTFLAGS = CILOPTINCS = diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 44e7e9d..726501c 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -355,7 +355,8 @@ cleanup: static char * -testSCSIDeviceGetSgName(const char *adapter ATTRIBUTE_UNUSED, +testSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, + const char *adapter ATTRIBUTE_UNUSED, unsigned int bus ATTRIBUTE_UNUSED, unsigned int target ATTRIBUTE_UNUSED, unsigned int unit ATTRIBUTE_UNUSED) diff --git a/tests/virscsitest.c b/tests/virscsitest.c new file mode 100644 index 0000000..d256a0e --- /dev/null +++ b/tests/virscsitest.c @@ -0,0 +1,188 @@ +/* + * Copyright (C) 2014 Red Hat, Inc. + * + * This library 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; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library 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. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Osier Yang <jyang@redhat.com> + * + */ + +#include <config.h> + +#include <stdlib.h> + +#include "virscsi.h" +#include "testutils.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define VIR_SCSI_DATA "/virscsidata" + +static const char *abs_top_srcdir; +static char *virscsi_prefix = NULL; + +static int +test1(const void *data ATTRIBUTE_UNUSED) +{ + char *name = NULL; + int ret = -1; + + if (!(name = virSCSIDeviceGetDevName(virscsi_prefix, + "scsi_host1", 0, 0, 0))) + return -1; + + if (STRNEQ(name, "sr0")) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(name); + return ret; +} + +/* + * Two test devices are used, one has address "0:0:0:0", the + * other has address "1:0:0:0", see "virscsidata/" for more + * details. + */ +static int +test2(const void *data ATTRIBUTE_UNUSED) +{ + virSCSIDeviceListPtr list = NULL; + virSCSIDevicePtr dev = NULL; + virSCSIDevicePtr dev1 = NULL; + bool free_dev = true; + bool free_dev1 = true; + virSCSIDevicePtr tmp = NULL; + char *sgname = NULL; + int ret = -1; + + sgname = virSCSIDeviceGetSgName(virscsi_prefix, + "scsi_host1", 0, 0, 0); + + if (!sgname || STRNEQ(sgname, "sg1")) + goto cleanup; + + if (!(dev = virSCSIDeviceNew(virscsi_prefix, "scsi_host1", + 0, 0, 0, false, true))) + goto cleanup; + + if (STRNEQ_NULLABLE(virSCSIDeviceGetName(dev), "1:0:0:0") || + virSCSIDeviceGetAdapter(dev) != 1 || + virSCSIDeviceGetBus(dev) != 0 || + virSCSIDeviceGetTarget(dev) != 0 || + virSCSIDeviceGetUnit(dev) != 0 || + virSCSIDeviceGetReadonly(dev) || + !virSCSIDeviceGetShareable(dev)) + goto cleanup; + + if (!virSCSIDeviceIsAvailable(dev)) + goto cleanup; + + if (virSCSIDeviceSetUsedBy(dev, "fc18") < 0) + goto cleanup; + + if (virSCSIDeviceIsAvailable(dev)) + goto cleanup; + + if (virSCSIDeviceSetUsedBy(dev, "fc20") < 0) + goto cleanup; + + if (virSCSIDeviceIsAvailable(dev)) + goto cleanup; + + if (!(list = virSCSIDeviceListNew())) + goto cleanup; + + if (virSCSIDeviceListAdd(list, dev) < 0) + goto cleanup; + + /* virSCSIDeviceListDispose will take care of freeing + * the device. + */ + free_dev = false; + + if (!virSCSIDeviceListFind(list, dev)) + goto cleanup; + + virSCSIDeviceListDel(list, dev, "fc20"); + + if (!virSCSIDeviceListFind(list, dev)) + goto cleanup; + + if (virSCSIDeviceIsAvailable(dev)) + goto cleanup; + + if (virSCSIDeviceListCount(list) != 1) + goto cleanup; + + if (!(dev1 = virSCSIDeviceNew(virscsi_prefix, "scsi_host0", + 0, 0, 0, true, false))) + goto cleanup; + + if (virSCSIDeviceListAdd(list, dev1) < 0) + goto cleanup; + + /* virSCSIDeviceListDispose will take care of freeing + * the device. + */ + free_dev1 = false; + + if (virSCSIDeviceListCount(list) != 2) + goto cleanup; + + if (!(tmp = virSCSIDeviceListSteal(list, dev1))) + goto cleanup; + virSCSIDeviceFree(tmp); + + if (virSCSIDeviceListCount(list) != 1) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(sgname); + if (free_dev) + virSCSIDeviceFree(dev); + if (free_dev1) + virSCSIDeviceFree(dev1); + virObjectUnref(list); + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + abs_top_srcdir = getenv("abs_top_srcdir"); + if (!abs_top_srcdir) + abs_top_srcdir = abs_srcdir "/.."; + + if (virAsprintf(&virscsi_prefix, "%s" VIR_SCSI_DATA, abs_srcdir) < 0) { + ret = -1; + goto cleanup; + } + + if (virtTestRun("test1", test1, NULL) < 0) + ret = -1; + if (virtTestRun("test2", test2, NULL) < 0) + ret = -1; + +cleanup: + VIR_FREE(virscsi_prefix); + return ret; +} + +VIRT_TEST_MAIN(mymain) -- 1.8.1.4
On 01/29/2014 10:22 AM, Osier Yang wrote:
To support to pass the path of the test data to the utils, one
s/to pass/passing/
more argument is added to virSCSIDeviceGetSgName, virSCSIDeviceGetDevName, and virSCSIDeviceNew, and the related code is changed accordingly.
Signed-off-by: Osier Yang <jyang@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 12 ++- src/qemu/qemu_hostdev.c | 9 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 +- src/security/security_selinux.c | 6 +- src/util/virscsi.c | 26 +++--- src/util/virscsi.h | 9 +- tests/Makefile.am | 14 +++ tests/testutilsqemu.c | 3 +- tests/virscsitest.c | 188 +++++++++++++++++++++++++++++++++++++++
It might be worth splitting this into two patches - one that adds the argument and adjusts existing callers, and the other that adds the new testsuite addition. ACK with that split. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 30/01/14 03:35, Eric Blake wrote:
On 01/29/2014 10:22 AM, Osier Yang wrote:
To support to pass the path of the test data to the utils, one s/to pass/passing/
Changed.
more argument is added to virSCSIDeviceGetSgName, virSCSIDeviceGetDevName, and virSCSIDeviceNew, and the related code is changed accordingly.
Signed-off-by: Osier Yang <jyang@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 3 +- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_conf.c | 12 ++- src/qemu/qemu_hostdev.c | 9 +- src/security/security_apparmor.c | 3 +- src/security/security_dac.c | 6 +- src/security/security_selinux.c | 6 +- src/util/virscsi.c | 26 +++--- src/util/virscsi.h | 9 +- tests/Makefile.am | 14 +++ tests/testutilsqemu.c | 3 +- tests/virscsitest.c | 188 +++++++++++++++++++++++++++++++++++++++ It might be worth splitting this into two patches - one that adds the argument and adjusts existing callers, and the other that adds the new testsuite addition.
Splitted and added "virscsitest" to .gitignore. Pushed. Thanks. Osier
participants (2)
-
Eric Blake -
Osier Yang