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.

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

Move SNAZZER_SUBVOLS_EXCLUDE_FILE init code to function #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamiereid
Copy link

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

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: csirac2#24
@florianjacob
Copy link
Collaborator

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
Copy link
Owner

csirac2 commented Dec 21, 2016

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 :)

This allows the code to only be called when needed, instead of every time snazzer
is run.

Also remove `$SUDO` from the `test -e`.

This solution replaces the one in commit b57fa2b after discussions with @csirac2
and @florianjacob

Resolves: csirac2#24
@jamiereid jamiereid changed the title Move argument detection to top of snazzer Move SNAZZER_SUBVOLS_EXCLUDE_FILE init code to function and remove $SUDO from test Dec 21, 2016
@jamiereid jamiereid changed the title Move SNAZZER_SUBVOLS_EXCLUDE_FILE init code to function and remove $SUDO from test Move SNAZZER_SUBVOLS_EXCLUDE_FILE init code to function Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants