Re: [dm-crypt] goto patch for cryptsetup?

[Milan Broz <gmazyland@gmail.com>]

On 03/07/2014 11:52 AM, Lars Winterfeld wrote:
> Am 03/07/2014 06:23 AM, schrieb Milan Broz:
>> On 03/06/2014 09:39 PM, Lars Winterfeld wrote:
>>> Hi,
>>>
>>> in light of the latest "goto fail"s out there, would you reject a patch
>>> replacing all 328 gotos with their semantic equivalents from structured
>>> C programming?
>>
>> cryptsetup code uss goto for only one purpose: to have only one exit
>> place from a function (with check for errors, release memory).
>>
>> Replacing it with anything else means duplicating of lot of code.
> 
> I agree that code duplication is undesirable. I can think of two
> alternatives to goto that do not duplicate code. The first is to
> introduce a one-time loop like this:

No please. Using goto here the style I prefer (you would use exception
in other languages but this is intentionally low level C).

I really do no understand which problem you are trying to solve here
- goto can be used very wrong, but this is not the case.


> do {
> // foo
> if(a) break; // was: goto out;
> // bar
> if(b) break; // was: goto out;
> // "all good" code
> return 0;
> } while(0);
> // was: out:
> // "on error" code
> return -1;

Which is not only ugly but adds one level of indentation.

I am basically using kernel code style I like, read
https://www.kernel.org/doc/Documentation/CodingStyle

(specifically chapter 7 - goto)

...

Please do not spend time with replacing goto this way.

If you want help - write a tests, there is a lot of things which
should have regression tests and still missing there.

I would also appreciate script for fuzzy testing, run time checks
(to avoid things like gcrypt KDF fail) etc.

Running various static/dynamic analyzers and filter out false positives
(and send patches for real problems) helps as well.

And if anyone understand recent changes in automake, I would appreciate
fixes for warnings. (I am thinking also if we could replace it with Cmake:)

Thanks,
Milan
p.s.
If the reason for this is recent misapplied patch for OpenSSL,
the same problem can happen even without goto. Testsuite should detect
such problems...
see e.g. http://www.tedunangst.com/flak/post/a-brief-history-of-one-line-fixes