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

[Jonathan Cameron]

On Wed, 25 Oct 2017 20:07:48 +0200
SF Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:

> > 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.)

I'd keep it simple for now. Also, I'd actually take a different
approach to tidy up this case we are talking about here.

Factor out the whole case IIO_CHAN_INFO_RAW block as a 
utility function.  Then nice clean and simple lock handling
can be done in the error paths without the readability problems
that you get doing it deeply nested.

Btw. There is another issue in that code that needs fixing
which is that it will race with the buffer being enabled.
It should be using the iio_claim_direct infrastructure to
prevent that cleanly. 

That example is definitely more ugly that it needs to be
so would be nice to clean it up if you have time.

Thanks,

Jonathan
> 
> 
> > 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html