6:27 a.m.
On 07/05/13 18:57, John Ferlan wrote:
On 05/03/2013 02:07 PM, Osier Yang wrote:
> From: Han Cheng <hanc.fnst(a)cn.fujitsu.com>
>
> Although virtio-scsi supports SCSI PR, the device on host may do not
> support it. To avoid losing data, we only allow one scsi hostdev to
> be passthroughed to one live guest, Just like what we do for PCI
> and USB devices.
What is "PR"? (Persistent Reservations?)
Yes, I should make it more clear.
s/To avoid losing data....USB devices/
"Just like PCI and USB pass through devices, only one live guest is
allowed per SCSI host pass through device."
Okay.
> Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com>
> ---
> src/qemu/qemu_conf.h | 2 +
> src/qemu/qemu_driver.c | 3 +
> src/qemu/qemu_hostdev.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_hostdev.h | 10 +++
> src/qemu/qemu_process.c | 3 +
> 5 files changed, 231 insertions(+)
>
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 77d3d2f..30a2a22 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -36,6 +36,7 @@
> # include "security/security_manager.h"
> # include "virpci.h"
> # include "virusb.h"
> +# include "virscsi.h"
> # include "cpu_conf.h"
> # include "driver.h"
> # include "virportallocator.h"
> @@ -203,6 +204,7 @@ struct _virQEMUDriver {
> virPCIDeviceListPtr activePciHostdevs;
> virPCIDeviceListPtr inactivePciHostdevs;
> virUSBDeviceListPtr activeUsbHostdevs;
> + virSCSIDeviceListPtr activeScsiHostdevs;
>
> /* Immutable pointer. Unsafe APIs. XXX */
> virHashTablePtr sharedDisks;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d5d7de3..b631a1f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -675,6 +675,9 @@ qemuStateInitialize(bool privileged,
> if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL)
> goto error;
>
> + if (!(qemu_driver->activeScsiHostdevs = virSCSIDeviceListNew()))
> + goto error;
> +
A nit, but keeping things with the same look and feel in code is
sometimes easier rather than something that looks different, but
accomplishes the same thing. So rather than !(), use the existing () ==
NULL)...
Personally I like "!", but I don't have a good reason to keep it,
and keep consistence sounds good.
Furthermore a bunch of those if's could probably be put together,
but
that's outside of this set of changes... eg, if (cond1 || cond2 ||
cond3 || etc) goto error;
Another personal habit I think, though I prefer to put them together
too.
> if (!(qemu_driver->sharedDisks = virHashCreate(30,
qemuSharedDiskEntryFree)))
> goto error;
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index e4af036..d5f94d5 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -29,6 +29,7 @@
> #include "viralloc.h"
> #include "virpci.h"
> #include "virusb.h"
> +#include "virscsi.h"
> #include "virnetdev.h"
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
> @@ -226,6 +227,47 @@ cleanup:
> return ret;
> }
>
> +int
> +qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
> + virDomainDefPtr def)
> +{
> + virDomainHostdevDefPtr hostdev = NULL;
> + int i;
> + int ret = -1;
> +
> + 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 ||
> + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> + continue;
> +
> + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + hostdev->readonly)))
> + goto cleanup;
I see that the qemuUpdateActiveUsbHostdevs() had a VIR_WARN before the
goto cleanup - something you may want to consider here as well.
The VIR_WARN is useless, as virSCSIDeviceNew has errors.
> +
> + virSCSIDeviceSetUsedBy(scsi, def->name);
> +
> + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
> + virSCSIDeviceFree(scsi);
> + goto cleanup;
> + }
> + }
> + ret = 0;
> +
> +cleanup:
> + virObjectUnlock(driver->activeScsiHostdevs);
> + return ret;
> +}
> +
> static int
> qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path)
> {
> @@ -827,6 +869,107 @@ cleanup:
> return ret;
> }
>
> +int
> +qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
> + const char *name,
> + virDomainHostdevDefPtr *hostdevs,
> + int nhostdevs)
> +{
> + int i, j, count;
> + virSCSIDeviceListPtr list;
> + virSCSIDevicePtr tmp;
> +
> + /* To prevent situation where SCSI device is assigned to two domains
> + * we need to keep a list of currently assigned SCSI devices.
> + * This is done in several loops which cannot be joined into one big
> + * loop. See qemuPrepareHostdevPCIDevices()
> + */
> + if (!(list = virSCSIDeviceListNew()))
> + goto cleanup;
> +
> + /* Loop 1: build temporary list */
And I see from PrepareHostDevices that nhostdevs is guaranteed to be non
zero.... good...
> + for (i = 0 ; i < nhostdevs ; i++) {
> + virDomainHostdevDefPtr hostdev = hostdevs[i];
> + virSCSIDevicePtr scsi;
> +
> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> + continue;
> +
> + if (hostdev->managed) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("SCSI host device doesn't support managed
mode"));
> + goto cleanup;
> + }
> +
> + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + hostdev->readonly)))
> + goto cleanup;
> +
> + if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) {
> + virSCSIDeviceFree(scsi);
> + goto cleanup;
> + }
> + }
> +
> + /* Loop 2: Mark devices in temporary list as used by @name
> + * and add them to driver list. However, if something goes
> + * wrong, perform rollback.
> + */
> + virObjectLock(driver->activeScsiHostdevs);
> + count = virSCSIDeviceListCount(list);
> +
> + for (i = 0; i < count; i++) {
> + virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i);
> + if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
> + const char *other_name = virSCSIDeviceGetUsedBy(tmp);
> +
> + if (other_name)
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("SCSI device %s is in use by domain
%s"),
> + virSCSIDeviceGetName(tmp), other_name);
> + else
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("SCSI device %s is already in use"),
> + virSCSIDeviceGetName(tmp));
> + goto error;
> + }
> +
> + virSCSIDeviceSetUsedBy(scsi, name);
> + VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
> +
> + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> + goto error;
> + }
> +
> + virObjectUnlock(driver->activeScsiHostdevs);
> +
> + /* Loop 3: Temporary list was successfully merged with
> + * driver list, so steal all items to avoid freeing them
> + * when freeing temporary list.
> + */
> + while (virSCSIDeviceListCount(list) > 0) {
> + tmp = virSCSIDeviceListGet(list, 0);
> + virSCSIDeviceListSteal(list, tmp);
> + }
> +
> + virObjectUnref(list);
> + return 0;
> +
> +error:
> + for (j = 0; j < i; j++) {
> + tmp = virSCSIDeviceListGet(list, i);
> + virSCSIDeviceListSteal(driver->activeScsiHostdevs, tmp);
> + }
So what happens to the devices that were taken from 'list', had a 'name'
associated with them on the active list? Don't they need to have the
name now disassociated? And be removed from there?
It's not *taken* from list, I.E. list still keep the pointers, so they
are just
stealed (let's say detached) from activeList, and virObjectUnref will
takes care of freeing them.
That's why I don't like the vir{pci,usb}.[ch], it causes a lot of mess.
> + virObjectUnlock(driver->activeScsiHostdevs);
> +cleanup:
If we get here from loop1, since the 'list' is potentially full of
'scsi' devices via virSCSIDeviceListAdd(), should there be a loop to
remove those or does this Unref do that for you? Or is that what loop3
is supposed to do?
virObjectUnref will do it, virSCSIDeviceListDispose is involked.
Osier