Re: [patch] Problem with two NIC on same NET (in_scrubprefix: err=17, new prefix add failed)

Hi,

  thank you very much for your review and related fixes.

On Fri, Aug 12, 2011 at 3:48 AM, Li, Qing <qing.li_at_bluecoat.com> wrote:
>>  I have no problem with loopback routes when I work with not
>> point-to-point interfaces as I can NOT set same source address on
>> them. However, if the interface is going down and up, then loopback
>> route is deleted without checking IFA_RTSELF flag (it must be
>> consistent! especially in kernel) and re-added regardless of
>> "useloopback" setting. So, at least, a loopback route is installed
>> even if useloopback is NOT allowed!
>>
>
>   I hope the question does not offend you, but you do know the history
>   behind IFA_RTSELF loopback route for each interface address, right ?
>
>   The interface address loopback route is used for reaching the
>   interface address within the system after the L2/L3 separation
>   redesign, that's why "useloopback" setting is inapplicable.
>
>   The check in various code paths may have a bit of consistency issue,
>   but "useloopback" setting does not apply here.

In fact, I don't know the history, so you question is quite in place.
I always try to find more about problems that I'm solving. However,
sometimes it is not easy to find all things about an issue. Sometimes,
I miss the ideas behind to figure out the issue in clear view. That's
why I've started this discussion. So, maybe I'm not perfect, but I'm
trying to learn. ;))

>> The bigger problem was with loopback routes on un-numbered
>> interfaces. In in_ifinit(), when un-numbered interface is setting
>> loopback route, then refcount on existing route is incremented and
>> IFA_RTSELF flags is set on its address. This is done if and only if
>> useloopback is set and interface is not IFF_LOOPBACK. It is OK. The
>> rest is hacked somehow and I don't know why.
>>
>
>   The loopback route for the IFA should be installed unconditionally.
>   So the check in in_ifinit() for "V_useloopback" needs to be removed.

It is clear now, but I don't know that when I was proposing the patch.
I saw "V_useloopback" in in_ifinit(), so I put it to my patch too.

>>> Unless you have a really good reason, other than code inspection,
>>> and have a set of test cases, please leave this code alone for now.
>>
>> I have good reason, but I can hack kernel just for me only in worse
>> scenario. However, I always try to minimalize the hacks count.
>>
>
>   You can hack the kernel however you see fit, but when you are
>   ready for a patch commit, please provide sufficient context and
>   problem description, and test cases whenever possible to make the
>   code review process effective.

I understand.

>   Again, my only point is, since these areas are core to the networking
>   kernel, please test as many scenarios as possible, more than just your
>   specific setup. (I made this mistake myself sometimes.)

I undestand and once again,  thanks,

     Svata