linux-kernel - Re: [PATCH 18/44] kdbus: Add var initialization to kdbus_conn_entry

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: <20151009175254.GE2189@localhost.localdomain>
Date:	Fri, 9 Oct 2015 20:52:54 +0300
From:	Sergei Zviagintsev <sergei@...v.net>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	Djalal Harouni <tixxdz@...ndz.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 18/44] kdbus: Add var initialization to
 kdbus_conn_entry_insert()

Hi,

On Thu, Oct 08, 2015 at 04:28:29PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev <sergei@...v.net> wrote:
> > Assign zero to `ret' in the beginning of function instead of doing it
> > in the end.
> >
> > Signed-off-by: Sergei Zviagintsev <sergei@...v.net>
> > ---
> >  ipc/kdbus/connection.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index 4f3cd370ecd9..185ed3ba1bce 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -889,7 +889,7 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
> >                             const struct kdbus_name_entry *name)
> >  {
> >         struct kdbus_queue_entry *entry;
> > -       int ret;
> > +       int ret = 0;
> >
> >         kdbus_conn_lock2(conn_src, conn_dst);
> >
> > @@ -916,8 +916,6 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src,
> >         kdbus_queue_entry_enqueue(entry, reply);
> >         wake_up_interruptible(&conn_dst->wait);
> >
> > -       ret = 0;
> > -
> 
> Not a big fan of this. It makes it less obvious, and this style is
> wrong in several cases (but not here). We often only check for "ret <
> 0", but generally want >0 to be turned into 0 on return.
> 
> It does not matter in this specific case, but I prefer making return
> codes explicit, rather than relying on a previous initialization to be
> still valid.
> 
> What's your rationale here?

The rationale is to keep things simple. That `ret' var is used only once
to deliver the error code, and the function itself has only two local
vars and fits into my 12.5 inch thinkpad screen, so IMO that extra line
with assignment is redundant. I agree that in some cases we need to
handle 'ret > 0', but using same templates for every particular case
produces boring code :)

And BTW we have this style in number of places over kdbus code. For
example see kdbus_handle_ioctl_control(), kdbus_handle_ioctl_ep(),
kdbus_name_update(), kdbus_name_release(), kdbus_pool_release_offset(),
kdbus_pool_slice_copy().

> 
> Thanks
> David
> 
> >  exit_unlock:
> >         kdbus_conn_unlock2(conn_src, conn_dst);
> >         return ret;
> > --
> > 1.8.3.1
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/