On 08/29/2018 11:11 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1621910

When introducing this check back in 4ad54a417a1 my mindset was that if an element is missing in update XML then user is requesting for removal of the corresponding setting. For instance, if <bandwidth/> is not present in update XML any QoS previously set on <interface/> is cleared out. Well this assumption is correct but only to some extent.

Turns out, we have some lazy users who when updating path to ISO image construct very minimalistic disk XML and pass it to device update API. Such XML is lacking a lot of information, and alias is one of them. This triggers error in virDomainDefCompatibleDevice() because we think that user is requesting to remove the alias. Well, they are not.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

You may want to re-read your whole commit message after a 24 hour cool down period before pushing it as is. Realistically all this patch does is not fail the update to a live system if someone doesn't pass the alias in the XML (an alias that libvirt generates for them). If someone passes an alias it has to match. I cannot think of a way the oldInfo->alias could be NULL, but those would be famous last words to use. While yes it's not the sole way to identify the device to be updated it is used as the "id=%s" field which QEMU uses for identification, which was my point in the other response. A simple "ps -ef | grep id=" and "virsh dumpxml $DOM | grep "alias name=" shows those kinds of matches. And remember, the user didn't provide those, libvirt generated them - again to me that *is* the primary point/problem. Reviewed-by: John Ferlan <jferlan@redhat.com> John

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 38cac07913..603a4ad652 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28785,9 +28785,9 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,

if (action == VIR_DOMAIN_DEVICE_ACTION_UPDATE && live && - ((!!data.newInfo != !!data.oldInfo) || - (data.newInfo && data.oldInfo && - STRNEQ_NULLABLE(data.newInfo->alias, data.oldInfo->alias)))) { + (data.newInfo && data.oldInfo && + data.newInfo->alias && data.oldInfo->alias && + STRNEQ(data.newInfo->alias, data.oldInfo->alias))) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", _("changing device alias is not allowed")); return -1;