[libvirt] [PATCH] conf: always format os.bootloaderArgs if set
At the moment the bootloader arguments never get formatted if the bootloader is unset. However, in cases where the bootloader defaults to a default value when unset, specifying bootloader arguments does make sense. --- src/conf/domain_conf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 568c699..66bba6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22608,6 +22608,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->os.bootloader) { virBufferEscapeString(buf, "<bootloader>%s</bootloader>\n", def->os.bootloader); + } + if (def->os.bootloaderArgs) { virBufferEscapeString(buf, "<bootloader_args>%s</bootloader_args>\n", def->os.bootloaderArgs); -- 2.7.0
On Tue, May 31, 2016 at 08:06:06PM +0200, Fabian Freyer wrote:
At the moment the bootloader arguments never get formatted if the bootloader is unset. However, in cases where the bootloader defaults to a default value when unset, specifying bootloader arguments does make sense.
Please wrap long lines. If the bootloader defaults to something, it should be set to that default. So NACK to this approach (unless I missed something).
--- src/conf/domain_conf.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 568c699..66bba6e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22608,6 +22608,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->os.bootloader) { virBufferEscapeString(buf, "<bootloader>%s</bootloader>\n", def->os.bootloader); + } + if (def->os.bootloaderArgs) {
the conditions would not be needed, look at how virBufferEscapeString() works.
virBufferEscapeString(buf, "<bootloader_args>%s</bootloader_args>\n", def->os.bootloaderArgs); -- 2.7.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Martin Kletzander wrote:
On Tue, May 31, 2016 at 08:06:06PM +0200, Fabian Freyer wrote:
At the moment the bootloader arguments never get formatted if the bootloader is unset. However, in cases where the bootloader defaults to a default value when unset, specifying bootloader arguments does make sense.
Please wrap long lines. If the bootloader defaults to something, it should be set to that default. So NACK to this approach (unless I missed something).
In other words, if we don't have bootloader defined and decide to go with bhyveload, it means that we need to update XML accordingly? This sounds like a reasonable thing to do, but probably we should agree on the UEFI boot case first. As it does not require the 2-step boot process with external loader, we should differentiate this case in order not to see bootloader when it's not needed. Roman Bogorodskiy
On Mon, Jun 06, 2016 at 07:43:46AM +0300, Roman Bogorodskiy wrote:
Martin Kletzander wrote:
On Tue, May 31, 2016 at 08:06:06PM +0200, Fabian Freyer wrote:
At the moment the bootloader arguments never get formatted if the bootloader is unset. However, in cases where the bootloader defaults to a default value when unset, specifying bootloader arguments does make sense.
Please wrap long lines. If the bootloader defaults to something, it should be set to that default. So NACK to this approach (unless I missed something).
In other words, if we don't have bootloader defined and decide to go with bhyveload, it means that we need to update XML accordingly?
I believe that if something is used and we have a field for that in the XML, then it should be visible there. That way the user has ad much information as possible.
This sounds like a reasonable thing to do, but probably we should agree on the UEFI boot case first. As it does not require the 2-step boot process with external loader, we should differentiate this case in order not to see bootloader when it's not needed.
I don't know what is the default now and how it works. If you want some sensible default to work without being in the XML, well, that's another option (I didn't understand it that way), but from my experience, that will bite you in the back later on. I was just commenting on the patch without knowing the history, so it might make sense to do what you say. I just didn't feel that's the best option based on the commit message.
Roman Bogorodskiy
participants (3)
-
Fabian Freyer -
Martin Kletzander -
Roman Bogorodskiy