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

Add modules whitelist #160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

c0d3z3r0
Copy link
Contributor

No description provided.

@corna
Copy link
Owner

corna commented Jan 29, 2018

Thanks for this PR. Just a couple of things:

  • this breaks backwards compatibility, as now -k can't be used alone anymore. You can add nargs="?", default="" to parser.add_argument to have
    • a string if the list of the modules has been passed
    • an empty string (the default) if nothing has been passed
    • None if -k has been specified, but no list has been provided
      Now you can just replace if keep_modules == "all": with if keep_modules == None:.
  • I think it's better if we pass a list of strings (or None for "all modules") to the two check_and_remove_modules functions, instead of a comma-separated string. Move and adapt this part before calling check_and_remove_modules
  • break the lines if they are more than 80 characters

@c0d3z3r0
Copy link
Contributor Author

c0d3z3r0 commented Jan 29, 2018

"None if -k has been specified, but no list has been provided"
-> Nope ;-) This is the one thing I needed sooo many times... but that does not work since argparse cannot know if the last argument after -k is -k's or file's value.

I found a way to solve this but that is very ugly.
-k, nargs="?", default="" works for these invocations:
./me_cleaner.py -k touch_fw image.bin
./me_cleaner.py -k -- image.bin
./me_cleaner.py image.bin -k
but not for ./me_cleaner.py -k image.bin for the reason stated above.

I think this is very, very ugly so my idea would be keeping -k as backward compatibly parameter and add two new parameters -W,--module-whitelist and -B, --module-blacklist or maybe -R (remove) and -K (keep). This way one can specify -k to keep all modules or one of -W, -B (-R, -K).
@corna What do you like better?

@c0d3z3r0 c0d3z3r0 force-pushed the for-upstream/modules_whitelist branch 2 times, most recently from 1795758 to d43f0e3 Compare January 29, 2018 17:37
@c0d3z3r0
Copy link
Contributor Author

c0d3z3r0 commented Mar 16, 2018

@corna any progress here? is there anything I can do?

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

2 participants