linux-kernel - Re: virtio_blk: Less function calls in init

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: <55fbd4d0-3fb3-0334-6545-cb4da0d12edf@users.sourceforge.net>
Date:   Tue, 13 Sep 2016 16:33:03 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Christian Bornträger <borntraeger@...ibm.com>,
        virtualization@...ts.linux-foundation.org,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Julia Lawall <julia.lawall@...6.fr>,
        kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>, linux-doc@...r.kernel.org,
        Minfei Huang <mnghuan@...il.com>
Subject: Re: virtio_blk: Less function calls in init_vq() after error
 detection

>>  drivers/block/virtio_blk.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> Can't you see from this diffstat that the patch actually seems to makes
> the code more complex?

I find that the repeated usage of a bit more error handling code is almost
unavoidable if you would like to handle allocation failures more directly
as I dared to propose again here.

> In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
>     virtio_blk: Fix a slient kernel panic
> 
> which did the opposite of your patch.

This software update adjusted also the jump targets. This approach
triggered another update suggestion (in addition to improvements around
the function "kmalloc_array").

Such a software development shows different views on the implementation
for correct exception handling. I am not so "silent" on this development topic
for years.

> And in fact it fixed a bug.

I get the impression from Minfei's contribution that the statement "err = -ENOMEM;"
was added behind memory allocations.
It was also chosen to restructure this function implementation so that
the single label "out" was used there for a while.

* Is this detail worth for another look?

* How does this name selection fit to the current Linux coding style convention?

> Quite obviously multiple labels are harder to read

I do not agree agree completely to your opinion.

> and harder to get right.

These identifiers can generate their own kind of software development
challenges as usual.

> For error handling with just kfree one label is just the right thing to.

This approach can look convenient at first glance.
Does the correctness aspect need any further considerations?

Regards,
Markus