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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 6 Dec 2017 16:11:11 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        devel@...verdev.osuosl.org, linux@...f-Entwicklungen.de,
        linux-kernel@...r.kernel.org,
        Simon Sandström <simon@...anor.nu>
Subject: Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into
 two functions

On Wed, Dec 06, 2017 at 12:07:20PM +0200, Marcus Wolf wrote:
> 
> 
> Am 06.12.2017 um 11:37 schrieb Dan Carpenter:
> > On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote:
> > > 
> > > 
> > > Am 06.12.2017 um 00:08 schrieb Simon Sandström:
> > > > Splits rf69_set_crc_enabled(dev, enabled) into
> > > > rf69_enable_crc(dev) and rf69_disable_crc(dev).
> > > > 
> > > > Signed-off-by: Simon Sandström <simon@...anor.nu>
> > > > ---
> > > >    drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++--
> > > >    drivers/staging/pi433/rf69.c     | 18 ++++++------------
> > > >    drivers/staging/pi433/rf69.h     |  4 ++--
> > > >    3 files changed, 28 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > > > index 2ae19ac565d1..614eec7dd904 100644
> > > > --- a/drivers/staging/pi433/pi433_if.c
> > > > +++ b/drivers/staging/pi433/pi433_if.c
> > > > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
> > > >    			return ret;
> > > >    	}
> > > >    	SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering));
> > > > -	SET_CHECKED(rf69_set_crc_enable	    (dev->spi, rx_cfg->enable_crc));
> > > > +
> > > > +	if (rx_cfg->enable_crc == OPTION_ON) {
> > > > +		ret = rf69_enable_crc(dev->spi);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	} else {
> > > > +		ret = rf69_disable_crc(dev->spi);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	}
> > > 
> > > Why don't you use SET_CHECKED(...)?
> > > 
> > 
> > Marcus, please don't introduce new uses of SET_CHECKED().  It has a
> > hidden return in it which is against kernel style and introduces very
> > predictable and avoidable bugs.  For example, in probe().
> 
> Ah ok.
> 
> Thanks for clarifiytion!
> 
> What a pitty - another bunch of extra lines of code...
> 
> Or is there an other construction, allowing for one line per register
> change? Something like
> 
> 	ret = rf69_set_xyz(...); if (ret) return ret;
> 	ret = rf69_set_abc(...); if (ret) return ret;
> 
> is pretty ugly and voids the style guide...

Just spell it out:
	ret = rf69_set_xyz();
	if (ret)
		goto unwind_xyz;

Almost never do you want to instantly return.  You should clean up from
the error first.

But if you do just want to exit, that's fine too, just return.  That's
the normal way here, don't do funny things in macros (like return from a
function), that way lies madness...

thanks,

greg k-h

Powered by blists - more mailing lists