[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v12 02/11] qemu_hostdev: move cfg->relaxedACS as a flag



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]