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

[John Ferlan]

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.

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.

>      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.

> -    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).

John

> -    if (STRNEQ_NULLABLE(olddev->info.alias, newdev->info.alias)) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                       _("cannot modify network device alias"));
> -        goto cleanup;
> -    }
>      if (olddev->info.rombar != newdev->info.rombar) {
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>                         _("cannot modify network device rom bar setting"));
>