netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Kubecek <mkubecek@suse.cz>
To: netdev@vger.kernel.org
Cc: Horatiu Vultur <horatiu.vultur@microchip.com>,
	nikolay@cumulusnetworks.com, roopa@cumulusnetworks.com,
	davem@davemloft.net, kuba@kernel.org, andrew@lunn.ch,
	UNGLinuxDriver@microchip.com, bridge@lists.linux-foundation.org,
	linux-kernel@vger.kernel.org
Subject: Re: MRP netlink interface
Date: Mon, 25 May 2020 12:03:22 +0200	[thread overview]
Message-ID: <20200525100322.sjlfxhz2ztrfjia7@lion.mk-sys.cz> (raw)
In-Reply-To: <20200525112827.t4nf4lamz6g4g2c5@soft-dev3.localdomain>

On Mon, May 25, 2020 at 11:28:27AM +0000, Horatiu Vultur wrote:
[...]
> My first approach was to extend the 'struct br_mrp_instance' with a field that
> contains the priority of the node. But this breaks the backwards compatibility,
> and then every time when I need to change something, I will break the backwards
> compatibility. Is this a way to go forward?

No, I would rather say it's an example showing why passing data
structures as binary data via netlink is a bad idea. I definitely
wouldn't advice this approach for any new interface. One of the
strengths of netlink is the ability to use structured and extensible
messages.

> Another approach is to restructure MRP netlink interface. What I was thinking to
> keep the current attributes (IFLA_BRIDGE_MRP_INSTANCE,
> IFLA_BRIDGE_MRP_PORT_STATE,...) but they will be nested attributes and each of
> this attribute to contain the fields of the structures they represents.
> For example:
> [IFLA_AF_SPEC] = {
>     [IFLA_BRIDGE_FLAGS]
>     [IFLA_BRIDGE_MRP]
>         [IFLA_BRIDGE_MRP_INSTANCE]
>             [IFLA_BRIDGE_MRP_INSTANCE_RING_ID]
>             [IFLA_BRIDGE_MRP_INSTANCE_P_IFINDEX]
>             [IFLA_BRIDGE_MRP_INSTANCE_S_IFINDEX]
>         [IFLA_BRIDGE_MRP_RING_ROLE]
>             [IFLA_BRIDGE_MRP_RING_ROLE_RING_ID]
>             [IFLA_BRIDGE_MRP_RING_ROLE_ROLE]
>         ...
> }
> And then I can parse each field separately and then fill up the structure
> (br_mrp_instance, br_mrp_port_role, ...) which will be used forward.
> Then when this needs to be extended with the priority it would have the
> following format:
> [IFLA_AF_SPEC] = {
>     [IFLA_BRIDGE_FLAGS]
>     [IFLA_BRIDGE_MRP]
>         [IFLA_BRIDGE_MRP_INSTANCE]
>             [IFLA_BRIDGE_MRP_INSTANCE_RING_ID]
>             [IFLA_BRIDGE_MRP_INSTANCE_P_IFINDEX]
>             [IFLA_BRIDGE_MRP_INSTANCE_S_IFINDEX]
>             [IFLA_BRIDGE_MRP_INSTANCE_PRIO]
>         [IFLA_BRIDGE_MRP_RING_ROLE]
>             [IFLA_BRIDGE_MRP_RING_ROLE_RING_ID]
>             [IFLA_BRIDGE_MRP_RING_ROLE_ROLE]
>         ...
> }
> And also the br_mrp_instance will have a field called prio.
> So now, if the userspace is not updated to have support for setting the prio
> then the kernel will use a default value. Then if the userspace contains a field
> that the kernel doesn't know about, then it would just ignore it.
> So in this way every time when the netlink interface will be extended it would
> be backwards compatible.

Silently ignoring unrecognized attributes in userspace requests is what
most kernel netlink based interfaces have been doing traditionally but
it's not really a good idea. Essentially it ties your hands so that you
can only add new attributes which can be silently ignored without doing
any harm, otherwise you risk that kernel will do something different
than userspace asked and userspace does not even have a way to find out
if the feature is supported or not. (IIRC there are even some places
where ignoring an attribute changes the nature of the request but it is
still ignored by older kernels.)

That's why there have been an effort, mostly by Johannes Berg, to
introduce and promote strict checking for new netlink interfaces and new
attributes in existing netlink attributes. If you don't have strict
checking for unknown attributes enabled yet, there isn't much that can
be done for already released kernels but I would suggest to enable it as
soon as possible.

Michal

  parent reply	other threads:[~2020-05-25 10:03 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-25 11:28 MRP netlink interface Horatiu Vultur
2020-05-25  9:33 ` Nikolay Aleksandrov
2020-05-25 11:48   ` Horatiu Vultur
2020-05-25 10:03 ` Michal Kubecek [this message]
2020-05-25 10:26   ` Nikolay Aleksandrov
2020-05-25 13:14     ` Horatiu Vultur
2020-05-25 13:18       ` Michal Kubecek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200525100322.sjlfxhz2ztrfjia7@lion.mk-sys.cz \
    --to=mkubecek@suse.cz \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=bridge@lists.linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=horatiu.vultur@microchip.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nikolay@cumulusnetworks.com \
    --cc=roopa@cumulusnetworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).