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

Refactor archivers #3820

Merged
merged 16 commits into from
Dec 15, 2020
Merged

Refactor archivers #3820

merged 16 commits into from
Dec 15, 2020

Conversation

glitsj16
Copy link
Collaborator

Introduce archive-common.inc: common file for all archivers.
For now this WIP simply recreates existing archiver profiles.
It's intention is to:

  • simplify adding new archiver profile(s);
  • review potential inconsistencies in existing archiver profile(s).
    E.g. most have nogroups and have noroot commented, but a few divert from this pattern. I intend to do a follow-up review after extended testing, so for now this is a WIP.

Copy link
Collaborator

@rusty-snake rusty-snake left a comment

Choose a reason for hiding this comment

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

LGTM


# common profile for archiver/compression tools

blacklist ${RUNUSER}/wayland-*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can blacklist ${RUNUSER}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rusty-snake Thanks for having a look at this draft. I'm in the middle of testing all my archivers including their .local overrides. Will add blacklist ${RUNUSER} to the tests but indeed, I agree this shouldn't pose any problems. The plan is to bring existing profiles in an easier-to-change state using archive-common.inc as baseline and do specific changes in seperate PR's once this is merged. Shouldn't take all that long to get the first part finished.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rusty-snake I added blacklist ${RUNUSER} to archiver-common.inc. What's the point in having that in combination with blacklist ${RUNUSER}/wayland-* again? I see such a doubling in several other profiles (curl, dig, drill, file, wget to name a few) but I can't say I actually understand the why. If ${RUNUSER} is blacklisted, shouldn't that imply ${RUNUSER}/wayland-* (and any other subdir for that matter) is blacklisted too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

blacklist ${RUNUSER}/wayland-* is useless with blacklist ${RUNUSER} and can be simplified, that's right.

The profiles are written with this logic:

If not requires(X11) then
    If 'net none' then
        add 'x11 none'
    Else
        add 'blacklist /tmp/.X11-unix'
    Fi
Fi

If 'x11 none' or 'blacklist /tmp/.X11-unix' then
    add 'blacklist ${RUNUSER}/wayland-*'
Fi

If possible_to_add(wruc) then
    add wruc
Fi

If get_used_commands(wruc) == nil then
    replace wruc with 'blacklist ${RUNUSER}'
Fi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rusty-snake Thanks for the explanation! I'll do the needed changes in a minute, before I forget. It sure is a luxury to have you on board :-)

@glitsj16 glitsj16 marked this pull request as ready for review December 15, 2020 13:10
@glitsj16 glitsj16 mentioned this pull request Dec 15, 2020
@glitsj16 glitsj16 merged commit 4a40e2a into netblue30:master Dec 15, 2020
@glitsj16 glitsj16 deleted the refactor-archivers branch December 15, 2020 19:06
@glitsj16 glitsj16 mentioned this pull request Dec 15, 2020
@matu3ba matu3ba mentioned this pull request Oct 7, 2021
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.

2 participants