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

gpg: set default in function to allow global patch #543

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Mar 21, 2023

Patching globals to modify function behavior is an anti-pattern and should not be recommended to a user!!

But it seems acceptable in tests (see #542). For this to work, the global must not be used before being modified, which, AFAIK, is not possible, if the global is defined and used (e.g. in a function signature) in the same scope.

This commit changes the relevant function to access the default value in local scope, so that it global can be imported and modified before it.

More invasive but probably more correct solutions would be:

  • move either GPG_TIMEOUT or is_available_gnupg to a different module, or
  • call relevant functions in tests with an explicit timeout arg

Patching globals to modify function behavior is an anti-pattern and
should not be recommended to a user!!

But it seems acceptable in tests (see secure-systems-lab#542). For this to work, the
global must not be used before being modified, which, AFAIK, is not
possible, if the global is defined and used (e.g. in a function
signature) in the same scope.

This commit changes the relevant function to access the default
value in local scope, so that it global can be imported and
modified before it.

More invasive but probably more correct solutions would be:
- move either GPG_TIMEOUT or is_available_gnupg to a different
  module, or
- call relevant functions in tests with an explicit timeout arg

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

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

This seems correct even if the other improvements are later planned

@lukpueh lukpueh requested a review from jku March 21, 2023 11:38
@lukpueh lukpueh merged commit 4da1e0e into secure-systems-lab:main Mar 21, 2023
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