Re: [PATCH 18/44] kdbus: Add var initialization to kdbus_conn_entry_insert()

[Sergei Zviagintsev]

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@xxxxxxxx> wrote:
> > Assign zero to `ret' in the beginning of function instead of doing it
> > in the end.
> >
> > Signed-off-by: Sergei Zviagintsev <sergei@xxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/