Messages in this thread | | | Date | Tue, 27 Jul 2021 15:56:11 +0300 | From | Leon Romanovsky <> | Subject | Re: [PATCH net-next 0/4] net: ipa: kill IPA_VALIDATION |
| |
On Tue, Jul 27, 2021 at 07:34:41AM -0500, Alex Elder wrote: > On 7/27/21 6:16 AM, Leon Romanovsky wrote: > > On Mon, Jul 26, 2021 at 12:40:06PM -0500, Alex Elder wrote: > >> A few months ago I proposed cleaning up some code that validates > >> certain things conditionally, arguing that doing so once is enough, > >> thus doing so always should not be necessary. > >> https://lore.kernel.org/netdev/20210320141729.1956732-1-elder@linaro.org/ > >> Leon Romanovsky felt strongly that this was a mistake, and in the > >> end I agreed to change my plans. > > > > <...> > > > >> The second patch fixes a bug that wasn't normally exposed because of > >> the conditional compilation (a reason Leon was right about this). > > > > Thanks Alex, > > > > If you want another anti pattern that is very popular in netdev, the following pattern is > > wrong by definition :): > > if (WARN_ON(...)) > > return ... > > I understand this reasoning. > > I had it return an error if the WARN_ON() condition was true in cases > where the function returned a value and callers already handled errors. > I looked back at the patch and here is one of those cases: > > gsi_channel_trans_alloc() > - If too many TREs are requested we do not want to allocate them > from the pool, or it will cause further breakage. By returning > early, no transaction will be filled or committed, and an error > message will (often) be reported, which will indicate the source > of the error. If any error occurs during initialization, we fail > that whole process and everything should be cleaned up. So in > this case at least, returning if this ever occurred is better > than allowing control to continue into the function. > > In any case I take your point. I will now add to my task list > a review of these spots. I'd like to be sure an error message > *is* reported at an appropriate level up the chain of callers so > I can always identify the culprit in the a WARN_ON() fires (even > though it should never > happen). And in each case I'll evaluate > whether returning is better than not.
You can, but users don't :). So if it is valid but error flow, that needs user awareness, simply print something to the dmesg with *_err() prints.
BTW, I'm trying to untangle some of the flows in net/core/devlink.c and such if(WARN()) pattern is even harmful, because it is very hard to understand when that error is rare/non-exist/real.
Thanks
> > Thanks. > > -Alex > > > The WARN_*() macros are intended catch impossible flows, something that > > shouldn't exist. The idea that printed stack to dmesg and return to the > > caller will fix the situation is a very naive one. That stack already > > says that something very wrong in the system. > > > > If such flow can be valid use "if(...) return ..", if not use plain > > WARN_ON(...). > > > > Thanks > > >
| |