Message-ID: <Z36TlACdNMwFD7wv@dev-ushankar.dev.purestorage.com>
Date: Wed, 8 Jan 2025 08:02:44 -0700
From: Uday Shankar <ushankar@...estorage.com>
To: Breno Leitao <leitao@...ian.org>
Cc: "David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH] netconsole: allow selection of egress interface via MAC
 address

On Fri, Jan 03, 2025 at 03:41:17AM -0800, Breno Leitao wrote:
> > For these reasons, allow selection of the egress interface via MAC
> > address. To maintain parity between interfaces, the local_mac entry in
> > configfs is also made read-write and can be used to select the local
> > interface, though this use case is less interesting than the one
> > highlighted above.
> 
> This will change slightly local_mac meaning. At the same time, I am not
> sure local_mac is a very useful field as-is. The configuration might be
> a bit confusing using `local_mac` to define the target interface. I am
> wondering if creating a new field might be more appropriate. Maybe
> `dev_mac`? (I am not super confident this approach is better TBH, but, it
> seems easier to reason about).

Do you mean creating a new field called dev_mac which replaces
local_mac? I do agree that naming is a bit better but I'd be worried
about breaking programs which expect local_mac to exist. Having the
field go read-only --> read-write via this change feels a lot less
disruptive to preexisting programs than renaming the field.

Or do you mean creating a new field dev_mac which will live alongside
local_mac, and letting local_mac keep its existing semantics? It feels
like that would lead to messier code, since dev_mac's semantics are kind
of a superset of local_mac's semantics (e.g. after selecting and
enabling a netconsole via dev_name, local_mac is populated with the mac
address of the interface and we'd probably want the same for dev_mac as
well).

A third option would be dropping the configfs changes altogether, which
I'd be okay with - as I highlighted in the commit message, I suspect
this interface is far less likely to see real use than the command-line
parameter. A downside of this option though is that automated testing
becomes difficult, as we can't write a variant of netcons_basic.sh
without configfs support. We'd have to have a test which uses the
parameter directly, and I'm not sure if we have a testing framework for
the kernel which would support that.

Let me know which option you think is best, and I'll move forward with
it in v2.

> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 4ea44a2f48f7..865c43a97f70 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> 
> > @@ -211,6 +211,8 @@ static struct netconsole_target *alloc_and_init(void)
> > +	/* the "don't use" or N/A value for this field */
> 
> This comment is not very clear. What do you mean exactly?

I wanted to maintain the invariant that when setting up a netconsole, at
most one of dev_name and local_mac is set to a meaningful value, as
otherwise we'd need to implement and document some sort of priority
system when it comes to selecting the local interface. This invariant
requires having a designated "invalid" value for each field - it's the
empty string for dev_name and the broadcast mac for local_mac (for
backwards compatibility purposes, see below).

> 
> > +	eth_broadcast_addr(nt->np.local_mac);
> 
> Why not just memzeroing the memory?

That could work, but we kind of had an unwritten rule that the broadcast
address was the invalid value for local_mac in the code before. For
example, when creating a brand new netconsole via configfs:

# cd /sys/kernel/config/netconsole/
# mkdir test
# cat test/local_mac
ff:ff:ff:ff:ff:ff

So I stuck with the broadcast mac address for the local_mac "invalid"
value.

ACK on the rest of the comments, I will address them in v2 once we have
clarity on the above issue.

Powered by blists - more mailing lists