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.

Sign up for GitHub

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

Jump to bottom

Use double quotes for remove_temporary_mountpoint "$BUILD_DIR/..." #2675

Merged
merged 6 commits into from Sep 9, 2021
Merged

Use double quotes for remove_temporary_mountpoint "$BUILD_DIR/..." #2675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bdb70d6
Update _input-output-functions.sh
jsmeix Sep 3, 2021
190cf10
Update global-functions.sh
jsmeix Sep 3, 2021
f930748
Update global-functions.sh
jsmeix Sep 8, 2021
dcb5ef3
Update global-functions.sh
jsmeix Sep 8, 2021
2965acf
Update global-functions.sh
jsmeix Sep 9, 2021
c8221eb
Update global-functions.sh
jsmeix Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion usr/share/rear/lib/_input-output-functions.sh
View file
Open in desktop
Expand Up @@ -898,7 +898,7 @@ function cleanup_build_area_and_end_program () {
# which is not yet sourced in case of early Error() in usr/sbin/rear
if has_binary remove_temporary_mountpoint ; then
# It is a bug in ReaR if BUILD_DIR/outputfs was not properly umounted and made empty by the scripts before:
remove_temporary_mountpoint '$BUILD_DIR/outputfs' || BugError "Directory $BUILD_DIR/outputfs not empty, cannot remove"
remove_temporary_mountpoint "$BUILD_DIR/outputfs" || BugError "Directory $BUILD_DIR/outputfs not empty, cannot remove"
fi
if ! rmdir $v "$BUILD_DIR" ; then
LogPrintError "Could not remove build area $BUILD_DIR (something still exists therein)"
Expand Down
22 changes: 19 additions & 3 deletions usr/share/rear/lib/global-functions.sh
View file
Open in desktop
Expand Up @@ -344,7 +344,20 @@ function url_path() {

### Returns true if one can upload files to the URL
function scheme_accepts_files() {
local scheme=$1
# Be safe against 'set -eu' which would exit 'rear' with "bash: $1: unbound variable"
# when scheme_accepts_files is called without an argument
# by bash parameter expansion with using an empty default value if $1 is unset or null.
# Bash parameter expansion with assigning a default value ${1:=} does not work
# (then it would still exit with "bash: $1: cannot assign in this way")
# but using a default value is practicable here because $1 is used only once
# cf. https://github.com/rear/rear/pull/2675#discussion_r705018956
local scheme=${1:-}
# Return false if scheme is empty or blank (e.g. when OUTPUT_URL is unset or empty or blank)
# cf. https://github.com/rear/rear/issues/2676
# and https://github.com/rear/rear/issues/2667#issuecomment-914447326
# also return false if scheme is more than one word (so no quoted "$scheme" here)
# cf. https://github.com/rear/rear/pull/2675#discussion_r704401462
test $scheme || return 1
case $scheme in
(null|tape|obdr)
# tapes do not support uploading arbitrary files, one has to handle them
Expand All @@ -368,7 +381,10 @@ function scheme_accepts_files() {
### Returning true does not imply that the URL is currently mounted at a filesystem and usable,
### only that it can be mounted (use mount_url() first)
function scheme_supports_filesystem() {
local scheme=$1
# Be safe against 'set -eu' exit if scheme_supports_filesystem is called without argument
local scheme=${1:-}
# Return false if scheme is empty or blank or more than one word, cf. scheme_accepts_files() above
test $scheme || return 1
Copy link
Member

Choose a reason for hiding this comment

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

don't you also need to quote $scheme in callers? In principle it should work even without quoting, because this function takes only one argument, but it will cause problems when set -eu is used ($1 can be undefined).

Copy link
Member

Choose a reason for hiding this comment

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

for consistency, I think that the same thing should be added to scheme_accepts_files() above.

Copy link
Member Author

@jsmeix jsmeix Sep 8, 2021 •
edited

Choose a reason for hiding this comment

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

I use it without quotes to not let it falsely succeed if $scheme is blank:

# unset scheme
# test $scheme && echo y || echo n
n

# scheme=""
# test $scheme && echo y || echo n
n

# scheme=" "
# test $scheme && echo y || echo n
n

I do no longer really care about set -eu
because that caused much more false exits
than it benefits in practice.

Copy link
Member Author

@jsmeix jsmeix Sep 8, 2021 •
edited

Choose a reason for hiding this comment

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

And it should also fail if scheme is more than one word

# scheme="foo bar"

# test $scheme && echo y || echo n
-bash: test: foo: unary operator expected
n

That doesn't look nice but actually it is perfectly right
to have that bash stderr message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cf. "Beware of the emptiness" in
https://github.com/rear/rear/wiki/Coding-Style

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented via
2965acf

Copy link
Member Author

@jsmeix jsmeix Sep 9, 2021 •
edited

Choose a reason for hiding this comment

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

I think I found out why if failed above with bash: $2: cannot assign in this way.
"man bash" reads (excerpt)

Positional Parameters
...
Positional parameters may not be assigned to with assignment statements.

Copy link
Member

Choose a reason for hiding this comment

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

because in general it is better to improve the functions instead of all its callers.

in general yes. but I think in case of quoting of parameters that can expand to empty strings or whitespace I would add quotes to the caller, because in general this can not be always compensated for inside the functions (imagine a function with more parameters, $scheme being the first of them - if it is empty, the others will shift - this is the case of output_path() and backup_path() functions here) and it is better to consistently give a correct example if the particular case does not strictly require it (other will copy and adapt the code). (I should have remembered this rule when writing the code and use quotes everywhere - I did not realize that $scheme may be empty.)

Copy link
Member

Choose a reason for hiding this comment

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

I think I found out why if failed above with bash: $2: cannot assign in this way.
"man bash" reads (excerpt)

Positional Parameters
...
Positional parameters may not be assigned to with assignment statements.

that must be it. Ans as in ReaR function arguments get quite consistently saved to local variables with more descriptive names at the start of every function (which is a good practice), it is not a big limitation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fully agree with you that callers should call functions in a correct way.
I only meant that first and foremost the functions itself should be fail-safe.
Of couse I know that currently most functions in ReaR are not fail-safe.
E.g. a function that requires two arguments should check that it actually got
exactly two valid arguments so that when a caller provides something different
the function could do "the right thing".
What "the right thing" is depends on the particular function.

That bash parameter expansion with using a default value
is OK for our needs is what I meant with my comment

# but using a default value is practicable here because $1 is used only once

that is currently at
https://github.com/rear/rear/blob/jsmeix-fix-issue-2667/usr/share/rear/lib/global-functions.sh#L352

case $scheme in
(null|tape|obdr|rsync|fish|ftp|ftps|hftp|http|https|sftp)
return 1
Expand Down Expand Up @@ -682,7 +698,7 @@ function umount_url() {

RemoveExitTask "perform_umount_url '$url' '$mountpoint' lazy"

remove_temporary_mountpoint '$mountpoint' && RemoveExitTask "remove_temporary_mountpoint '$mountpoint'"
remove_temporary_mountpoint "$mountpoint" && RemoveExitTask "remove_temporary_mountpoint '$mountpoint'"
return 0
}

Expand Down