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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 25-10-17 18:58, SF Markus Elfring wrote:
If that is the only unlock in the function, then it is probably
best to keep things as is. In general gotos are considered
better then multiple unlocks, but not having either is
even better.

Thanks for your quick feedback.


How do you think about to use the following code variant then?

     if (!ret)
         ret = IIO_VAL_INT;


I believe the goto unlock variant and setting ret = IIO_VAL_INT;
directly above the unlock label variant is better,

I would prefer the approach above so that an extra goto statement
could also be avoided before.

Usually code in a function follows a pattern of:

	err = step1()
	if (err)
		handle-err


	err = step2()
	if (err)
		handle-err


	err = step3()
	if (err)
		handle-err

What you are suggesting breaks this pattern (not
using a goto in the last if (err) case) which makes
the code harder to read and makes things harder
(and potentially leads to introducing bugs) when
a step4() gets added.

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.

Do any more contributors insist on such a code layout?

There definitely is some personal preference involved here,
but I do believe that consistency is more important then
saving a goto here.

How long should I wait for corresponding feedback before another small
source code adjustment will be appropriate?

That is hard to say.

I am just curious on how we can achieve progress here.


I usually just do a new version when I've time,

This is generally fine.


seldomly someone complains I should have waited longer for feedback
(when I'm quite quick) but usually sending out a new version as soon
as you've time to work on a new version is best, since if you wait
you may then not have time for the entire next week or so, at least
that is my experience :)  There is really no clear rule here.

I asked also because other well-known contributors showed strong
reactions for this change pattern (with the help of a script
for the semantic patch language).
Would you care for similar updates in source files like the following?

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, this one is tricky because the
lock is taking inside a switch-case and doing gotos inside
the case is not pretty. If I were to refactor this, I would
add an "if (mask == IIO_CHAN_INFO_SCALE) {}" block to
handle the unlocked scale case and then take the lock around
the entire switch-case, using breaks on error to jump to
the unlock after the switch-case without needing gotos.

To me this seems the right thing here, since the scale is
special here in that it does not need locking. Or optionally
one can just take the lock for scale regardless, it won't
hurt (much).

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.

Regards,

Hans



https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/accel/stk8312.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n432
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/adc/palmas_gpadc.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n304

Regards,
Markus

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux