csirac2 / snazzer

btrfs snapshotting and backup system offering snapshot measurement, transport and pruning.
BSD 2-Clause "Simplified" License
89 stars 9 forks source link

Move SNAZZER_SUBVOLS_EXCLUDE_FILE init code to function #50

Open jamiereid opened 7 years ago

jamiereid commented 7 years ago

Checking for the existance of /etc/snazzer/exclude.patterns is achieved through the use of sudo when snazzer is run as an unprivileged user.

By moving the argument detection to top of script we can avoid situations where root privileges were required for commands that don't need elevated privileges (such as --help).

Resolves: #24

florianjacob commented 7 years ago

Thanks for taking a look at this. :)

jamiereid: Is there a better solution than moving the case check for --help to be above these lines?

What I can think of:

1) One alternative would be to just drop the sudo in $SUDO test -e "$SNAZZER_SUBVOLS_EXCLUDE_FILE" and require the exclude file to be world-readable. 2) Another alternative would be to introduce a new method, something like get_exclude_file, that would combine the effect of the sudo check part that creates a temporary exclude file if needed and glob2grep_file and could be called instead of the EXCL_FILE=$(glob2grep_file "$SNAZZER_SUBVOLS_EXCLUDE_FILE") that is used now.

In case we want to keep the option to make the exclude file only readable for root (@csirac2 ?), I think yours is a simple and working solution for this.

csirac2 commented 7 years ago

Ideally I'd love to have function defs at the top, and entry points/actual code at the bottom - pascal warped my brain forever. It seems we've got things all over the place now :D

The $SUDO test -e seems misplaced - glob2grep can't do anything with that file unless the snazzer user has read permissions without sudo anyway. So we should just remove the $SUDO.

One day we should wrap up the SNAZZER_SUBVOLS_EXCLUDE_FILE initialization code (lines ~28-45) into a function, and then call that just after both places assert_btrfs_tools is called.

But for now @jamiereid do you have time to change your PR to just drop $SUDO from $SUDO test -e? (Feel free to tackle the other suggestion to wrap up the init code into a function too while you're in there :)