[libvirt] [PATCH 0/2] qemu: Use bootindex whenever possible
Jiri Denemark (2): qemu: Use bootindex whenever possible qemu: Drop emitBootindex parameter src/qemu/qemu_command.c | 89 ++++++++++------------ .../qemuxml2argv-boot-menu-enable-bootindex.args | 23 ++++++ .../qemuxml2argv-boot-menu-enable-bootindex.xml | 28 +++++++ tests/qemuxml2argvtest.c | 2 +- 4 files changed, 91 insertions(+), 51 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.xml -- 2.9.0
I'm not sure why our code claimed "-boot menu=on" cannot be used in combination with per-device bootindex, but it was proved wrong about four years ago by commit 8c952908. Let's always use bootindex when QEMU supports it. https://bugzilla.redhat.com/show_bug.cgi?id=1323085 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_command.c | 18 ++++++-------- .../qemuxml2argv-boot-menu-enable-bootindex.args | 23 ++++++++++++++++++ .../qemuxml2argv-boot-menu-enable-bootindex.xml | 28 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 +- 4 files changed, 59 insertions(+), 12 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 71e9e63..ecfc447 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6028,22 +6028,18 @@ qemuBuildBootCommandLine(virCommandPtr cmd, /* * We prefer using explicit bootindex=N parameters for predictable * results even though domain XML doesn't use per device boot elements. - * However, we can't use bootindex if boot menu was requested. */ - if (!def->os.nBootDevs) { - /* def->os.nBootDevs is guaranteed to be > 0 unless per-device boot - * configuration is used - */ - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { + *emitBootindex = true; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { + if (def->os.nBootDevs == 0) { + /* def->os.nBootDevs is guaranteed to be > 0 unless per-device boot + * configuration is used + */ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("hypervisor lacks deviceboot feature")); goto error; } - *emitBootindex = true; - } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX) && - (def->os.bootmenu != VIR_TRISTATE_BOOL_YES || - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU))) { - *emitBootindex = true; + *emitBootindex = false; } if (!*emitBootindex) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.args b/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.args new file mode 100644 index 0000000..6580b82 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.args @@ -0,0 +1,23 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot menu=on \ +-usb \ +-drive file=/dev/cdrom,format=raw,if=none,media=cdrom,id=drive-ide0-1-0,\ +readonly=on \ +-device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,bootindex=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.xml b/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.xml new file mode 100644 index 0000000..831933e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='cdrom'/> + <bootmenu enable='yes'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='cdrom'> + <source dev='/dev/cdrom'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a4b8bf4..a73db5e 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -622,7 +622,7 @@ mymain(void) DO_TEST("boot-multi", QEMU_CAPS_BOOT_MENU); DO_TEST("boot-menu-enable", QEMU_CAPS_BOOT_MENU); - DO_TEST("boot-menu-enable", + DO_TEST("boot-menu-enable-bootindex", QEMU_CAPS_BOOT_MENU, QEMU_CAPS_BOOTINDEX); DO_TEST("boot-menu-enable-with-timeout", -- 2.9.0
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_command.c | 75 ++++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ecfc447..3898ed7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1905,8 +1905,7 @@ qemuBuildDriveDevStr(const virDomainDef *def, static int qemuBuildDiskDriveCommandLine(virCommandPtr cmd, const virDomainDef *def, - virQEMUCapsPtr qemuCaps, - bool emitBootindex) + virQEMUCapsPtr qemuCaps) { size_t i; unsigned int bootCD = 0; @@ -1915,7 +1914,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virBuffer fdc_opts = VIR_BUFFER_INITIALIZER; char *fdc_opts_str = NULL; - if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || + virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { /* bootDevs will get translated into either bootindex=N or boot=on * depending on what qemu supports */ for (i = 0; i < def->os.nBootDevs; i++) { @@ -1936,6 +1936,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, for (i = 0; i < def->ndisks; i++) { char *optstr; unsigned int bootindex = 0; + bool driveBoot = false; virDomainDiskDefPtr disk = def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; @@ -1948,20 +1949,28 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, return -1; } - switch (disk->device) { - case VIR_DOMAIN_DISK_DEVICE_CDROM: - bootindex = bootCD; - bootCD = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - bootindex = bootFloppy; - bootFloppy = 0; - break; - case VIR_DOMAIN_DISK_DEVICE_DISK: - case VIR_DOMAIN_DISK_DEVICE_LUN: - bootindex = bootDisk; - bootDisk = 0; - break; + if (disk->info.bootIndex) { + bootindex = disk->info.bootIndex; + } else { + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + bootindex = bootCD; + bootCD = 0; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + bootindex = bootFloppy; + bootFloppy = 0; + break; + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + bootindex = bootDisk; + bootDisk = 0; + break; + } + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { + driveBoot = !!bootindex; + bootindex = 0; + } } if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) @@ -1969,19 +1978,11 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virCommandAddArg(cmd, "-drive"); - optstr = qemuBuildDriveStr(disk, - emitBootindex ? false : !!bootindex, - qemuCaps); - if (!optstr) + if (!(optstr = qemuBuildDriveStr(disk, driveBoot, qemuCaps))) return -1; virCommandAddArg(cmd, optstr); VIR_FREE(optstr); - if (!emitBootindex) - bootindex = 0; - else if (disk->info.bootIndex) - bootindex = disk->info.bootIndex; - if (qemuDiskBusNeedsDeviceArg(disk->bus)) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { if (virAsprintf(&optstr, "drive%c=drive-%s", @@ -6018,8 +6019,7 @@ qemuBuildPMCommandLine(virCommandPtr cmd, static int qemuBuildBootCommandLine(virCommandPtr cmd, const virDomainDef *def, - virQEMUCapsPtr qemuCaps, - bool *emitBootindex) + virQEMUCapsPtr qemuCaps) { size_t i; virBuffer boot_buf = VIR_BUFFER_INITIALIZER; @@ -6029,8 +6029,9 @@ qemuBuildBootCommandLine(virCommandPtr cmd, * We prefer using explicit bootindex=N parameters for predictable * results even though domain XML doesn't use per device boot elements. */ - *emitBootindex = true; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { + char boot[VIR_DOMAIN_BOOT_LAST+1]; + if (def->os.nBootDevs == 0) { /* def->os.nBootDevs is guaranteed to be > 0 unless per-device boot * configuration is used @@ -6039,11 +6040,6 @@ qemuBuildBootCommandLine(virCommandPtr cmd, _("hypervisor lacks deviceboot feature")); goto error; } - *emitBootindex = false; - } - - if (!*emitBootindex) { - char boot[VIR_DOMAIN_BOOT_LAST+1]; for (i = 0; i < def->os.nBootDevs; i++) { switch (def->os.bootDevs[i]) { @@ -8018,7 +8014,6 @@ qemuBuildNetCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps, virNetDevVPortProfileOp vmop, bool standalone, - bool emitBootindex, size_t *nnicindexes, int **nicindexes, unsigned int *bootHostdevNet) @@ -8030,7 +8025,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd, if (def->nnets) { unsigned int bootNet = 0; - if (emitBootindex) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) { /* convert <boot dev='network'/> to bootindex since we didn't emit * -boot n */ @@ -9137,7 +9132,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, size_t i; char uuid[VIR_UUID_STRING_BUFLEN]; virCommandPtr cmd = NULL; - bool emitBootindex = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); unsigned int bootHostdevNet = 0; @@ -9245,7 +9239,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildPMCommandLine(cmd, def, qemuCaps, monitor_json) < 0) goto error; - if (qemuBuildBootCommandLine(cmd, def, qemuCaps, &emitBootindex) < 0) + if (qemuBuildBootCommandLine(cmd, def, qemuCaps) < 0) goto error; if (qemuBuildGlobalControllerCommandLine(cmd, def, qemuCaps) < 0) @@ -9257,15 +9251,14 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0) goto error; - if (qemuBuildDiskDriveCommandLine(cmd, def, qemuCaps, emitBootindex) < 0) + if (qemuBuildDiskDriveCommandLine(cmd, def, qemuCaps) < 0) goto error; if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0) goto error; if (qemuBuildNetCommandLine(cmd, driver, def, qemuCaps, vmop, standalone, - emitBootindex, nnicindexes, nicindexes, - &bootHostdevNet) < 0) + nnicindexes, nicindexes, &bootHostdevNet) < 0) goto error; if (qemuBuildSmartcardCommandLine(logManager, cmd, cfg, def, qemuCaps) < 0) -- 2.9.0
On Tue, Jun 28, 2016 at 11:17:35PM +0200, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_command.c | 75 ++++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 41 deletions(-)
@@ -8030,7 +8025,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd, if (def->nnets) { unsigned int bootNet = 0;
- if (emitBootindex) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) {
This will override net->info.bootindex. Jan
On Wed, Jun 29, 2016 at 08:32:50 +0200, Ján Tomko wrote:
On Tue, Jun 28, 2016 at 11:17:35PM +0200, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_command.c | 75 ++++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 41 deletions(-)
@@ -8030,7 +8025,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd, if (def->nnets) { unsigned int bootNet = 0;
- if (emitBootindex) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) {
This will override net->info.bootindex.
Could you spend just a tiny bit more words describing what you think this hunk will break? Thanks, Jirka
On Wed, Jun 29, 2016 at 09:20:05AM +0200, Jiri Denemark wrote:
On Wed, Jun 29, 2016 at 08:32:50 +0200, Ján Tomko wrote:
On Tue, Jun 28, 2016 at 11:17:35PM +0200, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_command.c | 75 ++++++++++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 41 deletions(-)
@@ -8030,7 +8025,7 @@ qemuBuildNetCommandLine(virCommandPtr cmd, if (def->nnets) { unsigned int bootNet = 0;
- if (emitBootindex) { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX)) {
This will override net->info.bootindex.
Could you spend just a tiny bit more words describing what you think this hunk will break?
No ;) Jan
On Tue, Jun 28, 2016 at 11:17:33PM +0200, Jiri Denemark wrote:
Jiri Denemark (2): qemu: Use bootindex whenever possible qemu: Drop emitBootindex parameter
src/qemu/qemu_command.c | 89 ++++++++++------------ .../qemuxml2argv-boot-menu-enable-bootindex.args | 23 ++++++ .../qemuxml2argv-boot-menu-enable-bootindex.xml | 28 +++++++ tests/qemuxml2argvtest.c | 2 +- 4 files changed, 91 insertions(+), 51 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-boot-menu-enable-bootindex.xml
ACK series Jan
participants (2)
-
Jiri Denemark -
Ján Tomko