10:47 a.m.
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(a)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(a)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