[PATCH 2/7] qemu: Add bootIndexSpecified and support set bootIndex to '0'

[Peter Krempa]

On Thu, Nov 17, 2022 at 10:05:28 +0800, Jiang Jiacheng wrote:
> Add a bool bootIndexSpecified into _virDomainDeviceInfo, which means whether
> the bootindex could be update or not. BootIndexSpecified will be set to
> true if bootindex is set in XML.
> Support set bootindex to 0, which means cancel the previous bootindex setting,
> and format boot order = '0' into XML when its bootIndexSpecified is true to
> keep its bootindex changeble.
> 
> Signed-off-by: Jiang Jiacheng <jiangjiacheng at huawei.com>
> ---
>  src/conf/device_conf.h | 3 +++
>  src/conf/domain_conf.c | 9 ++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)

You'll need to note in the documentation the special 0 value and how it
behaves when updating a device.

In case where you disagree with my comment below that the bootindex
should not be in the output XML you'll also need to add XML2XML test
cases.

> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index f2907dc596..5259e25c10 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -144,6 +144,9 @@ struct _virDomainDeviceInfo {
>       * not formatted back. This allows HV drivers to update it if <os><boot ..
>       * is present. */
>      unsigned int effectiveBootIndex;
> +    /* bootIndexSpecified is set to true when device's bootIndex is provided in
> +     * the XML. This allows changing bootIndex online of some devices. */
> +    bool bootIndexSpecified;
>      /* Valid for any PCI device. Can be used for NIC to get
>       * stable numbering in Linux */
>      unsigned int acpiIndex;
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3790121cf7..dcd9696a93 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5142,7 +5142,10 @@ virDomainDeviceInfoFormat(virBuffer *buf,
>      g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
>      g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>  
> -    if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
> +    if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && (info->bootIndex || info->bootIndexSpecified)) {
> +        /* format the boot order = 0 in XML when its bootIndexSpecified is true,
> +         * which means the boot order could be changed by virsh update-device.
> +         */

I'm not persuaded that the logic here needs changing. If the boot index
is 0 and the device is not bootable we should avoid the element. This
includes cases where the device was updated in order co clear the boot
index.

>          virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex);
>  
>          if (info->loadparm)
> @@ -5304,12 +5307,12 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
>  {
>      g_autofree char *loadparm = NULL;
>  
> -    if (virXMLPropUInt(node, "order", 10,
> -                       VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO,
> +    if (virXMLPropUInt(node, "order", 10, VIR_XML_PROP_REQUIRED,
>                         &info->bootIndex) < 0)
>          return -1;
>  
>      info->effectiveBootIndex = info->bootIndex;
> +    info->bootIndexSpecified = true;
>  
>      loadparm = virXMLPropString(node, "loadparm");
>      if (loadparm) {
> -- 
> 2.33.0
>