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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87y1e646cd.fsf@nvidia.com>
Date: Thu, 7 Dec 2023 11:34:32 +0100
From: Petr Machata <petrm@...dia.com>
To: Paolo Abeni <pabeni@...hat.com>
CC: Petr Machata <petrm@...dia.com>, Hangbin Liu <liuhangbin@...il.com>,
	<netdev@...r.kernel.org>, "David S.  Miller" <davem@...emloft.net>, "Jakub
 Kicinski" <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, "Shuah Khan"
	<shuah@...nel.org>, David Ahern <dsahern@...nel.org>,
	<linux-kselftest@...r.kernel.org>, Po-Hsu Lin <po-hsu.lin@...onical.com>,
	Guillaume Nault <gnault@...hat.com>, James Prestwood <prestwoj@...il.com>,
	Jaehee Park <jhpark1013@...il.com>, Ido Schimmel <idosch@...dia.com>, Justin
 Iurman <justin.iurman@...ege.be>, Xin Long <lucien.xin@...il.com>, James
 Chapman <jchapman@...alix.com>
Subject: Re: [PATCHv3 net-next 01/14] selftests/net: add lib.sh


Paolo Abeni <pabeni@...hat.com> writes:

> On Wed, 2023-12-06 at 13:32 +0100, Petr Machata wrote:
>> Paolo Abeni <pabeni@...hat.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