[libvirt] [PATCH 3/3] qemu: Fix updating device with boot order

[John Ferlan]

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