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@xxxxxxxxxx>
> ---
> 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
>
