Commit v3.7.0-14-gc57f3fd2f8 prevented adding a <boot order='x'/>
element to an inactive domain with global <boot dev='...'/> element.
However, as a result of that change updating any device with boot order
would fail with 'boot order X is already used by another device', where
"another device" is in fact the device which is being updated.

To fix this we have to ignore the device which we're about to update
when checking for boot order conflicts.

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

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/conf/domain_conf.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c71c28e8d2..d96b012b98 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27381,18 +27381,30 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev)
     return false;
 }
 
+
+typedef struct _virDomainCompatibleDeviceData virDomainCompatibleDeviceData;
+typedef virDomainCompatibleDeviceData *virDomainCompatibleDeviceDataPtr;
+struct _virDomainCompatibleDeviceData {
+    virDomainDeviceInfoPtr newInfo;
+    virDomainDeviceInfoPtr oldInfo;
+};
+
 static int
 virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
                                   virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
                                   virDomainDeviceInfoPtr info,
                                   void *opaque)
 {
-    virDomainDeviceInfoPtr newinfo = opaque;
+    virDomainCompatibleDeviceDataPtr data = opaque;
 
-    if (info->bootIndex == newinfo->bootIndex) {
+    /* Ignore the device we're about to update */
+    if (data->oldInfo == info)
+        return 0;
+
+    if (info->bootIndex == data->newInfo->bootIndex) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("boot order %u is already used by another device"),
-                       newinfo->bootIndex);
+                       data->newInfo->bootIndex);
         return -1;
     }
     return 0;
@@ -27401,9 +27413,12 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
 int
 virDomainDefCompatibleDevice(virDomainDefPtr def,
                              virDomainDeviceDefPtr dev,
-                             virDomainDeviceDefPtr oldDev ATTRIBUTE_UNUSED)
+                             virDomainDeviceDefPtr oldDev)
 {
-    virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
+    virDomainCompatibleDeviceData data = {
+        .newInfo = virDomainDeviceGetInfo(dev),
+        .oldInfo = virDomainDeviceGetInfo(oldDev),
+    };
 
     if (!virDomainDefHasUSB(def) &&
         def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
@@ -27414,7 +27429,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
         return -1;
     }
 
-    if (info && info->bootIndex > 0) {
+    if (data.newInfo && data.newInfo->bootIndex > 0) {
         if (def->os.nBootDevs > 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("per-device boot elements cannot be used"
@@ -27423,7 +27438,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
         }
         if (virDomainDeviceInfoIterate(def,
                                        virDomainDeviceInfoCheckBootIndex,
-                                       info) < 0)
+                                       &data) < 0)
             return -1;
     }
 
-- 
2.16.2

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

On 02/22/2018 09:21 AM, Jiri Denemark wrote:
> Commit v3.7.0-14-gc57f3fd2f8 prevented adding a <boot order='x'/>
> element to an inactive domain with global <boot dev='...'/> element.
> However, as a result of that change updating any device with boot order
> would fail with 'boot order X is already used by another device', where
> "another device" is in fact the device which is being updated.
> 
> To fix this we have to ignore the device which we're about to update
> when checking for boot order conflicts.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1546971
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/conf/domain_conf.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 

While chasing something else - I've tripped across this change...

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c71c28e8d2..d96b012b98 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -27381,18 +27381,30 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev)
>      return false;
>  }
>  
> +
> +typedef struct _virDomainCompatibleDeviceData virDomainCompatibleDeviceData;
> +typedef virDomainCompatibleDeviceData *virDomainCompatibleDeviceDataPtr;
> +struct _virDomainCompatibleDeviceData {
> +    virDomainDeviceInfoPtr newInfo;
> +    virDomainDeviceInfoPtr oldInfo;
> +};
> +
>  static int
>  virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
>                                    virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
>                                    virDomainDeviceInfoPtr info,
>                                    void *opaque)
>  {
> -    virDomainDeviceInfoPtr newinfo = opaque;
> +    virDomainCompatibleDeviceDataPtr data = opaque;
>  
> -    if (info->bootIndex == newinfo->bootIndex) {
> +    /* Ignore the device we're about to update */
> +    if (data->oldInfo == info)
> +        return 0;
> +
> +    if (info->bootIndex == data->newInfo->bootIndex) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                         _("boot order %u is already used by another device"),
> -                       newinfo->bootIndex);
> +                       data->newInfo->bootIndex);
>          return -1;
>      }
>      return 0;
> @@ -27401,9 +27413,12 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
>  int
>  virDomainDefCompatibleDevice(virDomainDefPtr def,
>                               virDomainDeviceDefPtr dev,
> -                             virDomainDeviceDefPtr oldDev ATTRIBUTE_UNUSED)
> +                             virDomainDeviceDefPtr oldDev)
>  {
> -    virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
> +    virDomainCompatibleDeviceData data = {
> +        .newInfo = virDomainDeviceGetInfo(dev),
> +        .oldInfo = virDomainDeviceGetInfo(oldDev),
> +    };

I believe this has broken the ability to attach or update a CDROM for
both qemu and lxc as virDomainDefCompatibleDevice is called with a NULL
3rd parameter leading to virDomainDeviceGetInfo(NULL) being called
resulting in a fairly immediate coredump.

If I modify things to have .oldInfo = NULL, and then fill it in only if
@oldDev, that resolves the problem. Whether that's the right fix not
100% sure (I'm still trying to mentally dig out from a week away from work).

John

>  
>      if (!virDomainDefHasUSB(def) &&
>          def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
> @@ -27414,7 +27429,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>          return -1;
>      }
>  
> -    if (info && info->bootIndex > 0) {
> +    if (data.newInfo && data.newInfo->bootIndex > 0) {
>          if (def->os.nBootDevs > 0) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                             _("per-device boot elements cannot be used"
> @@ -27423,7 +27438,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
>          }
>          if (virDomainDeviceInfoIterate(def,
>                                         virDomainDeviceInfoCheckBootIndex,
> -                                       info) < 0)
> +                                       &data) < 0)
>              return -1;
>      }
>  
> 

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