[v2] elf: Fix tst-relro-symbols.py argument passing
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
Current scheme only consideres the first argument for both --required
and --optional, where the idea is to append a new item.
Checked on x86_64-linux-gnu.
---
elf/tst-relro-symbols.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Comments
* Adhemerval Zanella:
> Current scheme only consideres the first argument for both --required
> and --optional, where the idea is to append a new item.
>
> Checked on x86_64-linux-gnu.
> ---
> elf/tst-relro-symbols.py | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/elf/tst-relro-symbols.py b/elf/tst-relro-symbols.py
> index 368ea3349f..a572a47148 100644
> --- a/elf/tst-relro-symbols.py
> +++ b/elf/tst-relro-symbols.py
> @@ -56,10 +56,10 @@ def get_parser():
> """Return an argument parser for this script."""
> parser = argparse.ArgumentParser(description=__doc__)
> parser.add_argument('object', help='path to object file to check')
> - parser.add_argument('--required', metavar='NAME', default=(),
> - help='required symbol names', nargs='*')
> - parser.add_argument('--optional', metavar='NAME', default=(),
> - help='required symbol names', nargs='*')
> + parser.add_argument('--required', metavar='NAME', action='append', default=[],
> + help='required symbol names')
> + parser.add_argument('--optional', metavar='NAME', action='append', default=[],
Nit: Like length limit exceeded.
> + help='required symbol names')
> return parser
>
> def main(argv):
Despite the use of [] here, there does not seem to be a sharing hazard:
import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--foo', action='append', default=[])
print(parser.parse_args('--foo 1 --foo 2'.split()))
print(parser.parse_args('--foo 3 --foo 4'.split()))
Prints:
Namespace(foo=['1', '2'])
Namespace(foo=['3', '4'])
I would have expected:
Namespace(foo=['1', '2'])
Namespace(foo=['1', '2', '3', '4'])
Either way, given that the parser is only used once, that doesn't really
matter. So we can implement it this way even though the documentation
is ambiguous.
Reviewed-by: Florian Weimer <fweimer@redhat.com>
Thanks,
Florian
On 15/12/22 16:36, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> Current scheme only consideres the first argument for both --required
>> and --optional, where the idea is to append a new item.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>> elf/tst-relro-symbols.py | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/elf/tst-relro-symbols.py b/elf/tst-relro-symbols.py
>> index 368ea3349f..a572a47148 100644
>> --- a/elf/tst-relro-symbols.py
>> +++ b/elf/tst-relro-symbols.py
>> @@ -56,10 +56,10 @@ def get_parser():
>> """Return an argument parser for this script."""
>> parser = argparse.ArgumentParser(description=__doc__)
>> parser.add_argument('object', help='path to object file to check')
>> - parser.add_argument('--required', metavar='NAME', default=(),
>> - help='required symbol names', nargs='*')
>> - parser.add_argument('--optional', metavar='NAME', default=(),
>> - help='required symbol names', nargs='*')
>> + parser.add_argument('--required', metavar='NAME', action='append', default=[],
>> + help='required symbol names')
>> + parser.add_argument('--optional', metavar='NAME', action='append', default=[],
>
> Nit: Like length limit exceeded.
Right, I will fix it.
>
>> + help='required symbol names')
>> return parser
>>
>> def main(argv):
>
> Despite the use of [] here, there does not seem to be a sharing hazard:
>
> import argparse
> parser = argparse.ArgumentParser()
> parser.add_argument('--foo', action='append', default=[])
> print(parser.parse_args('--foo 1 --foo 2'.split()))
> print(parser.parse_args('--foo 3 --foo 4'.split()))
>
> Prints:
>
> Namespace(foo=['1', '2'])
> Namespace(foo=['3', '4'])
>
> I would have expected:
>
> Namespace(foo=['1', '2'])
> Namespace(foo=['1', '2', '3', '4'])
For state tracking you need to pass a namespace, as described by documentation [1].
import argparse
class State:
pass
state = State()
parser = argparse.ArgumentParser()
parser.add_argument('--foo', action='append', default=[])
parser.parse_args('--foo 1 --foo 2'.split(), namespace=state)
print(state.foo)
parser.parse_args('--foo 3 --foo 4'.split(), namespace=state)
print(state.foo)
It prints:
['1', '2']
['1', '2', '3', '4']
>
> Either way, given that the parser is only used once, that doesn't really
> matter. So we can implement it this way even though the documentation
> is ambiguous.
>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
>
> Thanks,> Florian
[1] https://docs.python.org/3/library/argparse.html#namespace
* Adhemerval Zanella Netto:
>>> + help='required symbol names')
>>> return parser
>>>
>>> def main(argv):
>>
>> Despite the use of [] here, there does not seem to be a sharing hazard:
>>
>> import argparse
>> parser = argparse.ArgumentParser()
>> parser.add_argument('--foo', action='append', default=[])
>> print(parser.parse_args('--foo 1 --foo 2'.split()))
>> print(parser.parse_args('--foo 3 --foo 4'.split()))
>>
>> Prints:
>>
>> Namespace(foo=['1', '2'])
>> Namespace(foo=['3', '4'])
>>
>> I would have expected:
>>
>> Namespace(foo=['1', '2'])
>> Namespace(foo=['1', '2', '3', '4'])
>
> For state tracking you need to pass a namespace, as described by
> documentation [1].
>
> import argparse
>
> class State:
> pass
>
> state = State()
> parser = argparse.ArgumentParser()
> parser.add_argument('--foo', action='append', default=[])
> parser.parse_args('--foo 1 --foo 2'.split(), namespace=state)
> print(state.foo)
> parser.parse_args('--foo 3 --foo 4'.split(), namespace=state)
> print(state.foo)
>
> It prints:
>
> ['1', '2']
> ['1', '2', '3', '4']
Oh, I was concerned that the [] would be used directly, and not cloned.
Then you'd get sharing, too. After all, the [] is only evaluated once,
when add_argument is called. I don't see the cloning described in the
manual.
Thanks,
Florian
@@ -56,10 +56,10 @@ def get_parser():
"""Return an argument parser for this script."""
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument('object', help='path to object file to check')
- parser.add_argument('--required', metavar='NAME', default=(),
- help='required symbol names', nargs='*')
- parser.add_argument('--optional', metavar='NAME', default=(),
- help='required symbol names', nargs='*')
+ parser.add_argument('--required', metavar='NAME', action='append', default=[],
+ help='required symbol names')
+ parser.add_argument('--optional', metavar='NAME', action='append', default=[],
+ help='required symbol names')
return parser
def main(argv):