7:57 a.m.
On 02/17/2014 11:15 AM, Cedric Bosdonnat wrote:
On Mon, 2014-02-17 at 14:32 +0800, Chunyan Liu wrote:
> For extracting hostdev codes from qemu_hostdev.c to common library, change qemu
> specific cfg->relaxedACS handling to be a flag, and pass it to hostdev
> functions.
>
> Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
> ---
> src/qemu/qemu_hostdev.c | 11 +++++++----
> src/qemu/qemu_hostdev.h | 10 ++++++++--
> src/qemu/qemu_hotplug.c | 7 ++++++-
> src/qemu/qemu_process.c | 5 ++++-
> 4 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 01c24f9..0d313c0 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -650,7 +650,8 @@ qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> const unsigned char *uuid,
> virDomainHostdevDefPtr *hostdevs,
> int nhostdevs,
> - virQEMUCapsPtr qemuCaps)
> + virQEMUCapsPtr qemuCaps,
> + unsigned int flags)
> {
> virPCIDeviceListPtr pcidevs = NULL;
> int last_processed_hostdev_vf = -1;
> @@ -682,8 +683,9 @@ qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
> virPCIDevicePtr other;
> + bool strict_acs_check = !!(flags & VIR_STRICT_ACS_CHECK);
Wouldn't that be more readable to have VIR_RELAXED_ACS_CHECK instead? It
wouldn't change the logic.
I agree that it would be better to make STRICT the default, and have a
RELAXED flag - this way if someone forgets to add the flag in some
circumstance, we won't be defaulting to lowering the level of security.
> - if (!virPCIDeviceIsAssignable(dev, !cfg->relaxedACS)) {
> + if (!virPCIDeviceIsAssignable(dev, strict_acs_check)) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> _("PCI device %s is not assignable"),
> virPCIDeviceGetName(dev));
> @@ -1187,14 +1189,15 @@ int
> qemuPrepareHostDevices(virQEMUDriverPtr driver,
> virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps,
> - bool coldBoot)
> + bool coldBoot,
> + unsigned int flags)
> {
> if (!def->nhostdevs)
> return 0;
>
> if (qemuPrepareHostdevPCIDevices(driver, def->name, def->uuid,
> def->hostdevs, def->nhostdevs,
> - qemuCaps) < 0)
> + qemuCaps, flags) < 0)
> return -1;
>
> if (qemuPrepareHostUSBDevices(driver, def, coldBoot) < 0)
> diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
> index ffb3167..ab7fb9f 100644
> --- a/src/qemu/qemu_hostdev.h
> +++ b/src/qemu/qemu_hostdev.h
> @@ -27,6 +27,10 @@
> # include "qemu_conf.h"
> # include "domain_conf.h"
>
> +typedef enum {
> + VIR_STRICT_ACS_CHECK = (1 << 0), /* strict acs check */
> +} qemuHostdevFlag;
> +
> int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver,
> virDomainDefPtr def);
> int qemuUpdateActiveUsbHostdevs(virQEMUDriverPtr driver,
> @@ -40,7 +44,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> const unsigned char *uuid,
> virDomainHostdevDefPtr *hostdevs,
> int nhostdevs,
> - virQEMUCapsPtr qemuCaps);
> + virQEMUCapsPtr qemuCaps,
> + unsigned int flags);
> int qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev,
> bool mandatory,
> virUSBDevicePtr *usb);
> @@ -54,7 +59,8 @@ int qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
> int qemuPrepareHostDevices(virQEMUDriverPtr driver,
> virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps,
> - bool coldBoot);
> + bool coldBoot,
> + unsigned int flags);
> void qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
> const char *name,
> virDomainHostdevDefPtr *hostdevs,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7066be6..c47c5e8 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1154,12 +1154,16 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
> bool teardownlabel = false;
> int backend;
> unsigned long long memKB;
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
Once you've gotten a valid return from virQEMUDriverGetConfig, you've
increased the refcount on the driver object, and you *must* unref it
when you're finished using it...
> + unsigned int flags = 0;
>
> if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) <
0)
> return -1;
...but if you take this return, you've not unrefed (yeah yeah, I know
that if you've had a memalloc failure you're hosed anyway...)
>
> + if (!cfg->relaxedACS)
> + flags |= VIR_STRICT_ACS_CHECK;
> if (qemuPrepareHostdevPCIDevices(driver, vm->def->name,
vm->def->uuid,
> - &hostdev, 1, priv->qemuCaps) < 0)
> + &hostdev, 1, priv->qemuCaps, flags) <
0)
> return -1;
...and similarly here. Both of these returns need to goto cleanup;, and
cleanup needs to be just before the call to virObjectUnref().
>
> /* this could have been changed by qemuPrepareHostdevPCIDevices */
> @@ -1254,6 +1258,7 @@ qemuDomainAttachHostPciDevice(virQEMUDriverPtr driver,
> VIR_FREE(devstr);
> VIR_FREE(configfd_name);
> VIR_FORCE_CLOSE(configfd);
> + virObjectUnref(cfg);
>
> return 0;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 33d2a77..c0f0719 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3596,6 +3596,7 @@ int qemuProcessStart(virConnectPtr conn,
> unsigned int stop_flags;
> virQEMUDriverConfigPtr cfg;
> virCapsPtr caps = NULL;
> + unsigned int hostdev_flags = 0;
>
> VIR_DEBUG("vm=%p name=%s id=%d pid=%llu",
> vm, vm->def->name, vm->def->id,
> @@ -3685,8 +3686,10 @@ int qemuProcessStart(virConnectPtr conn,
>
> /* Must be run before security labelling */
> VIR_DEBUG("Preparing host devices");
> + if (!cfg->relaxedACS)
> + hostdev_flags |= VIR_STRICT_ACS_CHECK;
> if (qemuPrepareHostDevices(driver, vm->def, priv->qemuCaps,
> - !migrateFrom) < 0)
> + !migrateFrom, hostdev_flags) < 0)
> goto cleanup;
>
> VIR_DEBUG("Preparing chr devices");
ACK.
With the unref problem fixed, and the polarity of the flag switched.
--
Cedric
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list