Re: [PATCHv3 net-next 01/14] selftests/net: add lib.sh

[Petr Machata <petrm@nvidia.com>]

Paolo Abeni <pabeni@redhat.com> writes:

> On Wed, 2023-12-06 at 13:32 +0100, Petr Machata wrote:
>> Paolo Abeni <pabeni@redhat.com> writes:
>> 
>> > Side note for a possible follow-up: if you maintain $ns_list as global
>> > variable, and remove from such list the ns deleted by cleanup_ns, you
>> > could remove the cleanup trap from the individual test with something
>> > alike:
>> > 
>> > final_cleanup_ns()
>> > {
>> > 	cleanup_ns $ns_list
>> > }
>> > 
>> > trap final_cleanup_ns EXIT
>> > 
>> > No respin needed for the above, could be a follow-up if agreed upon.
>> 
>> If you propose this for the library then I'm against it. The exit trap
>> is a global resource that the client scripts sometimes need to use as
>> well, to do topology teardowns or just general cleanups. 
>> So either the library would have to provide APIs for cleanup management, or the trap
>> is for exclusive use by clients. The latter is IMHO simpler.
>
> Even the former would not be very complex:
>
> TRAPS=""
> do_at_exit() {
>         TRAPS="${TRAPS}$@;"
>
>         trap "${TRAPS}" EXIT
> }
>
> And then use "do_at_exit <whatever>" instead of "trap <whatever> EXIT"

Yep. I mentioned this during v2 review:

    https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L13

Not much code at all, though you need to convert all EXIT trap users to
this contraption. Again, a mechanical process, just needs to be done.

>> It also puts the cleanups at the same place where the acquisition is
>> prompted: the client allocates the NS, the client should prompt its
>> cleanup.
>
> I guess I could argue that the the script is asking the library to
> allocate the namespaces, and the library could take care of disposing
> them.

It could also be said that since the script asked for NS creation, the
script should ask for NS disposal :)

But what I object against is that the library uses trap without having a
way for user scripts to schedule at-exit work, because that's used
literally everywhere in forwarding tests. If people are willing to do
the conversion, I'm OK with that.

> But I'm not pushing the proposed option, if there is no agreement no
> need for additional work ;)
>
> Cheers,
>
> Paolo