Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg: grub: Replace echo command by true command #3283

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

rene
Copy link
Contributor

@rene rene commented Jun 14, 2023

The load_hv_cmd variable present in EVE's GRUB config points to the command used to load a hypervisor. However, in case of "kvm" and "kubervirt" this command is not needed and it's just translated to an echo command. However, this approach will make messages like the following to appear during a KVM based boot:

/boot/xen.gz console=com1 smt=false clocksource=pit dom0_mem=800M,max:800M dom0_max_vcpus=1 dom0_vcpus_pin

This can lead to misunderstandings and confusion since a Xen's boot command line it's being printed on a KVM's system.

This commit replaces echo by true command. The true command can receive any number of arguments and will always return true without any printing, so no Xen's boot command line will be printed in "kvm" or "kubevirt" cases.

The load_hv_cmd variable present in EVE's GRUB config points to the command
used to load a hypervisor. However, in case of "kvm" and "kubervirt" this
command is not needed and it's just translated to an echo command. However,
this approach will make messages like the following to appear during a KVM
based boot:

/boot/xen.gz console=com1 smt=false clocksource=pit dom0_mem=800M,max:800M
dom0_max_vcpus=1 dom0_vcpus_pin

This can lead to misunderstandings and confusion since a Xen's boot command
line it's being printed on a KVM's system.

This commit replaces echo by true command. The true command can receive any
number of arguments and will always return true without any printing, so
no Xen's boot command line will be printed in "kvm" or "kubevirt" cases.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
@rene rene requested review from eriknordmark and rvs as code owners June 14, 2023 16:27
@OhmSpectator
Copy link
Member

OhmSpectator commented Jun 14, 2023

I'm wondering if this modification will have any impact on the bootargs. It's important to note that even in a KVM environment, the dom0_* arguments are required because they are utilized by the system's startup scripts. For example:
https://github.com/lf-edge/eve/blob/master/pkg/dom0-ztools/rootfs/etc/init.d/010-eve-cgroup

@rene
Copy link
Contributor Author

rene commented Jun 14, 2023

I'm wondering if this modification will have any impact on the bootargs. It's important to note that even in a KVM environment, the dom0_* arguments are required because they are utilized by the system's startup scripts. For example: https://github.com/lf-edge/eve/blob/master/pkg/dom0-ztools/rootfs/etc/init.d/010-eve-cgroup

@OhmSpectator , I don't see how this change could impact on dom0_* arguments because the boot entry will just execute an echo command for load_hv_cmd (https://github.com/lf-edge/eve/blob/master/pkg/grub/rootfs.cfg#L397). The line right after is the one which loads the dom0 (just linux in case of KVM) with all dom0_ * arguments...

@OhmSpectator
Copy link
Member

@OhmSpectator , I don't see how this change could impact on dom0_* arguments because the boot entry will just execute an echo command for load_hv_cmd (https://github.com/lf-edge/eve/blob/master/pkg/grub/rootfs.cfg#L397). The line right after is the one which loads the dom0 (just linux in case of KVM) with all dom0_ * arguments...

I just had to ask that to be sure. Thanks!

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eriknordmark eriknordmark merged commit 5cadd87 into lf-edge:master Jun 15, 2023
@rene rene deleted the grub-echo branch September 15, 2023 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants