linux-kernel - Re: iio/accel/bmc150: Improve unlocking of a mutex in two functions

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: <12013256-ad6a-86ed-9bc1-e5d868189914@users.sourceforge.net>
Date:   Wed, 25 Oct 2017 20:07:48 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Hans de Goede <hdegoede@...hat.com>, linux-iio@...r.kernel.org
Cc:     Hartmut Knaack <knaack.h@....de>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: iio/accel/bmc150: Improve unlocking of a mutex in two functions

> What you are suggesting breaks this pattern

I might be looking for an other balance between involved implementation
details after your constructive feedback for my first approach
in this software module.

> (not using a goto in the last if (err) case)

I would find it nice when a bit more code reduction is feasible.

> which makes the code harder to read and makes things harder
> (and potentially leads to introducing bugs) when
> a step4() gets added.

There is a choice between conservative adjustments and progressive
software refactoring where both directions can lead to similar
development risks.

>>> because that way the error handling is consistent between all steps
>>> and if another step is later added at the end, the last step will
>>> not require modification.

Such a view might express a change resistance.

>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/kxcjk-1013.c?id=1540d0106bcbc4e52013d759a0a0752ae7b4a09d#n760
> 
> So I just checked this one,

Thanks for your interest.

> this one is tricky because the lock is taking inside
> a switch-case and doing gotos inside the case is not pretty.

I imagine that I would like to use scoped lock objects
in affected source files. (Other programming languages support
such synchronisation constructs directly.)

> Basically I believe there is no one-size fits all solution
> here and refactoring like this may introduce bugs, so one
> needs to weight the amount of work + regression risk vs
> the benefits of the code being cleaner going forward.

It seems that our software development discussion can be
continued in a constructive way then.

Regards,
Markus