Re: install.sub: Get rid of useless/confusing subshell
On October 10, 2022 12:45:09 AM GMT+02:00, Klemens Nanni
wrote:
>On Wed, Oct 05, 2022 at 04:56:57PM +0200, Alexander Hall wrote:
>> While I dislike the ">/dev/null 2>&1" sledgehammer, this is in the right
>> direction.
>
>Agreed.
>
>We should be fine silencing only the test condition which produces legit
>output and warnings.
>
>All else produces no output and should not error out; if it does, those
>warnings should be printed and fixed.
>
>Feedback? OK?
Just what I had in mind, with one optional nit below.
OK halex@ with or without said nit.
>
>Index: install.sub
>===
>RCS file: /cvs/src/distrib/miniroot/install.sub,v
>retrieving revision 1.1210
>diff -u -p -U4 -r1.1210 install.sub
>--- install.sub5 Oct 2022 19:30:47 - 1.1210
>+++ install.sub9 Oct 2022 22:43:41 -
>@@ -3316,17 +3316,17 @@ check_unattendedupgrade() {
>
> _d=${_d%% *}
> if [[ -n $_d ]]; then
> make_dev $_d
>- if mount -t ffs -r /dev/${_d}a /mnt; then
>+ if mount -t ffs -r /dev/${_d}a /mnt >/dev/null 2>&1; then
Sorry for not doing the test myself, but does it even need to silence stdout?
Wouldn't "2>/dev/null" suffice?
/Alexander
> [[ -f /mnt/bsd.upgrade && -f /mnt/auto_upgrade.conf ]]
> _rc=$?
> ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
> echo "Which disk is the root disk = ${_d}" >>
> /auto_upgrade.conf
> umount /mnt
> fi
> rm -f /dev/{r,}$_d?
>- fi >/dev/null 2>&1
>+ fi
>
> return $_rc
> }
>
Re: install.sub: Get rid of useless/confusing subshell
On Wed, Oct 05, 2022 at 04:56:57PM +0200, Alexander Hall wrote:
> While I dislike the ">/dev/null 2>&1" sledgehammer, this is in the right
> direction.
Agreed.
We should be fine silencing only the test condition which produces legit
output and warnings.
All else produces no output and should not error out; if it does, those
warnings should be printed and fixed.
Feedback? OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1210
diff -u -p -U4 -r1.1210 install.sub
--- install.sub 5 Oct 2022 19:30:47 - 1.1210
+++ install.sub 9 Oct 2022 22:43:41 -
@@ -3316,17 +3316,17 @@ check_unattendedupgrade() {
_d=${_d%% *}
if [[ -n $_d ]]; then
make_dev $_d
- if mount -t ffs -r /dev/${_d}a /mnt; then
+ if mount -t ffs -r /dev/${_d}a /mnt >/dev/null 2>&1; then
[[ -f /mnt/bsd.upgrade && -f /mnt/auto_upgrade.conf ]]
_rc=$?
((_rc == 0)) && cp /mnt/auto_upgrade.conf /
echo "Which disk is the root disk = ${_d}" >>
/auto_upgrade.conf
umount /mnt
fi
rm -f /dev/{r,}$_d?
- fi >/dev/null 2>&1
+ fi
return $_rc
}
Re: install.sub: Get rid of useless/confusing subshell
On October 4, 2022 10:11:46 PM GMT+02:00, Klemens Nanni
wrote:
>This function's style is a bit off: it wraps the body in a subshell to
>discard all stdout/err at once, but a still uses return inside it.
>
>1. A command list (using {}) would be enough here as it groups like a
> subshell but avoids spawning another shell;
>2. discarding stdout/err at the end of an if block works the same
> (effecting both condition and body) and saves one level of indent;
>3. return inside a subshell inside a function does NOT return from the
> function but merely exits the subshell; this is easily misread.
>
>Saving a fork and indent and improving readability boils down to this
>(cvs diff -wU1):
>
> |@@ -3320,3 +3317,2 @@ check_unattendedupgrade() {
> | _d=${_d%% *}
> |- (
> | if [[ -n $_d ]]; then
> |@@ -3331,5 +3327,5 @@ check_unattendedupgrade() {
> | rm -f /dev/{r,}$_d?
> |- fi
> |+ fi >/dev/null 2>&1
> |+
> | return $_rc
> |- ) > /dev/null 2>&1
> | }
>
>Feedback? OK?
While I dislike the ">/dev/null 2>&1" sledgehammer, this is in the right
direction.
ok halex@
>
>
>Index: install.sub
>===
>RCS file: /cvs/src/distrib/miniroot/install.sub,v
>retrieving revision 1.1209
>diff -u -p -r1.1209 install.sub
>--- install.sub4 Oct 2022 19:59:10 - 1.1209
>+++ install.sub4 Oct 2022 20:00:54 -
>@@ -3315,20 +3315,19 @@ check_unattendedupgrade() {
> local _d=$(get_dkdevs_root) _rc=1
>
> _d=${_d%% *}
>- (
>- if [[ -n $_d ]]; then
>- make_dev $_d
>- if mount -t ffs -r /dev/${_d}a /mnt; then
>- [[ -f /mnt/bsd.upgrade && -f
>/mnt/auto_upgrade.conf ]]
>- _rc=$?
>- ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
>- echo "Which disk is the root disk = ${_d}" >>
>/auto_upgrade.conf
>- umount /mnt
>- fi
>- rm -f /dev/{r,}$_d?
>+ if [[ -n $_d ]]; then
>+ make_dev $_d
>+ if mount -t ffs -r /dev/${_d}a /mnt; then
>+ [[ -f /mnt/bsd.upgrade && -f /mnt/auto_upgrade.conf ]]
>+ _rc=$?
>+ ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
>+ echo "Which disk is the root disk = ${_d}" >>
>/auto_upgrade.conf
>+ umount /mnt
> fi
>- return $_rc
>- ) > /dev/null 2>&1
>+ rm -f /dev/{r,}$_d?
>+ fi >/dev/null 2>&1
>+
>+ return $_rc
> }
>
> WATCHDOG_PERIOD_SEC=$((30 * 60))
>
install.sub: Get rid of useless/confusing subshell
This function's style is a bit off: it wraps the body in a subshell to
discard all stdout/err at once, but a still uses return inside it.
1. A command list (using {}) would be enough here as it groups like a
subshell but avoids spawning another shell;
2. discarding stdout/err at the end of an if block works the same
(effecting both condition and body) and saves one level of indent;
3. return inside a subshell inside a function does NOT return from the
function but merely exits the subshell; this is easily misread.
Saving a fork and indent and improving readability boils down to this
(cvs diff -wU1):
|@@ -3320,3 +3317,2 @@ check_unattendedupgrade() {
| _d=${_d%% *}
|- (
| if [[ -n $_d ]]; then
|@@ -3331,5 +3327,5 @@ check_unattendedupgrade() {
| rm -f /dev/{r,}$_d?
|- fi
|+ fi >/dev/null 2>&1
|+
| return $_rc
|- ) > /dev/null 2>&1
| }
Feedback? OK?
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1209
diff -u -p -r1.1209 install.sub
--- install.sub 4 Oct 2022 19:59:10 - 1.1209
+++ install.sub 4 Oct 2022 20:00:54 -
@@ -3315,20 +3315,19 @@ check_unattendedupgrade() {
local _d=$(get_dkdevs_root) _rc=1
_d=${_d%% *}
- (
- if [[ -n $_d ]]; then
- make_dev $_d
- if mount -t ffs -r /dev/${_d}a /mnt; then
- [[ -f /mnt/bsd.upgrade && -f
/mnt/auto_upgrade.conf ]]
- _rc=$?
- ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
- echo "Which disk is the root disk = ${_d}" >>
/auto_upgrade.conf
- umount /mnt
- fi
- rm -f /dev/{r,}$_d?
+ if [[ -n $_d ]]; then
+ make_dev $_d
+ if mount -t ffs -r /dev/${_d}a /mnt; then
+ [[ -f /mnt/bsd.upgrade && -f /mnt/auto_upgrade.conf ]]
+ _rc=$?
+ ((_rc == 0)) && cp /mnt/auto_upgrade.conf /
+ echo "Which disk is the root disk = ${_d}" >>
/auto_upgrade.conf
+ umount /mnt
fi
- return $_rc
- ) > /dev/null 2>&1
+ rm -f /dev/{r,}$_d?
+ fi >/dev/null 2>&1
+
+ return $_rc
}
WATCHDOG_PERIOD_SEC=$((30 * 60))
