[PATCH v2] elf: Fix tst-relro-symbols.py argument passing

[PATCH v2] elf: Fix tst-relro-symbols.py argument passing

[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=[],
+                        help='required symbol names')
     return parser
 
 def main(argv):
-- 
2.34.1

Re: [PATCH v2] elf: Fix tst-relro-symbols.py argument passing

[Florian Weimer]

* 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

Re: [PATCH v2] elf: Fix tst-relro-symbols.py argument passing

[Adhemerval Zanella Netto]

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

Re: [PATCH v2] elf: Fix tst-relro-symbols.py argument passing

[Florian Weimer]

* 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