linux-kernel - Re: [PATCH v2] ubifs: compress lines for immediate return

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:   Mon, 5 Sep 2016 21:44:31 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Heiko Schocher <hs@...x.de>, Richard Weinberger <richard@....at>
Cc:     linux-mtd@...ts.infradead.org,
        Artem Bityutskiy <dedekind1@...il.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] ubifs: compress lines for immediate return

Hi Heiko, Richard,




2016-09-05 15:54 GMT+09:00 Heiko Schocher <hs@...x.de>:
> From: Masahiro Yamada <yamada.masahiro@...ionext.com>
>
> Cleanup the following code construct:
> ret = expression;
> if (ret)
>         return ret;
> return 0;
>
> into a simple form:
> return expression;
>
> From: Masahiro Yamada <yamada.masahiro@...ionext.com>
> posted on the U-Boot mailinglist.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> Signed-off-by: Heiko Schocher <hs@...x.de>



I am the author of the original patch in the U-Boot ML.

Please notice it has not passed the review in U-Boot ML yet.
Actually, I got some feedback against this patch.

See
http://patchwork.ozlabs.org/patch/665199/

Stephan Warren suggested that
we should not break code uniformity.


After I considered it and took a closer look,
I decided that we should not do these changes.


This patch breaks the code uniformity.
See blow:




>  /**
> diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> index 821b348..88cd61d 100644
> --- a/fs/ubifs/gc.c
> +++ b/fs/ubifs/gc.c
> @@ -297,10 +297,8 @@ static int sort_nodes(struct ubifs_info *c, struct ubifs_scan_leb *sleb,
>         err = dbg_check_data_nodes_order(c, &sleb->nodes);
>         if (err)
>                 return err;
> -       err = dbg_check_nondata_nodes_order(c, nondata);
> -       if (err)
> -               return err;
> -       return 0;
> +
> +       return dbg_check_nondata_nodes_order(c, nondata);
>  }

Original code has uniformity here.


err = dbg_check_data_nodes_order(c, &sleb->nodes);
if (err)
       return err;
err = dbg_check_nondata_nodes_order(c, nondata);
if (err)
       return err;



>  /**
> diff --git a/fs/ubifs/lpt_commit.c b/fs/ubifs/lpt_commit.c
> index ce89bdc..79a8e96 100644
> --- a/fs/ubifs/lpt_commit.c
> +++ b/fs/ubifs/lpt_commit.c
> @@ -313,10 +313,7 @@ static int layout_cnodes(struct ubifs_info *c)
>         alen = ALIGN(offs, c->min_io_size);
>         upd_ltab(c, lnum, c->leb_size - alen, alen - offs);
>         dbg_chk_lpt_sz(c, 4, alen - offs);
> -       err = dbg_chk_lpt_sz(c, 3, alen);
> -       if (err)
> -               return err;
> -       return 0;
> +       return dbg_chk_lpt_sz(c, 3, alen);
>

We have dbg_chk_lpt_sz() call just above  (its return value is ignored)

So, returning the value of the last dbg_chk_lpt_sz() call
seems a bit weird.  So, I do not want to touch this.




Heiko,
If you want to post this patch, it is up to you.
But, in that case, could you drop my Author and Signed-off-by,
then send it as your patch, please?

I do not feel comfortable with my authorship
for what I decided to not do.


I will retract my original patch from the U-Boot ML, too.




-- 
Best Regards
Masahiro Yamada