linux-kernel - Re: [PATCH] sr9800: Use common error handling code in sr9800_phy

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:   Mon, 30 Oct 2017 10:59:20 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        Bjørn Mork <bjorn@...k.no>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Ungerer <gerg@...ux-m68k.org>,
        Liu Junliang <liujunliang_ljl@....com>,
        Philippe Reynes <tremyfr@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH] sr9800: Use common error handling code in sr9800_phy_powerup()

Hi Markus,

On Sun, Oct 29, 2017 at 1:03 PM, SF Markus Elfring
<elfring@...rs.sourceforge.net> wrote:
>>> @@ -700,10 +700,9 @@ static int sr9800_phy_powerup(struct usbnet *dev)
>>>
>>>         /* set the embedded Ethernet PHY in power-up state */
>>>         ret = sr_sw_reset(dev, SR_SWRESET_IPRL);
>>> -       if (ret < 0) {
>>> -               netdev_err(dev->net, "Failed to reset PHY: %d\n", ret);
>>> -               return ret;
>>> -       }
>>> +       if (ret < 0)
>>> +               goto report_reset_failure;
>>
>> So now I have to look below to see what error handling it does...
>
> Yes. - Can this be an usual consequence if you apply information from
> the section “7) Centralized exiting of functions” in the document
> “coding-style.rst” a bit more?

That section is useful if several cleanups steps have to be performed.
In this case, the only common part is the printing of the error message.
Printing error messages is not a cleanup step.

What's also bad in this case, is that after the first "goto", there are other
error cases that don't use goto, but just return, because there's no cleanup
to do there.

Hence:
NAKed-by: Geert Uytterhoeven <geert@...ux-m68k.org>

>> Hence I prefer the original version, which had _less_ lines of code...
>
> My update suggestion is only one line “bigger” in this case, isn't it?
>
> I propose an other source code layout so that a bit smaller executable
> object code could be achieved.
> Do find such a software design direction feasible?

If you play the "smaller executable object code" card, people expect that
you provide the actual numbers, too.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds