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

[Michal Privoznik]

On 06/15/2018 02:38 PM, John Ferlan wrote:
> 
> 
> On 06/12/2018 11:04 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 | 12 +++++++++++-
>>  src/conf/domain_conf.h |  3 ++-
>>  src/lxc/lxc_driver.c   |  6 +++---
>>  src/qemu/qemu_driver.c | 16 ++++++++--------
>>  4 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 85f07af46e..91cac75c0a 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -28195,7 +28195,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
>>  int
>>  virDomainDefCompatibleDevice(virDomainDefPtr def,
>>                               virDomainDeviceDefPtr dev,
>> -                             virDomainDeviceDefPtr oldDev)
>> +                             virDomainDeviceDefPtr oldDev,
>> +                             bool live)
>>  {
>>      virDomainCompatibleDeviceData data = {
>>          .newInfo = virDomainDeviceGetInfo(dev),
>> @@ -28205,6 +28206,15 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>>      if (oldDev)
>>          data.oldInfo = virDomainDeviceGetInfo(oldDev);
>>  
>> +    if (live &&
>> +        ((!!data.newInfo != !!data.oldInfo) ||
>> +         (data.newInfo && data.oldInfo &&
>> +          STRNEQ(data.newInfo->alias, data.oldInfo->alias)))) {
>> +        virReportError(VIR_ERR_OPERATION_DENIED, "%s",
>> +                       _("changing device alias is not allowed"));
>> +        return -1;
>> +    }
>> +
> 
> Does this work for attach-device from the bz?  I just tried it and it
> doesn't - probably because oldInfo would be NULL for attach.

Huh, okay. I guess I need to rename the argument to say "checkAlias" and
pass false from everywhere but qemuDomainUpdateDeviceLive(). Would you
agree with this solution?

> 
> Additionally, if the update/detach doesn't include an alias, then would
> this fail too?

That's the idea. In general, one can argue that when detaching a device,
only partial device XML could be parsed (the minimum needed to uniquely
identify the device). But with update device the semantic is changed.
Not only we use the XML to find the device we also use it to perform
changes: what's new is set, what's left out is unset. For instance,
imagine an <interface/> with QoS set. Then, call to update device with
the very same <interface/> with QoS left out must be treated as "user
wants QoS to be removed". Or vice versa. Because of this reason, leaving
out <alias/> is viewed as "unset alias" (which of course is not possible
for live XMLs, but is possible for inactive XMLs). Similarly, providing
different <alias/> is viewed as "change alias".

> 
> By comparison qemuDomainDiskChangeSupported only cares that the updated
> disk definition has an alias before making check and of course that
> would now be duplicitous to this.
> 
> Then there's qemuDomainChangeNet which at first I didn't understand why
> it wasn't throwing up since it does check the alias. Then I noticed
> something from the case, for the updated interface XML - if you include
> the <address> from the live guest, but change the "<alias>", then you
> get the error. However, if you remove the <address> from the update XML
> things then succeed. That's because of this gem in the code:
> 
>     if (!virDomainDeviceAddressIsValid(&newdev->info,
> 
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
>         virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) {
>         goto cleanup;
>     }
> 
> where virDomainDeviceInfoCopy essentially *overwrites* the
> provided/changed newdev->info.alias.
> 
> So I think if you fix this part of the code, then you don't have the
> need for the rest of this patch. Yes, that does possibly leave the
> graphics devices as not checking alias updates, but that would seemingly
> be fixable separately if it's considered a problem or not checked as I
> didn't follow all the graphics update code.

Yep, this definitely deserves fixing (actually, the lines should be
removed, not providing <address/> should be viewed as request for its
removal which is of course impossible). But I still think we need one
place where we would check for the alias change.

Michal