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

Auto-mount boot (and several recipe cleanups) #245

Merged
merged 11 commits into from
Feb 4, 2022
Merged

Auto-mount boot (and several recipe cleanups) #245

merged 11 commits into from
Feb 4, 2022

Conversation

MaxKellermann
Copy link
Collaborator

No description provided.


case "${fbcon}" in
"rotate:3")
case "`cat /sys/class/graphics/fbcon/rotate`" in
Copy link
Contributor

Choose a reason for hiding this comment

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

please use new-style subshell syntax $(..) and avoid this "useless use of cat" by using
$(</sys/class/graphics/fbcon/rotate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a bashism, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The redirect is, and we seem not to be using bash here.
But the command substitution should nevertheless be in $()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the `` form is only supported due to backward compatibility. $() is POSIX and current day syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the same document: https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html#tag_23_02_06_03

"Because of these inconsistent behaviors, the backquoted variety of command substitution is not recommended for new applications that nest command substitutions or attempt to embed complex scripts."

More clear here in the BashFAQ:
https://mywiki.wooledge.org/BashFAQ/082 :
... is the legacy syntax required by only the very oldest of non-POSIX-compatible bourne-shells. There are several reasons to always prefer the $(...) syntax:

https://guide.bash.academy/expansions/?=Command_Substitution#p1.3.0_7

"As a closing note, I will make brief mention of the deprecated ... syntax. Old-style bourne shells used this syntax for Command Substitution instead of the more modern $(...) syntax. In bash and all modern POSIX shells, both syntaxes are supported, but it is highly recommended that you stop using the backtick (`) syntax and change this syntax into the value-expansion equivalent whenever you see it used in the wild. Although they are functionally equivalent, the backtick variant has some very important downsides:

The backtick syntax looks a lot like quoting. This has caused widespread confusion among users. Even to the trained eye, it is sometimes hard to not forget that backtick expansions still need double-quotes around them to be safe, just like all other value expansions.
The backtick syntax is inconsistent with value expansions. The beauty of $(...) is that is clearly communicates that we are expanding a value into place here, just like all other dollar-style value expansions do. This clarity is absent with the backtick syntax.
The backtick syntax turns quoting and nesting into an exercise in absurdity. It requires a maze of backslash-escaping which makes it almost impossible to parse and nearly guarantees that you will make mistakes: echo "`echo \"\`echo \\"hello\\"\`\"`" vs. echo "$(echo "$(echo "hello")")"

Shellcheck will also complain:
https://github.com/koalaman/shellcheck/wiki/SC2006

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... that nest command substitutions or attempt to embed complex scripts

This precondition does not apply here. This is no nesting and no complex script.

This is not a bash script, so a bash FAQ is not relevant here.

Copy link
Contributor

Choose a reason for hiding this comment

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

BASH is a BourneShell

... that nest command substitutions or attempt to embed complex scripts

This precondition does not apply here. This is no nesting and no complex script.

The point is to habitually not use this syntax at all.
Someone may copy or modify your code and not know the limitations / dangers.

This is not a bash script, so a bash FAQ is not relevant here.
Your escaping on technicalities here:
BASH is a BourneShell compatible shell, which adds many new features to its ancestor. Most of them are available in the KornShell, too. The answers given in this FAQ may be slanted toward Bash, or they may be slanted toward the lowest common denominator Bourne shell, depending on who wrote the answer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a POSIX shell script (/bin/sh), and neither bash nor bourne nor korn shell script. None of that is relevant.
You can say you prefer $() over backticks because you like it more, as a matter of personal taste, and the OpenVario project could even make it a coding style rule. All of that would be valid.
But you can't claim it's deprecated or only in POSIX for backwards compatibility, because that's not true.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lovely discussion on what deprecated in this context means here:
https://unix.stackexchange.com/questions/126927/have-backticks-i-e-cmd-in-sh-shells-been-deprecated

Backwards compatibility: If you add a new method to the standard, that eclipses all function of your old method, why would you keep the old function? The only reason is Backward compatibility.

Copy link
Contributor

@lordfolken lordfolken left a comment

Choose a reason for hiding this comment

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

This shouldn't be in /usr/bin as its a "foreing" package and not part of the usual distribution.
Clean and according to FHS this should be /opt/ovmenu-ng/bin/ovmenu-ng.sh
This would also be good for the ovmenu-ng-skripts and the tscal packages.

@MaxKellermann
Copy link
Collaborator Author

It is part of the usual OpenVario distribution, just like XCSoar and anything else. There's no point in using /opt here.

It makes no sense to use two different recipes for this.  If you
install ovmenu-ng, it should contain the systemd unit, just like all
other packages which provide systemd units.
…of /proc/cmdline

This is simpler and more robust.
This avoids the security problems mentioned in the two issues ("root
password is blank") not by actually setting a root password, but by
disabling SSH access by default.  There is a new menu to enable it (or
enable it temporarily until the next reboot).

Closes #27
Closes #224
@MaxKellermann
Copy link
Collaborator Author

If you believe "disable SSH access by default" is debatable, I'll remove it from this PR and create a separate PR for discussing it.


[Mount]
What=/dev/mmcblk0p1
Where=/boot
Copy link
Member

Choose a reason for hiding this comment

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

Would /etc/fstab entry be a simpler solution for this?

Copy link
Member

Choose a reason for hiding this comment

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

Think we can change this afterwards as well, but it doesn't matter from my point of view ..

@linuxianer99 linuxianer99 merged commit 5f85d70 into Openvario:master Feb 4, 2022
@MaxKellermann
Copy link
Collaborator Author

MaxKellermann commented Feb 4, 2022 via email

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.

4 participants