Hi,
While I was working on adding support for MRA role to MRP, I noticed that I
might have some issues with the netlink interface, so it would be great if you
can give me an advice on how to continue.
First a node with MRA role can behave as a MRM(Manager) or as a
MRC(Client). The behaviour is decided by the priority of each node. So
to have this functionality I have to extend the MRP netlink interface
and this brings me to my issues.
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?
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.
If it is not possible to break the compatibility then the safest way is to
just add more attributes under IFLA_BRIDGE_MRP but this would just complicate
the kernel and the userspace and it would make it much harder to be extended in
the future.
My personal choice would be the second approach, even if it breaks the backwards
compatibility. Because it is the easier to go forward and there are only 3
people who cloned the userspace application
(https://github.com/microchip-ung/mrp/graphs/traffic). And two of
these unique cloners is me and Allan.
So if you have any advice on how to go forward it would be great.
--
/Horatiu
On 25/05/2020 14:28, Horatiu Vultur wrote:
> Hi,
>
> While I was working on adding support for MRA role to MRP, I noticed that I
> might have some issues with the netlink interface, so it would be great if you
> can give me an advice on how to continue.
>
> First a node with MRA role can behave as a MRM(Manager) or as a
> MRC(Client). The behaviour is decided by the priority of each node. So
> to have this functionality I have to extend the MRP netlink interface
> and this brings me to my issues.
>
> 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?
>
> 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.
>
> If it is not possible to break the compatibility then the safest way is to
> just add more attributes under IFLA_BRIDGE_MRP but this would just complicate
> the kernel and the userspace and it would make it much harder to be extended in
> the future.
>
> My personal choice would be the second approach, even if it breaks the backwards
> compatibility. Because it is the easier to go forward and there are only 3
> people who cloned the userspace application
> (https://github.com/microchip-ung/mrp/graphs/traffic). And two of
> these unique cloners is me and Allan.
>
> So if you have any advice on how to go forward it would be great.
>
IIRC this is still in net-next only, right? If so - now would be the time to change it.
Once it goes into a release, we'll be stuck with workarounds. So I'd go for solution 2).
I haven't cloned it, but I do sync your user-space mrp repo to check against the patches. :)
The 05/25/2020 12:33, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 25/05/2020 14:28, Horatiu Vultur wrote:
> > Hi,
> >
> > While I was working on adding support for MRA role to MRP, I noticed that I
> > might have some issues with the netlink interface, so it would be great if you
> > can give me an advice on how to continue.
> >
> > First a node with MRA role can behave as a MRM(Manager) or as a
> > MRC(Client). The behaviour is decided by the priority of each node. So
> > to have this functionality I have to extend the MRP netlink interface
> > and this brings me to my issues.
> >
> > 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?
> >
> > 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.
> >
> > If it is not possible to break the compatibility then the safest way is to
> > just add more attributes under IFLA_BRIDGE_MRP but this would just complicate
> > the kernel and the userspace and it would make it much harder to be extended in
> > the future.
> >
> > My personal choice would be the second approach, even if it breaks the backwards
> > compatibility. Because it is the easier to go forward and there are only 3
> > people who cloned the userspace application
> > (https://github.com/microchip-ung/mrp/graphs/traffic). And two of
> > these unique cloners is me and Allan.
> >
> > So if you have any advice on how to go forward it would be great.
> >
>
> IIRC this is still in net-next only, right? If so - now would be the time to change it.
> Once it goes into a release, we'll be stuck with workarounds. So I'd go for solution 2).
Yes, this is only in net-next. Then I should ASAP update this with
solution 2.
>
> I haven't cloned it, but I do sync your user-space mrp repo to check against the patches. :)
>
--
/Horatiu
On 25/05/2020 13:03, Michal Kubecek wrote:
> 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
>
+1, we don't have strict checking for the bridge main af spec attributes, but
you could add that for new nested interfaces that need to be parsed like the
above
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
On Mon, May 25, 2020 at 01:14:35PM +0000, Horatiu Vultur wrote:
> The 05/25/2020 13:26, Nikolay Aleksandrov wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On 25/05/2020 13:03, Michal Kubecek wrote:
> > > 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
>
> Thanks for the detail explanation. Currently this is in net-next so I
> would try to change it.
> Can you point me to some code that is using this strict checking for
> netlink attributes? Just to have a better understanding of it.
AFAICS you are using nla_parse_nested() in br_mrp_parse() so that the
validation should be strict, including rejection of unknown attributes.
See the comments at nla_parse() and nla_parse_deprecated() and
enum netlink_validation in include/net/netlink.h for details.
Michal
> > +1, we don't have strict checking for the bridge main af spec attributes, but
> > you could add that for new nested interfaces that need to be parsed like the
> > above
The 05/25/2020 13:26, Nikolay Aleksandrov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On 25/05/2020 13:03, Michal Kubecek wrote:
> > 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
Thanks for the detail explanation. Currently this is in net-next so I
would try to change it.
Can you point me to some code that is using this strict checking for
netlink attributes? Just to have a better understanding of it.
> >
>
> +1, we don't have strict checking for the bridge main af spec attributes, but
> you could add that for new nested interfaces that need to be parsed like the
> above
>
>
>
>
>
>
>
--
/Horatiu