On 2022/11/22 23:02, Peter Krempa wrote:
> 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.
>
Well, I think I should organize and describe my patches better. My
patches only support update and cancel the bootindex which has been set
in the domain'xml, and don't support add bootindex to a device whose
bootindex hasn't been set.
The new parameter 'bootIndexSpecified' in _virDomainDeviceInfo is used
to distinguish whether the bootindex is specified or not. It is set to
true only when device's bootindex is specified in domain'xml, which
means the device's bootindex could be changed after.
The special 0 is used to cancel the bootindex setting. We get bootindex
= 0 if the bootindex isn't set or specified to 0 in the device's xml.
Update with bootindex = 0 should not effect 'bootIndexSpecified', since
the device's bootindex can still be changed after canceling.
However, we only get the 'bootIndexSpecified' when parsing the domian's
xml, to keep it true with devices whose bootindex is canceld, I format
'boot order = 0' into xml. I do need a XML2XML case here.
If all devices' bootindex is set to 0, qemu will boot in an default
sequence 'disk->cdrom->net' in those devices.
>> 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.
>
As I mentioned before, the 'boot order = 0' will be format into xml only
when bootIndexSpecified is true. BootIndexSpecified is set to true only
if the device is bootable and it's bootindex is specified when starting
the domain, and it could not be changed after the domain started.
>> 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
>>
>
