gre: minor cleanups in netlink interface

Some minor cleanups, there doesn't seem to be a reason for not
using the typeful helpers everywhere.

I noticed the interface expects to always get a full configuration
on change requests. Is there are particular reason for not
supporting incremental changes, lets say

"ip link change dev gre0 type gre remote <new remote>"

? It looks easy enough to change, so I could take care of this.

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 10 Oct 2008 18:04:52 +0200

>     gre: minor cleanups in netlink interface
>     
>     - use typeful helpers for IFLA_GRE_LOCAL/IFLA_GRE_REMOTE
>     - replace magic value by FIELD_SIZEOF
>     - use MODULE_ALIAS_RTNL_LINK macro
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks Patrick.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 10, 2008 at 06:04:52PM +0200, Patrick McHardy wrote:
> Some minor cleanups, there doesn't seem to be a reason for not
> using the typeful helpers everywhere.

The reason for that was future IPv6 support.  But I suppose having
it as a be32 for now doesn't prevent us from changing it later, or
does it?

> I noticed the interface expects to always get a full configuration
> on change requests. Is there are particular reason for not
> supporting incremental changes, lets say
>
> "ip link change dev gre0 type gre remote <new remote>"
>
> ? It looks easy enough to change, so I could take care of this.

I think this should be done in iproute.  That way the user (or
rather the user-space programmer) gets to choose the behaviour.
This is also how the existing interface works too.

I'll update the iproute change to do this.

Thanks,

Herbert Xu wrote:
> On Fri, Oct 10, 2008 at 06:04:52PM +0200, Patrick McHardy wrote:
>> Some minor cleanups, there doesn't seem to be a reason for not
>> using the typeful helpers everywhere.
> 
> The reason for that was future IPv6 support.  But I suppose having
> it as a be32 for now doesn't prevent us from changing it later, or
> does it?

No, its equivalant to using memcpy.

>> I noticed the interface expects to always get a full configuration
>> on change requests. Is there are particular reason for not
>> supporting incremental changes, lets say
>>
>> "ip link change dev gre0 type gre remote <new remote>"
>>
>> ? It looks easy enough to change, so I could take care of this.
> 
> I think this should be done in iproute.  That way the user (or
> rather the user-space programmer) gets to choose the behaviour.
> This is also how the existing interface works too.

We don't have much precedent for rtnl_link besides VLAN (which
does support incremental changes), but actually all other route
netlink interfaces do support incremental changes by sending only
a subset of the attributes. A reason for supporting this in the
interface is that incremental userspace changes will always be
racy because you need two seperate operations.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patrick McHardy wrote:
> Herbert Xu wrote:
>>> I noticed the interface expects to always get a full configuration
>>> on change requests. Is there are particular reason for not
>>> supporting incremental changes, lets say
>>>
>>> "ip link change dev gre0 type gre remote <new remote>"
>>>
>>> ? It looks easy enough to change, so I could take care of this.
>>
>> I think this should be done in iproute.  That way the user (or
>> rather the user-space programmer) gets to choose the behaviour.
>> This is also how the existing interface works too.
> 
> We don't have much precedent for rtnl_link besides VLAN (which
> does support incremental changes), but actually all other route
> netlink interfaces do support incremental changes by sending only
> a subset of the attributes. A reason for supporting this in the
> interface is that incremental userspace changes will always be
> racy because you need two seperate operations.


Its actually also what defines the difference between using
NLM_F_REPLACE (ip link replace) and no flags (ip link change).
The former replaces the replaces the object, the later changes
an existing object.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Oct 11, 2008 at 04:43:03PM +0200, Patrick McHardy wrote:
>
> No, its equivalant to using memcpy.

Great.

> We don't have much precedent for rtnl_link besides VLAN (which
> does support incremental changes), but actually all other route
> netlink interfaces do support incremental changes by sending only
> a subset of the attributes. A reason for supporting this in the
> interface is that incremental userspace changes will always be
> racy because you need two seperate operations.

It is true that it is going to be racy when done in user-space,
however that's easily solved with locking.  Even if we did the
incremental change in the kernel it only helps certain kinds of
usage scenarios.  For instance, if the race is between two updates
to the local address you're still going to need synchronisation
in user-space.

Having said that, I'm certainly not against changing the interface
since you do have precedence with the other two :)

Do be warned that doing this for GRE is going to be less trivial
than the existing rtnl link interfaces.  For example, we'll need
to break down the iflags/oflags into individual bits as otherwise
you'll be back in the same situation.  It's a good thing that
there aren't too many bits in use :)

Also, for ikey/okey we'll need to introduce another attribute to
indicate their presence as well as their value.

Hmm, it seems that there is a bug in how we treat a zero key.
You can't have a tunnel with a zero key and one with no key at
the same time.

