On Tue, Oct 11, 2022 at 21:38:31 +0800, Jiang Jiacheng wrote:
> Enable bootindex can be set to -1, it means cancel the device's bootindex.
> Change bootindex's type from unsigned int to int and modify other related
> codes concered with type.
>
> Signed-off-by: Jiang Jiacheng <jiangjiacheng@xxxxxxxxxx>
> ---
> src/conf/device_conf.h | 4 ++--
> src/conf/domain_conf.c | 15 ++++++++++-----
> src/conf/domain_postparse.c | 2 +-
> src/qemu/qemu_command.c | 38 ++++++++++++++++++-------------------
> src/qemu/qemu_process.c | 8 ++++----
> src/qemu/qemu_validate.c | 6 +++---
> 6 files changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index f2907dc596..a9df2e6e5d 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -139,11 +139,11 @@ struct _virDomainDeviceInfo {
> char *romfile;
> /* bootIndex is only used for disk, network interface, hostdev
> * and redirdev devices */
> - unsigned int bootIndex;
> + int bootIndex;
Here you should describe also what the negative value means.
> /* 'effectiveBootIndex' is same as 'bootIndex' (if provided in the XML) but
> * not formatted back. This allows HV drivers to update it if <os><boot ..
> * is present. */
> - unsigned int effectiveBootIndex;
> + int effectiveBootIndex;
IMO it makes no sense to change the effective boot index value and type
as those will be only positive.
> /* 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 c904d9c63d..fe7e5f9116 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5093,7 +5093,7 @@ virDomainDeviceInfoFormat(virBuffer *buf,
> g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
>
> if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
> - virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex);
> + virBufferAsprintf(buf, "<boot order='%d'", info->bootIndex);
In subsequent patch 7/7 you have a special copy function and state:
Since we don't format bootindex's xml when it is -1, we will lost bootindex
So either that is wrong or you do in fact format it here.
In general I don't think that allowing '-1' for any other case than the
changing of bootindex should be allowed. In fact I find it questionable
to even allow -1 for it as it can cause problems in corner cases.
In fact I'd probably prefer if -1 is not considered at all.
Internally we use 0 as no boot index. You can add another bool field
'bootIndexSpecified' and set it to true if user provided the field in
XML (virXMLProp*) has different return value if the element was
specified.
The new field will be used in the specific case when you want to know
whether you have to clear the boot index.
That way we won't try to overload (and potentially break) the meaning of
the actual boot index value.
[...]
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 32f03ff79a..327307de9c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6264,10 +6264,10 @@ static void
> qemuProcessPrepareDeviceBootorder(virDomainDef *def)
> {
> size_t i;
> - unsigned int bootCD = 0;
> - unsigned int bootFloppy = 0;
> - unsigned int bootDisk = 0;
> - unsigned int bootNetwork = 0;
> + int bootCD = 0;
> + int bootFloppy = 0;
> + int bootDisk = 0;
> + int bootNetwork = 0;
This change isn't needed. We'll only ever assign positive values here.
>
> if (def->os.nBootDevs == 0)
> return;
