Re: [PATCH v2 2/2] ipv6 addrconf: don't cleanup route prefix for IFA_F_NOPREFIXROUTE

[Thomas Haller <thaller@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

On Tue, 2014-01-07 at 20:01 +0100, Hannes Frederic Sowa wrote:
> On Tue, Jan 07, 2014 at 07:32:57PM +0100, Thomas Haller wrote:
> > On Tue, 2014-01-07 at 17:28 +0100, Hannes Frederic Sowa wrote:
> > > On Tue, Jan 07, 2014 at 03:39:13PM +0100, Thomas Haller wrote:
> > > > Also, when adding the NOPREFIXROUTE flag to an already existing address,
> > > > check if there there is a prefix that was likly added by the kernel
> > > > and delete it.
> > > 
> > > Hmm, could you give a bit more details why you have done this? I find
> > > that a bit counterintuitive. Maybe it has a reason?
> > > 
> > 
> > You find the behavior or the commit message counterintuitive? Didn't you
> > suggest this behavior in your email from "7 Jan 2014 13:01:11 +0100"?
> 
> I guess I was a bit confused, sorry. I think I confused the deleted and modify
> case. However:
> 
> So we have the following changes on addresses:
> 
> add is simple: just as in the first patch
> 
> modify: is a bit hairy. To be extremly exact, we would have to recreate the
> 	route with proper metrics etc. so delete in any case and reinsert.
> 	I really dislike removing a route someone else might have inserted
> 	manually, and this is a likely scenario.
> 
> 	Somehow I tend to just don't allow NOPREFIXROUTE on modify at all and
> 	just return a proper error value. What do you think? What would be the
> 	best behavior for NM?
> 
> delete: if IFA_F_NOPREFIXROUTE is set, we don't care about removing a prefix
> 	route, it must be set by user space and should get cleaned up by user
> 	space
> 
> > 
> > 
> > For v3 I will reword the commit message. How about the following:
> > ...
> 
> If we want go with the current modify behavior this sounds good.


Hi,


I think, the modify case is not that hairy and the patch does IMO the
sensible thing:

case 1) "change NOPREFIXROUTE -> !NOPREFIXROUTE":
    update or add prefix route (as before);;
case 2) "change !NOPREFIXROUTE -> !NOPREFIXROUTE":
    update or add prefix route (as before);;
case 3) "change NOPREFIXROUTE -> NOPREFIXROUTE":
    ;;
case 4) "change !NOPREFIXROUTE -> NOPREFIXROUTE":
    cleanup prefix route;;

where "cleanup" means the same as done in ipv6_del_addr(), as determined
by check_cleanup_prefix_routes().


Allowing modify with case 2) and 3) is important. But for case 4) (and
possibly 1)), we could also fail with error. I tend to the scheme above
though because it makes it easier for userspace and is likely what it
wants.



The problem of deleting a route created by somebody else is already
present without this patch in ipv6_del_addr. This is indeed a bit shaky,
but I guess it's good enough in practice. Do I understand correctly,
that you think about to use the information from ifp->rt to ensure, that
what we really cleanup the correct route? If that's what you intend, can
you elaborate a bit on how to do that?


ciao,
Thomas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]