netdev - Re: [PATCH net] failover: eliminate callback hell

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180605221049-mutt-send-email-mst@kernel.org>
Date:   Tue, 5 Jun 2018 22:38:43 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     kys@...rosoft.com, haiyangz@...rosoft.com, davem@...emloft.net,
        sridhar.samudrala@...el.com, netdev@...r.kernel.org,
        Stephen Hemminger <sthemmin@...rosoft.com>
Subject: Re: [PATCH net] failover: eliminate callback hell

On Tue, Jun 05, 2018 at 11:53:05AM -0700, Stephen Hemminger wrote:
> > >   * Now, netvsc and net_failover use the same delayed work type
> > >     mechanism for setup. Previously, net_failover code was triggering off
> > >     name change but a similar policy was rejected for netvsc.
> > >     "what is good for the goose is good for the gander"  
> > 
> > I don't really understand what you are saying here.  I think the delayed
> > hack is kind of ugly and seems racy.  Current failover code was rejected
> > by whom?  Why is new one good and for whom?  Did you want to do a name
> > change in netvsc but it was rejected? Could you clarify please?
> 
> See:
>    https://patchwork.ozlabs.org/patch/851711/

Let me try to summarize that:

	You wanted to speed up the delayed link up.  You had an idea to
	additionally take link up when userspace renames the interface (standby
	one which is also the failover for netvsc).

	But userspace might not do any renames, in which case there will
	still be the delay, and so this never got applied.

	Is this a good summary?

Davem said delay should go away completely as it's not robust, and I
think I agree.  So I don't think we should make all failover users use
delay. IIUC failover kept a delay option especially for netvsc to
minimize the surprise factor. Hopefully we can come up with
something more robust and drop that option completely.


> > >   * Set permanent and current address of net_failover device
> > >     to match the primary.
> > > 
> > >   * Carrier should be marked off before registering device
> > >     the net_failover device.  
> > 
> > Are above two bugfixes?
> 
> Yes.

Maybe fix these two as first patches in the set?

> > > Although this patch needs to go into 4.18 (linux-net),  
> > 
> > I'd rather we focused on fixing bugs in 4.18, and left refactoring to
> > 4.19.
> >
> 
> Either we fix or revert the current code in 4.18.
> Sorry, I am not having callback hell code in any vendor or upstream kernel.

I agree callbacks add complexity which often isn't necessary, so
removing them where possible is a good cleanup.  But maybe a patch
shouldn't mix bugfixes, cleanups and behaviour changes all together.  If
nothing else it makes review harder.  Splitting patches up might make it
more likely they can go into 4.18 which seems to be what you want.

HTH,
-- 
MST