On 2012/07/21 21:52, Mike Belopuhov wrote:
> On Sat, Jul 21, 2012 at 19:25 +0100, Stuart Henderson wrote:
> > On 2012/07/21 18:49, Mike Belopuhov wrote:
> > > this sppp_clear_ip_addrs_args dance looks totally unneeded if i read
> > > the diff correctly. why don't you just pass "sp" as an argument to
> > > the workq_add_task?
> >
> > yes, that makes sense. I think he did it that way because it was
> > modelled on sppp_set_ip_addrs which needed to pass in two addresses
> > as well as the pointer.
> >
>
> i see. we can change that other call to workq_add_task to take "sp"
> and "args" parameters because workq callback takes two and currently
> the second one is ignored.
makes sense, but I think that one can wait until after unlock.
> > Index: if_spppsubr.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if_spppsubr.c,v
> > retrieving revision 1.96
> > diff -u -p -r1.96 if_spppsubr.c
> > --- if_spppsubr.c 28 Jan 2012 12:14:45 -0000 1.96
> > +++ if_spppsubr.c 21 Jul 2012 18:24:59 -0000
> > @@ -398,7 +398,7 @@ HIDE void sppp_qflush(struct ifqueue *if
> > int sppp_update_gw_walker(struct radix_node *rn, void *arg, u_int);
> > void sppp_update_gw(struct ifnet *ifp);
> > HIDE void sppp_set_ip_addrs(void *, void *);
> > -HIDE void sppp_clear_ip_addrs(struct sppp *sp);
> > +HIDE void sppp_clear_ip_addrs(struct sppp *, void *);
> > HIDE void sppp_set_phase(struct sppp *sp);
> >
> > /* our control protocol descriptors */
> > @@ -3096,9 +3096,15 @@ sppp_ipcp_tls(struct sppp *sp)
> > HIDE void
> > sppp_ipcp_tlf(struct sppp *sp)
> > {
> > + struct ifnet *ifp = &sp->pp_if;
> > +
> > if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN))
> > /* Some address was dynamic, clear it again. */
> > - sppp_clear_ip_addrs(sp);
> > + if (workq_add_task(NULL, 0,
> > + (void *)sppp_clear_ip_addrs, sp, NULL)) {
>
> this cast is wrong. sppp_clear_ip_addrs should take two "void *"
> arguments.
oops yes, I put it on the wrong argument.
> > + printf("%s: workq_add_task failed, cannot clear "
> > + "addresses\n", ifp->if_xname);
> > + }
> >
> > /* we no longer need LCP */
> > sp->lcp.protos &= ~(1 << IDX_IPCP);
> > @@ -4760,16 +4766,22 @@ sppp_set_ip_addrs(void *arg1, void *arg2
> > }
> >
> > /*
> > - * Clear IP addresses. Must be called at splnet.
> > + * Work queue task clearing addresses from process context.
> > + * Clear IP addresses.
> > */
> > HIDE void
> > -sppp_clear_ip_addrs(struct sppp *sp)
> > +sppp_clear_ip_addrs(struct sppp *sp, void *arg2)
> > {
>
> it should still have a "void sppp_clear_ip_addrs(void *, void *)"
> signature.
>
better like this?
Index: if_spppsubr.c
===================================================================
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.96
diff -u -p -r1.96 if_spppsubr.c
--- if_spppsubr.c 28 Jan 2012 12:14:45 -0000 1.96
+++ if_spppsubr.c 22 Jul 2012 10:00:47 -0000
@@ -398,7 +398,7 @@ HIDE void sppp_qflush(struct ifqueue *if
int sppp_update_gw_walker(struct radix_node *rn, void *arg, u_int);
void sppp_update_gw(struct ifnet *ifp);
HIDE void sppp_set_ip_addrs(void *, void *);
-HIDE void sppp_clear_ip_addrs(struct sppp *sp);
+HIDE void sppp_clear_ip_addrs(void *, void *);
HIDE void sppp_set_phase(struct sppp *sp);
/* our control protocol descriptors */
@@ -3096,9 +3096,15 @@ sppp_ipcp_tls(struct sppp *sp)
HIDE void
sppp_ipcp_tlf(struct sppp *sp)
{
+ struct ifnet *ifp = &sp->pp_if;
+
if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN))
/* Some address was dynamic, clear it again. */
- sppp_clear_ip_addrs(sp);
+ if (workq_add_task(NULL, 0,
+ sppp_clear_ip_addrs, (void *)sp, NULL)) {
+ printf("%s: workq_add_task failed, cannot clear "
+ "addresses\n", ifp->if_xname);
+ }
/* we no longer need LCP */
sp->lcp.protos &= ~(1 << IDX_IPCP);
@@ -4760,16 +4766,23 @@ sppp_set_ip_addrs(void *arg1, void *arg2
}
/*
- * Clear IP addresses. Must be called at splnet.
+ * Work queue task clearing addresses from process context.
+ * Clear IP addresses.
*/
HIDE void
-sppp_clear_ip_addrs(struct sppp *sp)
+sppp_clear_ip_addrs(void *arg1, void *arg2)
{
+ struct sppp *sp = (struct sppp *)arg1;
struct ifnet *ifp = &sp->pp_if;
+ int debug = ifp->if_flags & IFF_DEBUG;
struct ifaddr *ifa;
struct sockaddr_in *si;
struct sockaddr_in *dest;
+ int s;
+
+ s = splsoftnet();
+
u_int32_t remote;
if (sp->ipcp.flags & IPCP_HISADDR_DYN)
remote = sp->ipcp.saved_hisaddr;
@@ -4792,6 +4805,7 @@ sppp_clear_ip_addrs(struct sppp *sp)
}
if (ifa && si) {
+ int error;
struct sockaddr_in new_sin = *si;
in_ifscrub(ifp, ifatoia(ifa));
@@ -4800,10 +4814,17 @@ sppp_clear_ip_addrs(struct sppp *sp)
if (sp->ipcp.flags & IPCP_HISADDR_DYN)
/* replace peer addr in place */
dest->sin_addr.s_addr = sp->ipcp.saved_hisaddr;
- if (!in_ifinit(ifp, ifatoia(ifa), &new_sin, 0, 0))
+ if (!(error = in_ifinit(ifp, ifatoia(ifa), &new_sin, 0, 0)))
dohooks(ifp->if_addrhooks, 0);
+ if (debug && error) {
+ log(LOG_DEBUG, SPP_FMT "sppp_clear_ip_addrs: in_ifinit "
+ " failed, error=%d\n", SPP_ARGS(ifp), error);
+ splx(s);
+ return;
+ }
sppp_update_gw(ifp);
}
+ splx(s);
}
