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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> 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")); > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
