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

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;
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list