[libvirt] [PATCH v2 3/3] conf: Forbid device alias change on device-update

Michal Privoznik mprivozn at redhat.com
Tue Jun 26 15:47:07 UTC 2018


On 06/26/2018 05:04 PM, John Ferlan wrote:
> 
> 
> On 06/26/2018 06:21 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1585108
>>
>> When updating a live device users might pass different alias than
>> the one the device has. Currently, this is silently ignored which
>> goes against our behaviour for other parts of the device where we
>> explicitly allow only certain changes and error out loudly on
>> anything else.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/conf/domain_conf.c  | 13 ++++++++++++-
>>  src/conf/domain_conf.h  |  3 ++-
>>  src/lxc/lxc_driver.c    |  9 ++++++---
>>  src/qemu/qemu_domain.c  |  8 --------
>>  src/qemu/qemu_driver.c  | 24 ++++++++++++++++--------
>>  src/qemu/qemu_hotplug.c |  5 -----
>>  6 files changed, 36 insertions(+), 26 deletions(-)
>>
> 
> Reviewed-by: John Ferlan <jferlan at redhat.com>
> 
> John
> 
> NB: I've left a couple of "my review notes" below for your consideration
> or thoughts... They are somewhat tangents to the changes, but may give
> you reason to add a comment or two somewhere if you feel compelled to do so.
> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 93cfca351c..b8b53450fa 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -28206,7 +28206,8 @@ int
>>  virDomainDefCompatibleDevice(virDomainDefPtr def,
>>                               virDomainDeviceDefPtr dev,
>>                               virDomainDeviceDefPtr oldDev,
>> -                             virDomainDeviceAction action ATTRIBUTE_UNUSED)
>> +                             virDomainDeviceAction action,
>> +                             bool live)
>>  {
>>      virDomainCompatibleDeviceData data = {
>>          .newInfo = virDomainDeviceGetInfo(dev),
>> @@ -28216,6 +28217,16 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>>      if (oldDev)
>>          data.oldInfo = virDomainDeviceGetInfo(oldDev);
>>  
>> +    if (action == VIR_DOMAIN_DEVICE_ACTION_UPDATE &&
>> +        live &&
>> +        ((!!data.newInfo != !!data.oldInfo) ||
>> +         (data.newInfo && data.oldInfo &&
>> +          STRNEQ_NULLABLE(data.newInfo->alias, data.oldInfo->alias)))) {
>> +        virReportError(VIR_ERR_OPERATION_DENIED, "%s",
>> +                       _("changing device alias is not allowed"));
>> +        return -1;
>> +    }
>> +
> 
> I'd assume that this isn't affected by whether or not REMOVE was
> supplied since we seem to allow a lot less to match for REMOVE and I
> wouldn't think alias really matters at that point.

Yes. Users are required to pass full device XML for detach. To cite from
virDomainDetachDeviceFlags() documentation:

  The supplied XML description of the device should be as specific
  as its definition in the domain XML. The set of attributes used
  to match the device are internal to the drivers. Using a partial definition,
  or attempting to detach a device that is not present in the domain XML,
  but shares some specific attributes with one that is present,
  may lead to unexpected results.

So at REMOVE it doesn't really matter if alias matches or not. We can
change the code to require matching alias, but I think that is another
issue orthogonal to this one ;-)

> 
> I assume what happens is people use the same XML for attach as they use
> for remove and thus don't provide alias for either.  Update - yeah,
> that's a different scenario.

Yep, because of the reasoning I provided in one of the replies to
previous version. Users provide us with new device XML and any
difference to old XML must be treated as request for change. But we
can't change some things for live XML (e.g. alias), therefore we have to
check for them explicitly and error out.

> 
>>      if (!virDomainDefHasUSB(def) &&
>>          def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
>>          virDomainDeviceIsUSB(dev)) {
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 6d203e1f2e..d750f3382a 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -8613,14 +8613,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
>>          return false;
>>      }
>>  
> 
> Perhaps leaving the breadcrumb (e.g. comment) here or at the top of the
> function indicating alias is checked in virDomainDefCompatibleDevice.
> That then at least "matches" what the function description indicates and
> the reason why alias isn't specifically checked.

Okay, done.

> 
>> -    if (disk->info.alias &&
>> -        STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
>> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>> -                       _("cannot modify field '%s' of the disk"),
>> -                       "alias");
>> -        return false;
>> -    }
>> -
>>      CHECK_EQ(info.bootIndex, "boot order", true);
>>      CHECK_EQ(rawio, "rawio", true);
>>      CHECK_EQ(sgio, "sgio", true);
> 
> [...]
> 
>>  
>>          if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 7a1bbc7c8c..a8991800b3 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -3193,11 +3193,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>>      if (!newdev->info.alias &&
>>          VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0)
>>          goto cleanup;
> 
> I'll casually note that based on what I showed from the last review the
> the comment at the top of virDomainDeviceInfoCopy doesn't seem to ring
> true and thus if virDomainDeviceAddressIsValid does return 0 (for
> multiple reasons) it would cause virDomainDeviceInfoCopy to overwrite
> alias, romfile, and loadparm.
> 
> But that's unrelated to your code.
> 
> This at least takes care of the empty alias case, which is expected.
> Whether you decide to note that virDomainDefCompatibleDevice ensures the
> provided alias matches - is your call.  Since there is only one caller
> to this function, removing this check is fine because of that call
> (hence the hedge whether to note it or not so that some future change
> realizes that alias is checked elsewhere).

Yup, I'll put the comment here too.

Thanks,
Michal




More information about the libvir-list mailing list