Re: rtfree(9) and nd6_prefix_{on,off}link
On 17/08/15(Mon) 19:24, Alexander Bluhm wrote:
On Mon, Aug 17, 2015 at 07:03:55PM +0200, Martin Pieuchot wrote:
On 17/08/15(Mon) 18:25, Alexander Bluhm wrote:
On Mon, Aug 17, 2015 at 12:34:13PM +0200, Martin Pieuchot wrote:
Ultimately my goal is to use rt_ifa_{add,del}() instead of
nd6_prefix_{on,off}link() but right now I need to remove the
rt-ref_cnt--.
@@ -1861,26 +1856,11 @@ nd6_prefix_onlink(struct nd_prefix *pr)
info.rti_info[RTAX_NETMASK] = sin6tosa(mask6);
error = rtrequest1(RTM_ADD, info, RTP_CONNECTED, rt,
ifp-if_rdomain);
- if (error == 0) {
- if (rt != NULL) /* this should be non NULL, though */
- rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
+ if (error == 0 rt != NULL) {
pr-ndpr_stateflags |= NDPRF_ONLINK;
Here you change the check for setting ndpr_stateflags from (error
== 0) to (error == 0 rt != NULL). Although I think that both
checks have the same result, would it be better to use the same
logic as in defrouter_addreq()?
I'm changing the logic to match what's done in rt_ifa_add() but if you
look at rtrequest1(RTM_ADD, ...) you'll see that rt cannot be NULL when
error is 0.
I think it is be better to have the same check everywhere and I don't
care if it is (error != 0), (rt == NULL) or both.
I also want to have the same logic everywhere, it is quite inconsistent
now.
I think the best would be to check only for (error == 0). Then it
is clear that rtrequest1() guarantees rt != NULL. The man page
rtrequest1(9) should mention this.
If we agree upon this, I can make a diff to unify the caller.
I agree on this, I'd love to see a diff to unify this behavior.
Re: rtfree(9) and nd6_prefix_{on,off}link
On Mon, Aug 17, 2015 at 12:34:13PM +0200, Martin Pieuchot wrote:
Ultimately my goal is to use rt_ifa_{add,del}() instead of
nd6_prefix_{on,off}link() but right now I need to remove the
rt-ref_cnt--.
This diff does that and reduce the differences between these functions.
Note that nd6_prefix_onlink()'s error are always logged so I doubt we
need two messages.
Ok?
We, mpi@ and bluhm@, both agree that checking error == 0 and rt !=
NULL after rtrequest1() is equivalent. We can figure out the best
coding style later.
OK bluhm@
Index: netinet6/nd6.h
===
RCS file: /cvs/src/sys/netinet6/nd6.h,v
retrieving revision 1.44
diff -u -p -r1.44 nd6.h
--- netinet6/nd6.h18 Jul 2015 15:05:32 - 1.44
+++ netinet6/nd6.h17 Aug 2015 10:04:04 -
@@ -309,8 +309,6 @@ void prelist_remove(struct nd_prefix *);
int prelist_update(struct nd_prefix *, struct nd_defrouter *, struct mbuf *);
int nd6_prelist_add(struct nd_prefix *, struct nd_defrouter *,
struct nd_prefix **);
-int nd6_prefix_onlink(struct nd_prefix *);
-int nd6_prefix_offlink(struct nd_prefix *);
void pfxlist_onlink_check(void);
struct nd_defrouter *defrouter_lookup(struct in6_addr *, struct ifnet *);
Index: netinet6/nd6_rtr.c
===
RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
retrieving revision 1.113
diff -u -p -r1.113 nd6_rtr.c
--- netinet6/nd6_rtr.c18 Jul 2015 15:51:17 - 1.113
+++ netinet6/nd6_rtr.c17 Aug 2015 10:04:05 -
@@ -69,7 +69,8 @@ void pfxrtr_del(struct nd_pfxrouter *);
struct nd_pfxrouter *find_pfxlist_reachable_router(struct nd_prefix *);
void defrouter_delreq(struct nd_defrouter *);
void purge_detached(struct ifnet *);
-
+int nd6_prefix_onlink(struct nd_prefix *);
+int nd6_prefix_offlink(struct nd_prefix *);
void in6_init_address_ltimes(struct nd_prefix *, struct in6_addrlifetime *);
int rt6_deleteroute(struct rtentry *, void *, unsigned int);
@@ -601,7 +602,7 @@ defrouter_addreq(struct nd_defrouter *ne
{
struct rt_addrinfo info;
struct sockaddr_in6 def, mask, gate;
- struct rtentry *newrt = NULL;
+ struct rtentry *rt = NULL;
int s;
int error;
@@ -622,11 +623,11 @@ defrouter_addreq(struct nd_defrouter *ne
info.rti_info[RTAX_NETMASK] = sin6tosa(mask);
s = splsoftnet();
- error = rtrequest1(RTM_ADD, info, RTP_DEFAULT, newrt,
+ error = rtrequest1(RTM_ADD, info, RTP_DEFAULT, rt,
new-ifp-if_rdomain);
- if (newrt) {
- rt_sendmsg(newrt, RTM_ADD, new-ifp-if_rdomain);
- newrt-rt_refcnt--;
+ if (rt) {
+ rt_sendmsg(rt, RTM_ADD, new-ifp-if_rdomain);
+ rtfree(rt);
}
if (error == 0)
new-installed = 1;
@@ -1783,14 +1784,8 @@ nd6_prefix_onlink(struct nd_prefix *pr)
char addr[INET6_ADDRSTRLEN];
/* sanity check */
- if ((pr-ndpr_stateflags NDPRF_ONLINK) != 0) {
- nd6log((LOG_ERR,
- nd6_prefix_onlink: %s/%d is already on-link\n,
- inet_ntop(AF_INET6, pr-ndpr_prefix.sin6_addr,
- addr, sizeof(addr)),
- pr-ndpr_plen));
+ if ((pr-ndpr_stateflags NDPRF_ONLINK) != 0)
return (EEXIST);
- }
/*
* Add the interface route associated with the prefix. Before
@@ -1861,26 +1856,11 @@ nd6_prefix_onlink(struct nd_prefix *pr)
info.rti_info[RTAX_NETMASK] = sin6tosa(mask6);
error = rtrequest1(RTM_ADD, info, RTP_CONNECTED, rt, ifp-if_rdomain);
- if (error == 0) {
- if (rt != NULL) /* this should be non NULL, though */
- rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
+ if (error == 0 rt != NULL) {
pr-ndpr_stateflags |= NDPRF_ONLINK;
- } else {
- char gw[INET6_ADDRSTRLEN], mask[INET6_ADDRSTRLEN];
- nd6log((LOG_ERR, nd6_prefix_onlink: failed to add route for a
- prefix (%s/%d) on %s, gw=%s, mask=%s, flags=%lx
- errno = %d\n,
- inet_ntop(AF_INET6, pr-ndpr_prefix.sin6_addr,
- addr, sizeof(addr)),
- pr-ndpr_plen, ifp-if_xname,
- inet_ntop(AF_INET6, satosin6(ifa-ifa_addr)-sin6_addr,
- gw, sizeof(gw)),
- inet_ntop(AF_INET6, mask6.sin6_addr, mask, sizeof(mask)),
- rtflags, error));
+ rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
+ rtfree(rt);
}
-
- if (rt != NULL)
- rt-rt_refcnt--;
return (error);
}
Re: rtfree(9) and nd6_prefix_{on,off}link
On Mon, Aug 17, 2015 at 12:34:13PM +0200, Martin Pieuchot wrote:
Ultimately my goal is to use rt_ifa_{add,del}() instead of
nd6_prefix_{on,off}link() but right now I need to remove the
rt-ref_cnt--.
@@ -1861,26 +1856,11 @@ nd6_prefix_onlink(struct nd_prefix *pr)
info.rti_info[RTAX_NETMASK] = sin6tosa(mask6);
error = rtrequest1(RTM_ADD, info, RTP_CONNECTED, rt, ifp-if_rdomain);
- if (error == 0) {
- if (rt != NULL) /* this should be non NULL, though */
- rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
+ if (error == 0 rt != NULL) {
pr-ndpr_stateflags |= NDPRF_ONLINK;
Here you change the check for setting ndpr_stateflags from (error
== 0) to (error == 0 rt != NULL). Although I think that both
checks have the same result, would it be better to use the same
logic as in defrouter_addreq()?
if (rt) {
rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
rtfree(rt);
}
if (error == 0)
pr-ndpr_stateflags |= NDPRF_ONLINK;
bluhm
Re: rtfree(9) and nd6_prefix_{on,off}link
On 17/08/15(Mon) 18:25, Alexander Bluhm wrote:
On Mon, Aug 17, 2015 at 12:34:13PM +0200, Martin Pieuchot wrote:
Ultimately my goal is to use rt_ifa_{add,del}() instead of
nd6_prefix_{on,off}link() but right now I need to remove the
rt-ref_cnt--.
@@ -1861,26 +1856,11 @@ nd6_prefix_onlink(struct nd_prefix *pr)
info.rti_info[RTAX_NETMASK] = sin6tosa(mask6);
error = rtrequest1(RTM_ADD, info, RTP_CONNECTED, rt, ifp-if_rdomain);
- if (error == 0) {
- if (rt != NULL) /* this should be non NULL, though */
- rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
+ if (error == 0 rt != NULL) {
pr-ndpr_stateflags |= NDPRF_ONLINK;
Here you change the check for setting ndpr_stateflags from (error
== 0) to (error == 0 rt != NULL). Although I think that both
checks have the same result, would it be better to use the same
logic as in defrouter_addreq()?
I'm changing the logic to match what's done in rt_ifa_add() but if you
look at rtrequest1(RTM_ADD, ...) you'll see that rt cannot be NULL when
error is 0.
I think it is be better to have the same check everywhere and I don't
care if it is (error != 0), (rt == NULL) or both.
Re: rtfree(9) and nd6_prefix_{on,off}link
On Mon, Aug 17, 2015 at 07:03:55PM +0200, Martin Pieuchot wrote:
On 17/08/15(Mon) 18:25, Alexander Bluhm wrote:
On Mon, Aug 17, 2015 at 12:34:13PM +0200, Martin Pieuchot wrote:
Ultimately my goal is to use rt_ifa_{add,del}() instead of
nd6_prefix_{on,off}link() but right now I need to remove the
rt-ref_cnt--.
@@ -1861,26 +1856,11 @@ nd6_prefix_onlink(struct nd_prefix *pr)
info.rti_info[RTAX_NETMASK] = sin6tosa(mask6);
error = rtrequest1(RTM_ADD, info, RTP_CONNECTED, rt, ifp-if_rdomain);
- if (error == 0) {
- if (rt != NULL) /* this should be non NULL, though */
- rt_sendmsg(rt, RTM_ADD, ifp-if_rdomain);
+ if (error == 0 rt != NULL) {
pr-ndpr_stateflags |= NDPRF_ONLINK;
Here you change the check for setting ndpr_stateflags from (error
== 0) to (error == 0 rt != NULL). Although I think that both
checks have the same result, would it be better to use the same
logic as in defrouter_addreq()?
I'm changing the logic to match what's done in rt_ifa_add() but if you
look at rtrequest1(RTM_ADD, ...) you'll see that rt cannot be NULL when
error is 0.
I think it is be better to have the same check everywhere and I don't
care if it is (error != 0), (rt == NULL) or both.
I also want to have the same logic everywhere, it is quite inconsistent
now.
I think the best would be to check only for (error == 0). Then it
is clear that rtrequest1() guarantees rt != NULL. The man page
rtrequest1(9) should mention this.
If we agree upon this, I can make a diff to unify the caller.
bluhm
