svn commit: r227791 - head/sys/netinet

Qing Li qingli at freebsd.org
Mon Nov 21 21:36:16 UTC 2011


>
> From my point of view logically speaking, we should first remove route,
> then remove address. Otherwise, for a short time we've got an invalid
> route in table.
>

For a short time you have an invalid address, it is faster to remove the
address from the list to prevent usage, then to flush the route, which would
also trigger L2 table flushes as well.

>
> Q> Putting "in_scrubprefix()" at the top does not gain you anything at
> Q> all, but can
> Q> potentially be problematic if additional tasks are in fact performed
> Q> in "if_ioctl()"
> Q> that may in fact affect the logic in "in_ifinit()".
> Q>
> Q> Case in point, I had to perform additional routing related tasks so I
> Q> re-introduced "nd6_rtrequest()" in r227460.
>
> Pardon, can you please elaborate on this? I don't see any problems that
> I intoroduced, but if ther are any, we can either push in_scrubprefix()
> down the function as it was before, of fix them some other way.
>

The point being, perhaps the address related tasks to be performed in
"if_ioctl()" today is limited, however, we may need to perform additional
housekeeping tasks in "if_ioctl()", and if there is error, we would want to
revert the process.

There are so many different L2 and pseudo interface functions that map
to "if_ioctl()", it is Not safe to ignore its error and not reflect that error
back to the consistency of the routing table and the interface address
list.

Again, removing the associated route at the beginning of in_ifinit() does
not gain much in terms of performance or code cleanness, nor does it
improve the logic, so why limit what we could do in the future in "if_ioctl()"
by making such a change ??

>
> Q> You are not simplifying much logic by removing the error recovery code from
> Q> the return of "if_ioctl()". So why removing the flexibility of what
> Q> "if_ioctl()" is
> Q> intended for as part of the address configuration logic ?
>
> Because in_ifinit() was inconsistent. It tried to recover in case
> of (*ifp->if_ioctl) failure, but did not try to recover in case
> of failure of:
>
> in_addprefix()
> ifa_add_loopback_route()
>

The "inconsistency" is due to the fact failures from these two functions
are not fatal, and does not necessarily affect the actual operations.

In function "in_addprefix()", there are 2 main possible errors. EEXIST is
non fatal, it just means the same prefix route is already covered by
another address, and what is being inserted is just an alias.

The other error is due to RTM_ADD failure, however, since IFA_ROUTE is
not set on the address when failure occurs, no route will ever be directed
to either the interface or that address. Therefore no harm is done.

In the case of function "ifa_add_loopback_route()", again, a failure is just
an indication of an EEXIST.

You may ask why not check for EEXIST before calling these functions, and
I can tell you having worked those code regions for quite some time, the
actual work amounts to the same efforts, but complicates the logic WRT
the callers. It's actually better just let the failure occur.

Unless you have strong reasons otherwise, I'd appreciate you put
back the original logic in function "in_ifinit()".

--Qing


More information about the svn-src-all mailing list