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@xxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list