[libvirt] [PATCHv2 0/2] Boot options cleanup
Depends on the first two (ACKed) patches from: https://www.redhat.com/archives/libvir-list/2015-February/msg00662.html Patch 4/5 Rename boot_buf to boot_opts was dropped. In this version, only one virBuffer is used and just the strings are separate. Ján Tomko (2): Make -boot arg generation more readable Use virBufferTrim when generating boot options src/qemu/qemu_command.c | 59 ++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 33 deletions(-) -- 2.0.5
If we combine the boot order on the command line with other boot options, we prepend order= in front of it. Instead of checking if the number of added arguments is between 0 and 2, separate the strings for boot order and options and prepend boot order only if both strings are not empty. --- src/qemu/qemu_command.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 736e285..b504461 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8213,6 +8213,7 @@ qemuBuildCommandLine(virConnectPtr conn, virArch hostarch = virArchFromHost(); virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virBuffer boot_buf = VIR_BUFFER_INITIALIZER; + char *boot_order_str = NULL, *boot_opts_str = NULL; int boot_nparams = 0; VIR_DEBUG("conn=%p driver=%p def=%p mon=%p json=%d " @@ -8778,7 +8779,9 @@ qemuBuildCommandLine(virConnectPtr conn, boot[def->os.nBootDevs] = '\0'; virBufferAsprintf(&boot_buf, "%s", boot); - boot_nparams++; + if (virBufferCheckError(&boot_buf) < 0) + goto error; + boot_order_str = virBufferContentAndReset(&boot_buf); } if (def->os.bootmenu) { @@ -8834,23 +8837,23 @@ qemuBuildCommandLine(virConnectPtr conn, virBufferAddLit(&boot_buf, "strict=on"); } - if (boot_nparams > 0) { - virCommandAddArg(cmd, "-boot"); + if (virBufferCheckError(&boot_buf) < 0) + goto error; - if (virBufferCheckError(&boot_buf) < 0) - goto error; + boot_opts_str = virBufferContentAndReset(&boot_buf); + if (boot_order_str || boot_opts_str) { + virCommandAddArg(cmd, "-boot"); - if (boot_nparams < 2 || emitBootindex) { - virCommandAddArgBuffer(cmd, &boot_buf); - virBufferFreeAndReset(&boot_buf); - } else { - char *str = virBufferContentAndReset(&boot_buf); - virCommandAddArgFormat(cmd, - "order=%s", - str); - VIR_FREE(str); + if (boot_order_str && boot_opts_str) { + virCommandAddArgFormat(cmd, "order=%s,%s", + boot_order_str, boot_opts_str); + } else if (boot_order_str) { + virCommandAddArg(cmd, boot_order_str); + } else if (boot_opts_str) { + virCommandAddArg(cmd, boot_opts_str); } } + VIR_FREE(boot_opts_str); if (def->os.kernel) virCommandAddArgList(cmd, "-kernel", def->os.kernel, NULL); @@ -10342,6 +10345,7 @@ qemuBuildCommandLine(virConnectPtr conn, return cmd; error: + VIR_FREE(boot_order_str); virBufferFreeAndReset(&boot_buf); virObjectUnref(cfg); /* free up any resources in the network driver -- 2.0.5
Instead of tracking the number of added parameters, add a comma at the end of each one unconditionally and trim the trailing one at the end. --- src/qemu/qemu_command.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b504461..9da3c06 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8214,7 +8214,6 @@ qemuBuildCommandLine(virConnectPtr conn, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virBuffer boot_buf = VIR_BUFFER_INITIALIZER; char *boot_order_str = NULL, *boot_opts_str = NULL; - int boot_nparams = 0; VIR_DEBUG("conn=%p driver=%p def=%p mon=%p json=%d " "qemuCaps=%p migrateFrom=%s migrateFD=%d " @@ -8786,13 +8785,10 @@ qemuBuildCommandLine(virConnectPtr conn, if (def->os.bootmenu) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_MENU)) { - if (boot_nparams++) - virBufferAddChar(&boot_buf, ','); - if (def->os.bootmenu == VIR_TRISTATE_BOOL_YES) - virBufferAddLit(&boot_buf, "menu=on"); + virBufferAddLit(&boot_buf, "menu=on,"); else - virBufferAddLit(&boot_buf, "menu=off"); + virBufferAddLit(&boot_buf, "menu=off,"); } else { /* We cannot emit an error when bootmenu is enabled but * unsupported because of backward compatibility */ @@ -8809,11 +8805,8 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - if (boot_nparams++) - virBufferAddChar(&boot_buf, ','); - virBufferAsprintf(&boot_buf, - "reboot-timeout=%d", + "reboot-timeout=%d,", def->os.bios.rt_delay); } @@ -8825,17 +8818,13 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - if (boot_nparams++) - virBufferAddChar(&boot_buf, ','); - - virBufferAsprintf(&boot_buf, "splash-time=%u", def->os.bm_timeout); + virBufferAsprintf(&boot_buf, "splash-time=%u,", def->os.bm_timeout); } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_STRICT)) { - if (boot_nparams++) - virBufferAddChar(&boot_buf, ','); - virBufferAddLit(&boot_buf, "strict=on"); - } + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOT_STRICT)) + virBufferAddLit(&boot_buf, "strict=on,"); + + virBufferTrim(&boot_buf, ",", -1); if (virBufferCheckError(&boot_buf) < 0) goto error; -- 2.0.5
On 02/25/2015 07:52 AM, Ján Tomko wrote:
Depends on the first two (ACKed) patches from: https://www.redhat.com/archives/libvir-list/2015-February/msg00662.html
Patch 4/5 Rename boot_buf to boot_opts was dropped. In this version, only one virBuffer is used and just the strings are separate.
Ján Tomko (2): Make -boot arg generation more readable Use virBufferTrim when generating boot options
src/qemu/qemu_command.c | 59 ++++++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 33 deletions(-)
ACK both If I understand the rules of the road correctly... Since the original series was reviewed prior to the freeze and this just adjust that, it seems reasonable to say it's OK for freeze... John
On Wed, Feb 25, 2015 at 09:17:27 -0500, John Ferlan wrote:
On 02/25/2015 07:52 AM, Ján Tomko wrote:
...
ACK both
If I understand the rules of the road correctly... Since the original series was reviewed prior to the freeze and this just adjust that, it seems reasonable to say it's OK for freeze...
Actually I'd rather not codify that as a rule. After the freeze everythling should be re-evaluated if it makes sense actually to push. Purpose of the freeze is not to limit new features appearing but have a line where stuff that is likely to break the comming release in any way to be limited. If you get a review for a big feature prior to the freeze and then send a few patches after the freeze it will not make them automagically appear in the RC-package or any less likely to break the release. I think only fixes that target code that was touched in the last devel cycle or fix a obvious bug in a common path should be taken, otherwise we might as well as not have any freeze. Peter
On Wed, Feb 25, 2015 at 04:40:00PM +0100, Peter Krempa wrote:
On Wed, Feb 25, 2015 at 09:17:27 -0500, John Ferlan wrote:
On 02/25/2015 07:52 AM, Ján Tomko wrote:
...
ACK both
If I understand the rules of the road correctly... Since the original series was reviewed prior to the freeze and this just adjust that, it seems reasonable to say it's OK for freeze...
Actually I'd rather not codify that as a rule. After the freeze everythling should be re-evaluated if it makes sense actually to push.
Purpose of the freeze is not to limit new features appearing but have a line where stuff that is likely to break the comming release in any way to be limited.
If you get a review for a big feature prior to the freeze and then send a few patches after the freeze it will not make them automagically appear in the RC-package or any less likely to break the release.
I think only fixes that target code that was touched in the last devel cycle or fix a obvious bug in a common path should be taken, otherwise we might as well as not have any freeze.
Yep, any ack'd features really should be pushed before the freeze starts. If they miss that then they ought to wait in general, as we frequently see that new feature patches break the build on Win32 or BSD platforms, and validating those platforms is the main point of having a freeze. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/25/2015 10:40 AM, Peter Krempa wrote:
On Wed, Feb 25, 2015 at 09:17:27 -0500, John Ferlan wrote:
On 02/25/2015 07:52 AM, Ján Tomko wrote:
...
ACK both
If I understand the rules of the road correctly... Since the original series was reviewed prior to the freeze and this just adjust that, it seems reasonable to say it's OK for freeze...
Actually I'd rather not codify that as a rule. After the freeze everythling should be re-evaluated if it makes sense actually to push.
Purpose of the freeze is not to limit new features appearing but have a line where stuff that is likely to break the comming release in any way to be limited.
If you get a review for a big feature prior to the freeze and then send a few patches after the freeze it will not make them automagically appear in the RC-package or any less likely to break the release.
I think only fixes that target code that was touched in the last devel cycle or fix a obvious bug in a common path should be taken, otherwise we might as well as not have any freeze.
Hmm.. perhaps let me clarify my thoughts... These are bug fix patches which I had differentiated in my mind compared to patches for some feature addition when I wrote the above. Whether as a general rule of thumb we as a group adhere to your last paragraph regarding last devel cycle perhaps has a lot more to do with the ability to provide timely patch reviews when everyone is trying to submit patches to "beat" the freeze date and performing fewer reviews until after that date (especially in this global development cycle where times and dates are relative to your location). For this sequence of patches, the original patches were posted 2/18... I reviewed 2/23... Freeze 2/24... These changes were posted 2/25 as a reaction to review comments. Could they wait - sure... But it seems they are no less safe/harmful than some other patches taken after freezes in my historical recollection... John
On Wed, Feb 25, 2015 at 04:40:00PM +0100, Peter Krempa wrote:
On Wed, Feb 25, 2015 at 09:17:27 -0500, John Ferlan wrote:
On 02/25/2015 07:52 AM, Ján Tomko wrote:
...
ACK both
If I understand the rules of the road correctly... Since the original series was reviewed prior to the freeze and this just adjust that, it seems reasonable to say it's OK for freeze...
The original series was fixing a bug (-bootloader appearing on QEMU command line), but I didn't consider it important enough to push the already ACKed patches before sending another version of this cleanup. These two patches have no functional change, they aren't needed in this release.
Actually I'd rather not codify that as a rule. After the freeze everythling should be re-evaluated if it makes sense actually to push.
Purpose of the freeze is not to limit new features appearing but have a line where stuff that is likely to break the comming release in any way to be limited.
Actually, I always thought it was a feature freeze. Merging a new feature usually is more likely to break something than not.
If you get a review for a big feature prior to the freeze and then send a few patches after the freeze it will not make them automagically appear in the RC-package or any less likely to break the release.
If the feature was merged before the freeze, then the followup patches are fixing bugs in it, aren't they? :)
I think only fixes that target code that was touched in the last devel cycle or fix a obvious bug in a common path should be taken, otherwise we might as well as not have any freeze.
Fixes in less common paths should be taken too if they're serious enough. Either way, I'm bookmarking this thread. Jan
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Ján Tomko -
Peter Krempa