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

Date:   Fri, 4 May 2018 14:50:15 -0300
From:   Mauro Carvalho Chehab <mchehab@...radead.org>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:     linux-media@...r.kernel.org, Hans Verkuil <hans.verkuil@...co.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andi Shyti <andi.shyti@...sung.com>,
        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

Em Fri, 4 May 2018 18:08:59 +0200
SF Markus Elfring <elfring@...rs.sourceforge.net> escreveu:

> > Adjust jump targets so that a bit of exception handling can be better
> > reused at the end of these functions.  
> 
> Why was this update suggestion rejected once more a moment ago?
> 
> https://patchwork.linuxtv.org/patch/47827/
> lkml.kernel.org/r/<57ef3a56-2578-1d5f-1268-348b49b0c573@...rs.sourceforge.net>
> https://lkml.org/lkml/2018/3/9/823

Taking just the first diff there as an example:


diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 61a750fae465..17d05b05fa9d 100644
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -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, avoids goto
and probably even produce the same code, as gcc will likely optimize
it. It is also easier to review, as the error handling is closer
to the code. On the other hand, there's nothing wrong on taking
the approach you're proposing.

In the end, using goto or not on error handling like the above is 
a matter of personal taste - and taste changes with time 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'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.

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.


Thanks,
Mauro