atf_python/vnet: Make it possible to set the FIB of vnet interfaces

[zlei]

tests/atf_python/sys/net/vnet.py

146

The 0 is a perfect valid fib number. Why not supporting setting fib 0 ?

[ngie]

tests/atf_python/sys/net/vnet.py

60

Setting a default here would be wise: that allows you to skip a lot of the baked-in defaults elsewhere in the diff.

Other considerations:

• It might be a really good idea to create a setter to validate the input argument before setting the value as well. typing.assert_type could help enforce the type, unless you want to get fancy and implement an isinstance / ValueError pattern, but you should also ensure that the domain is >= 0, since negative FIB values don't make sense.
• It might be a good idea to use a constant here as well, e.g., NO_FIB.

146

No need for an explicit test, per PEP8.

380–386

General comment: using dict.get with hardcoded defaults sprinkled around instead of using explicit property variables adds a lot of fragile complexity in this module (and other API consumers):

1. If defaults change, you now have to do a sweeping change to update all of the defaults around the module and the consumers.
2. It makes it difficult to impossible for static analyzers like flake8 or ruff to figure out what's going on in the code.

[markj]

tests/atf_python/sys/net/vnet.py

146

Only because that's the default. But yes we could set it unconditionally. The only downside is an extra fork+exec per interface (which isn't free when running tests under QEMU).

[markj]

Make the FIB be a tuple representing the desired FIBs on both sides of the epair.

[zlei]

tests/atf_python/sys/net/vnet.py

383

@ngie Suggested

negative FIB values don't make sense.

Maybe it is possible to return a (-1, -1) , and then iface.setfib check against the negative value ?

[markj]

tests/atf_python/sys/net/vnet.py

383

What's the purpose of checking for negative values here? setfib(1) will fail if given an invalid FIB.

[zlei]

tests/atf_python/sys/net/vnet.py

383

Only because that's the default. But yes we could set it unconditionally. The only downside is an extra fork+exec per interface (which isn't free when running tests under QEMU).

I meant this, to avoid "an extra fork+exec per interface",

def setfib(self, fib: int):
        if fib >= 0:
            cmd = "/sbin/ifconfig {} fib {}".format(self.name, fib)
            self.run_cmd(cmd)

383

What's the purpose of checking for negative values here? setfib(1) will fail if given an invalid FIB.

Or probably a better choice, check the key "fib" exists, and then call iface.setfib().

if "fib" in topo[iface.alias]:
    fib = topo[iface.alias].get("fib")
    iface.setfib(fib[idx])

That avoids "an extra fork+exec per interface", and can also raise errors if bad config is passed in, say a negative FIB.

[markj]

Avoid invoking setfib except when a FIB is actually specified.

[markj]

tests/atf_python/sys/net/vnet.py

383

Thanks, I see. I think that's a good idea.

[zlei]

Looks good to me.

tests/atf_python/sys/net/vnet.py

384

A nit. Given we have checked the existence of the key "fib", I think the default value is no longer needed, but is somewhat misleading.

[markj]

Remove unneeded default.

[ngie]

tests/atf_python/sys/net/vnet.py

384

This is the only item that could stand to be fixed in this diff.
You already determined that "fib" is in topo[iface.alias]. You can just use dict.__getitem__ here :).

431–432

nitpick for another diff/time to avoid the extra indentation (all subsequent lines in the for-loop would need to be dedented one level OFC)..

441

Sentinel values like this should be made into constants for readability and maintainability.

449

Hanging systemwide subprocess calls off this object is an interesting choice...