Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Guillaume Nault <gnault@redhat.com>]

On Tue, Nov 17, 2020 at 12:26:38PM +0000, Tom Parkin wrote:
> On  Sun, Nov 15, 2020 at 17:28:38 +0100, Guillaume Nault wrote:
> > On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> > > On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > > However, my primary motivation for using ppp_channel_push was actually
> > > the handling for managing dropping the packet if the channel was
> > > deregistered.
> > 
> > I might be missing something, but I don't see what ppp_channel_push()
> > does appart from holding the xmit lock and handling the xmit queue.
> > If we agree that there's no need to use the xmit queue, all
> > ppp_channel_push() does for us is taking pch->downl, which we probably
> > can do on our own.
> > 
> > > It'd be simple enough to add another function which performed the same
> > > deregistration check but didn't transmit via. the queue.
> > 
> > That's probably what I'm missing: what do you mean by "deregistration
> > check"? I can't see anything like this in ppp_channel_push().
> 
> It's literally just the check on pch->chan once pch->downl is held.
> So it would be trivial to do the same thing in a different codepath: I
> just figured why reinvent the wheel :-)

Okay, I was thinking of something more complex. I agree with not
reinventing existing functions, but in this case, I think that
ppp_channel_push() does too many unecessary operations (like recursion
handling and processing the parent unit's queue). Also, a helper
function would be the natural place for calling skb_scrub_packet()
and for handling concurent access or modification of the ->bridge
pointer (as discussed earlier in this thread).

> 
> Sorry for the confusion.

No problem. It's nice to see some work being done in this area :).