Fixes and improvements #27
Conversation
- dbus: fixes timedatectl bug - man-db, manpages, less, bash-completion: useful
jjakob
force-pushed
the
gbpc2_fixes_rebase
branch
from
ca375d7 to
1f6c0cb
Compare
The initramfs didn't contain dropbearkey so host keys weren't generated. Use -R instead. Should be in all dropbear versions from jessie onward.
|
Thanks for the patches....
is pointless. Also with variable assignment there is no wildcard or IFS interpretation so
does not need extra quotes ... except maybe around the "$prog". It is always appropriate to quote a variable when it appears in command like arguments (unless you want spaces to be processed), but most other places where you have added quotes, I'd really rather not have them. Also, you have added E: W: I: in front of lots of messages, without any explanation in the commit message. Your other changes that change the behaviour, like the dropbear changes and the debian mirror configuration do look useful. If you could provide a pull request that just, I would appreciate it. Thanks. |
|
I add quotes around everything, even if not technically needed, out of habit. I agree quotes around constant strings are pointless. Others I add due to security, to prevent possibly malicious code getting executed (ACE). Since using a remote gnubee system as the source of many variables this is also possible if the system ssh connects to a different host or that host is compromised. I know, it's very unlikely in this case as the environments are controlled, but more quoting in the right places doesn't do any harm. I'll go through it and see what I can do. |
There was a problem hiding this comment.
Refrain from doing cosmetic changes (whitespace, comments, etc.) in the same commit as semantic changes, this makes them harder to review.
When you do a series of commits, put the cosmetic ones last, so that they can be easily skipped, as they often are a matter of taste.
So the semantic changes can be merged anyways, separately from the controversial ones.
It's a shame to see such contributions being ignored or postponed upon cosmetic discussions, as there are good things that could go in easily.
For this series to be (partially-) mergeable, I'd suggest at least splitting some commits, those that are mixing different things.
And maybe also splitting the whole PR into smaller logical pieces with one subject only.
Keep the cosmetical commits at the end of the serie(s) so that they don't block the whole PR.
I don't know if you're still interested in following up on this, just say so.
In any case, thanks for your work.
| fi | ||
| dropbear | ||
|
|
||
| # -R to generate new host keys on each boot |
There was a problem hiding this comment.
Is this comment accurate ?
Reading the man page @ https://manpages.org/dropbear/8
I'm not sure I can tell if it'll only be generating keys if they are absent or if it will (re-)generate keys at each invokation.
I've not tested, sorry, just asking while looking at your PR...
| @@ -16,7 +16,7 @@ | |||
| mp=/tmp/newroot | |||
| default_debian_suite=buster | |||
| default_debian_mirror="http://httpredir.debian.org/debian" | |||
| include_packages="vim,openssh-server,ntpdate,cron,locales,udev,fake-hwclock,mtd-utils,ca-certificates,apt-transport-https,vlan" | |||
| include_packages="vim,openssh-server,ntpdate,cron,locales,udev,fake-hwclock,mtd-utils,ca-certificates,apt-transport-https,vlan,bash-completion,less,man-db,manpages,dbus" | |||
There was a problem hiding this comment.
I'd separate that into 2 commits, one for the fix, maybe adding a reference in the commitmsg to the "timedatectl bug" that is being fixed.
And another commit for the "useful" stuff that may be a matter of taste (i.e.: you prefer minimalistic or with batteries included)
initramfs/config
Outdated
| @@ -16,7 +16,7 @@ | |||
| mp=/tmp/newroot | |||
| default_debian_suite=buster | |||
| default_debian_mirror="http://httpredir.debian.org/debian" | |||
| include_packages="vim,openssh-server,ntpdate,cron,locales,udev,fake-hwclock,mtd-utils,ca-certificates,apt-transport-https,vlan,libnl-3-200,libnl-genl-3-200" | |||
| include_packages="vim,openssh-server,ntpdate,cron,locales,udev,fake-hwclock,mtd-utils,ca-certificates,apt-transport-https,vlan" | |||
There was a problem hiding this comment.
I was asking myself a question about those version-specific packages, wrt your change about the default debian version to be used for the rootfs.
So is this removal because "they are not needed in the new debian version any more" or is it something else ?
In fact, I only ask to add a little bit more details to the commit msg, the actual patch may be fine.
| @@ -14,6 +14,8 @@ | |||
| # add leds udev rule | |||
|
|
|||
| mp=/tmp/newroot | |||
| default_debian_suite=buster | |||
There was a problem hiding this comment.
How is that related to the other change in config.sample:
GNUBEE_DEBIAN_SUITE="stable"
| @@ -72,9 +72,9 @@ gnubee_defconfig() { | |||
| case $mach in | |||
| "" | defconfig ) # guess | |||
| if mach=`gnubee_model`; then | |||
| echo Using config $mach | |||
| echo "I: Using config $mach" | |||
There was a problem hiding this comment.
I'm no fan of those one-letter prefixes, what does the "I" stand for ? "E" or "W", I can guess... But I also prefer the longer, more explicit ones.
There was a problem hiding this comment.
I: is info. Doing this is consistent with the messages that the debian installer produces so it certainly makes sense for the messages generated by the "config" script.
It is less obvious that it would be appropriate for the gbmake script, but maybe it does and I'm certainly open to making it clearer.
There was a problem hiding this comment.
OK debian consistency is an accepted excuse ;-)
This fixes some bugs I ran into and adds some new features. See the commits for more information.