error out instead of bugerror for not implemented URI schemes #932

jsmeix · 2016-07-20T09:06:54Z

see #931
and
some other fixes:

first steps to be prepared for 'set -e' which means
replaced COMMAND ; StopIfError
with COMMAND || Error
in output/PXE/default/82_copy_to_net.sh
error out in case of "Problem transferring..."
for each result file (before it only errors out
in case of "Problem transferring..." for the last
result file because the Errot was after the for loop)

…31) and some other fixes

jsmeix · 2016-07-21T07:25:31Z

After I slept on it I still think this is the right thing to do.
Therefore I merge it.

pcahyna · 2021-05-25T14:01:04Z

usr/share/rear/output/default/95_copy_result_files.sh

    ;;

-    (*) BugError "Support for $scheme is not implemented yet."
+    (*) Error "Invalid scheme '$scheme' in '$OUTPUT_URL'."


Why should be unknown schemes considered invalid? In

rear/usr/share/rear/lib/global-functions.sh

Line 557 in 3a69939

mount_cmd="mount $v -t $(url_scheme $url) -o $options $(url_host $url):$(url_path $url) $mountpoint"

unknown schemes are passed as the -t parameter to mount, so if it supports the scheme/filesystem type, there should be no problem.

Thinking about it more: the code above will work only for filesystems whose mount command line syntax is "NFS-like": host:path. Not many filesystem types will support that. Still, it may be better to leave supported filesystem handling to mount_url(), which errors out properly if the URL can not be mounted, and omit it from here (thanks to

rear/usr/share/rear/lib/global-functions.sh

Line 563 in 3a69939

eval $mount_cmd || Error "Mount command '$mount_cmd' failed."

this Error "Invalid scheme situation should be essentially unreachable).

@pcahyna
I don't know why it errors out there (there is no comment that explains why).

I had only replaced the BugError (i.e. abort when there is a bug in our code)
by Error (i.e. exit when there is an error - usually caused by the user or his environment).

Even if that Error "Invalid scheme ..." cannot be reached
I would like to keep it there because it is the default case handling.

I would prefer to always have default case handling code
to make it clear to others who read the code what the default case handling is.
In general I don't like case statements without default case.

Furthermore even if a default case cannot happen with current code
it may happen with future code so having default case handling
makes the code future proof (i.e. fail safe for future changes elsewhere).

FYI
where we have currently such Error "Invalid scheme ..." code:

# find usr/share/rear -type f | xargs grep -B1 'Invalid scheme ' usr/share/rear/output/default/950_copy_result_files.sh (*) Error "Invalid scheme '$scheme' in '$OUTPUT_URL'." usr/share/rear/output/PXE/default/820_copy_to_net.sh ;; (*) Error "Invalid scheme '$scheme' in '$OUTPUT_URL'." usr/share/rear/verify/YUM/default/050_check_YUM_requirements.sh (*) Error "Invalid scheme '$scheme' in BACKUP_URL '$BACKUP_URL' valid schemes: nfs cifs usb tape file iso sshfs ftpfs" usr/share/rear/prep/NETFS/default/050_check_NETFS_requirements.sh (*) Error "Invalid scheme '$scheme' in BACKUP_URL '$BACKUP_URL' valid schemes: nfs cifs usb tape file iso sshfs ftpfs" usr/share/rear/prep/DUPLICITY/default/210_check_NETFS_URL_requirements.sh (*) Error "Invalid scheme '$scheme' in BACKUP_DUPLICITY_NETFS_URL '$BACKUP_DUPLICITY_NETFS_URL' valid schemes: nfs cifs usb tape file iso sshfs ftpfs"

Even if that Error "Invalid scheme ..." cannot be reached
I would like to keep it there because it is the default case handling.

I would prefer to always have default case handling code
to make it clear to others who read the code what the default case handling is.
In general I don't like case statements without default case.

The problem with this approach is the inconsistency with the default case handling in mount_url(), which handles unknown schemes just fine, if there is support in mount for them with the right syntax.

I understand the need for default case, but I think it should be changed to something like

(*) # mount_url has handled it for us

and continue with the same code as in the nfs branch (actually, one can remove the nfs branch entirely and let the default branch handle it).

Anyway, I am working on a larger cleanup, so I will try to include something similar (as a separate commit, so it can be easily omitted, in case you don't like it).

One more thought

Furthermore even if a default case cannot happen with current code
it may happen with future code so having default case handling
makes the code future proof (i.e. fail safe for future changes elsewhere).

I think that if we determine that the default case is truly intended to be unreachable, the Error should be changed back to BugError since it means that some invariant in the code was violated. If then there is a code change which makes it legitimately happen again, it is the responsibility of the person who introduces the change to update the default branch, because we can't foresee now what the future change will be and how the default branch should look like.

Your default case proposal

(*) # mount_url has handled it for us

is perfectly fine with me.

Also BugError with an actually right bug error message what the bug is
would be perfectly fine with me.

Even only a comment at that case statement why it has no default case code
would be perfectly fine with me.

All I like to have is that others who read that particular code part at any later time
get a clear understanding what that particular code part is about
without the need to analyze some other code places elsewhere
that could be somehow releated to that particular code part.

@jsmeix I implemented this in the latest commit ( 1f8ef63 ) in my branch ( https://github.com/pcahyna/rear/tree/fix-backup-removal-in-exit-task ).

It is commit 696ce19 in PR #2625.

…-updated Fix backup removal in exit task and cleanup handling of URL mountpoints: Cleanup of temporary mount point handling, particularly for output. Unification of mount point umount and cleanup move to the mount_url() and umount_url() functions. Replaced the various "rm -rf" of the mountpoint by "rmdir" which fixes #2611 Added lazy umount in case normal umount does not succeed. If build dir is kept (cf. KEEP_BUILD_DIR), propose a safe way to remove it to the user via "rm -Rf --one-file-system" instead of just "rm -Rf" where the user risks to remove everything below that mountpoint if still mounted. Fixes also some other bugs noted in the process: Filesystem-specific umount command not called 20359a9#r51319634 Unknown schemes considered invalid, see the discussion under #932 Identical scripts under DUPLICITY and YUM replaced by symlinks. Reverted #782 that had reintroduced #465 which got re-reported as #2611 Reverted #578 because it is not clear how .lockfile can exist in the unmounted filesystem, and if it does, it is a bug. Reverted d850c40 because it became meanwhile obsoleted by a8fdc44

error out instead of bugerror for not implemented URI schemes (issue9…

f9833ea

…31) and some other fixes

jsmeix added enhancement Adaptions and new features cleanup labels Jul 20, 2016

jsmeix added this to the Rear v1.19 milestone Jul 20, 2016

jsmeix self-assigned this Jul 20, 2016

jsmeix mentioned this pull request Jul 20, 2016

PXE Support for null is not implemented yet #931

Closed

jsmeix merged commit 124bc16 into rear:master Jul 21, 2016

jsmeix added the fixed / solved / done label Jul 21, 2016

jsmeix deleted the better_bugerror_message_for_not_implemented_URI_scheme_issue931 branch July 21, 2016 14:21

pcahyna reviewed May 25, 2021

View reviewed changes

pcahyna mentioned this pull request Jun 4, 2021

Fix backup removal in exit task and cleanup handling of URL mountpoints #2625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

error out instead of bugerror for not implemented URI schemes #932

error out instead of bugerror for not implemented URI schemes #932

Uh oh!

jsmeix commented Jul 20, 2016

Uh oh!

jsmeix commented Jul 21, 2016

Uh oh!

pcahyna May 25, 2021

Uh oh!

pcahyna May 25, 2021

Uh oh!

jsmeix May 26, 2021

Uh oh!

pcahyna May 26, 2021

Uh oh!

pcahyna May 26, 2021

Uh oh!

pcahyna May 26, 2021

Uh oh!

jsmeix May 26, 2021

Uh oh!

pcahyna May 31, 2021

Uh oh!

pcahyna Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

error out instead of bugerror for not implemented URI schemes #932

error out instead of bugerror for not implemented URI schemes #932

Uh oh!

Conversation

jsmeix commented Jul 20, 2016

Uh oh!

jsmeix commented Jul 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants