linux-kernel - Re: [PATCH v2] staging: pi433: move get version func to where all other functions are

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: <20220108111910.GA1978@kadam>
Date:   Sat, 8 Jan 2022 14:19:10 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
Cc:     gregkh@...uxfoundation.org, realwakka@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: pi433: move get version func to where all
 other functions are

On Sat, Jan 08, 2022 at 08:24:38AM +1300, Paulo Miguel Almeida wrote:
> On Fri, Jan 07, 2022 at 11:53:44AM +0300, Dan Carpenter wrote:
> > Just say:
> > 
> > 	retval = rf69_read_reg(spi, REG_VERSION);
> 
> atm rf69_read_reg is a static function in rf69.c.
> 

I would remove be the static.

> I do agree that this is technically possible to write the code
> exactly as you suggested but on the other hand, that would be the only
> exception to the rule when considering all other higher-level functions
> provided in the rf69.c

There may be other functions which will benefit from this later on.  So
instead of "only" a better word is "first".  Some of those high level
functions make sense because they are slightly complicated and have
multiple callers.  But in this case open coding it seems easier to read.

> 
> are you strongly set on the rf69_read_reg approach or would you
> be open to keep the original approach? (rf69_get_version)

I think my approach is best but I don't care.

> 
> > 	if (retval < 0)
> > 		return retval;
> > 
> > Deleting the error handling was a bad style choice.  Also preserve the
> > error code.
> >
> 
> I just want to double-check if this suggestion is taking into
> consideration what was mentioned here:
> https://lore.kernel.org/lkml/20220106210134.GB3416@mail.google.com/ 
> 
> If it is, I'm happy to add it back but I just wanted to confirm it
> first.

Yes.  Keep the error handling.  Your way makes the code more complicated
to read.

regards,
dan carpenter