In fact my latest iproute patch has a similar problem.  You
can't unset the ikey/okey except by deleting the tunnel.  On
the other hand the old ip tunnel interface has the same bug :)

Cheers,

Herbert Xu wrote:
> On Sat, Oct 11, 2008 at 04:43:03PM +0200, Patrick McHardy wrote:
>> We don't have much precedent for rtnl_link besides VLAN (which
>> does support incremental changes), but actually all other route
>> netlink interfaces do support incremental changes by sending only
>> a subset of the attributes. A reason for supporting this in the
>> interface is that incremental userspace changes will always be
>> racy because you need two seperate operations.
> 
> It is true that it is going to be racy when done in user-space,
> however that's easily solved with locking.  Even if we did the
> incremental change in the kernel it only helps certain kinds of
> usage scenarios.  For instance, if the race is between two updates
> to the local address you're still going to need synchronisation
> in user-space.

Thats true.

> Having said that, I'm certainly not against changing the interface
> since you do have precedence with the other two :)
> 
> Do be warned that doing this for GRE is going to be less trivial
> than the existing rtnl link interfaces.  For example, we'll need
> to break down the iflags/oflags into individual bits as otherwise
> you'll be back in the same situation.  It's a good thing that
> there aren't too many bits in use :)

We usually use two values (value + mask) for flags.

> Also, for ikey/okey we'll need to introduce another attribute to
> indicate their presence as well as their value.

The flags already indicate whether keys should be used, don't
they? So if you want to unset them, you can simply unset the
GRE_KEY flag.

I'll give it a shot and will post a patch, probably tommorrow.

> Hmm, it seems that there is a bug in how we treat a zero key.
> You can't have a tunnel with a zero key and one with no key at
> the same time.
> 
> In fact my latest iproute patch has a similar problem.  You
> can't unset the ikey/okey except by deleting the tunnel.  On
> the other hand the old ip tunnel interface has the same bug :)

I can't find it :)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, Oct 11, 2008 at 04:45:48PM +0200, Patrick McHardy wrote:
>
> Its actually also what defines the difference between using
> NLM_F_REPLACE (ip link replace) and no flags (ip link change).
> The former replaces the replaces the object, the later changes
> an existing object.

That wasn't the case historically.  For routing NLM_F_REPLACE
had a real meaning because you can have routes that differ only
by scope.  So if you considered routes that are otherwise the
same to be duplicate routes, then NLM_F_REPLACE tells you whether
you're allowed to create a new route or you must modify one
of the existing ones.  Of course completely duplicate routes
where everything is equal is not allowed.

Of course NLM_F_REPLACE has since been seconded for other purposes
so I suppose it can mean whatever you want for rtnl link :)

Cheers,

On Sat, Oct 11, 2008 at 05:15:40PM +0200, Patrick McHardy wrote:
>
> We usually use two values (value + mask) for flags.

Aha that should handle it nicely.

>> Also, for ikey/okey we'll need to introduce another attribute to
>> indicate their presence as well as their value.
>
> The flags already indicate whether keys should be used, don't
> they? So if you want to unset them, you can simply unset the
> GRE_KEY flag.

Right if you have the flag then it's not an issue.

> I'll give it a shot and will post a patch, probably tommorrow.

Thanks!

>> Hmm, it seems that there is a bug in how we treat a zero key.
>> You can't have a tunnel with a zero key and one with no key at
>> the same time.
>>
>> In fact my latest iproute patch has a similar problem.  You
>> can't unset the ikey/okey except by deleting the tunnel.  On
>> the other hand the old ip tunnel interface has the same bug :)
>
> I can't find it :)

If you mean the patch, then you're on the cc list :)

If you mean the bug in ip tunnel, I was going by the fact that
there is nothing that unsets the GRE_KEY flag.  Actually let's
not worry about this one too much because you can't unset the
checksum flags either and nobody has complained about either
of these problems for a decade.

Cheers,

Herbert Xu wrote:
> On Sat, Oct 11, 2008 at 04:45:48PM +0200, Patrick McHardy wrote:
>> Its actually also what defines the difference between using
>> NLM_F_REPLACE (ip link replace) and no flags (ip link change).
>> The former replaces the replaces the object, the later changes
>> an existing object.
> 
> That wasn't the case historically.  For routing NLM_F_REPLACE
> had a real meaning because you can have routes that differ only
> by scope.  So if you considered routes that are otherwise the
> same to be duplicate routes, then NLM_F_REPLACE tells you whether
> you're allowed to create a new route or you must modify one
> of the existing ones.  Of course completely duplicate routes
> where everything is equal is not allowed.

Interesting, I wasn't aware of that.

> Of course NLM_F_REPLACE has since been seconded for other purposes
> so I suppose it can mean whatever you want for rtnl link :)

It actually not supported currently because replacement of links
(while keeping routes etc.) would be highly complicated.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html