On Sun, Jul 22, 2012 at 11:03 +0100, Stuart Henderson wrote:
> 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.
>
sure.
> better like this?
>
i'm OK with the diff, albeit a tiny nit below.
>
> 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;
you don't need to add extra empty line here.
> +
> + 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);
> }
