linux-kernel - Re: [v3] [media] Use common error handling code in 19 functions

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] [day] [month] [year] [list]

Date:   Sat, 5 May 2018 09:53:00 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Mauro Carvalho Chehab <mchehab@...radead.org>,
        linux-media@...r.kernel.org
Cc:     Hans Verkuil <hans.verkuil@...co.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andrey Utkin <andrey_utkin@...tmail.com>,
        Arvind Yadav <arvind.yadav.cs@...il.com>,
        Bhumika Goyal <bhumirks@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Brian Johnson <brijohn@...il.com>,
        Christoph Böhmwalder <christoph@...hmwalder.at>,
        Christophe Jaillet <christophe.jaillet@...adoo.fr>,
        Colin Ian King <colin.king@...onical.com>,
        Daniele Nicolodi <daniele@...nta.net>,
        David Härdeman <david@...deman.nu>,
        Devendra Sharma <devendra.sharma9091@...il.com>,
        "Gustavo A. R. Silva" <garsilva@...eddedor.com>,
        Inki Dae <inki.dae@...sung.com>, Joe Perches <joe@...ches.com>,
        Kees Cook <keescook@...omium.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Max Kellermann <max.kellermann@...il.com>,
        Mike Isely <isely@...ox.com>,
        Philippe Ombredanne <pombredanne@...b.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Santosh Kumar Singh <kumar.san1093@...il.com>,
        Satendra Singh Thakur <satendra.t@...sung.com>,
        Sean Young <sean@...s.org>,
        Seung-Woo Kim <sw0312.kim@...sung.com>,
        Shyam Saini <mayhs11saini@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Todor Tomov <todor.tomov@...aro.org>,
        Wei Yongjun <weiyongjun1@...wei.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: [v3] [media] Use common error handling code in 19 functions

> @@ -656,18 +656,18 @@  static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
>  	tsfeed->priv = filter;
>  
>  	ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
> -	if (ret < 0) {
> -		dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto release_feed;
>  
>  	ret = tsfeed->start_filtering(tsfeed);
> -	if (ret < 0) {
> -		dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto release_feed;
>  
>  	return 0;
> +
> +release_feed:
> +	dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> +	return ret;
>  }
> 
> There's *nothing* wrong with the above. It works fine,

I can agree to this view in principle according to the required control flow.


> avoids goto

How does this wording fit to information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”?


> and probably even produce the same code, as gcc will likely optimize it.

Would you like to clarify the current situation around supported
software optimisations any more?


> It is also easier to review, as the error handling is closer
> to the code.

Do we stumble on different coding style preferences once more?


> On the other hand, there's nothing wrong on taking the approach
> you're proposing.

Thanks for another bit of positive feedback.


> In the end, using goto or not on error handling like the above is 
> a matter of personal taste - and taste changes with time

Do Linux guidelines need any adjustments?


> and with developer. I really don't have time to keep reviewing patches
> that are just churning the code just due to someone's personal taste.

I tried to apply another general source code transformation pattern.


> I'm pretty sure if I start accepting things like that,
> someone else would be on some future doing patches just reverting it,
> and I would be likely having to apply them too.

Why?

I hope also that the source code can be kept consistent to some degree.


> So, except if the patch is really fixing something - e.g. a broken
> error handling code, I'll just ignore such patches and mark as
> rejected without further notice/comments from now on.

I would find such a communication style questionable.
Do you distinguish between bug fixes and possible corrections for
other error categories (or software weaknesses)?

Regards,
Markus