I'm really unhappy with this kind of commit.

First of all, it mixes several unrelated changes into one commit. This is bad practice.

Note that in mainproc.c SYMKEYS is unsigned and thus a greater than 0
condition is surprising because it leads to the assumption SYMKEYS
could be negative.

No, really not. It leads to the assumption that SYMKEYS could be 0, which it very well could be.

Better use a boolean test.

No, again, really not. At best it is different to use a boolean test, in no way is it better. I would even argue, that your version is worse because it implicitly "coerces" the integer value to a "boolean", whereas my version does the semantically same thing but does it explicitly.

Likewise for changing ptr == NULL to !ptr.

This is a matter of taste. Until these things are explicitly stated in the coding conventions, I feel that we should be allowed to have different styles, and you patching over my commits like that is unproductive, vcs noisy, and frankly a bit unfriendly.

The assert could have lead to a regression for no good reason.

The good reason is that code is executing under conditions that the coder (me) had not in mind. Using assertions shows that the state space is exhaustively covered by the conditionals. Using assertions gives stronger guarantees.

Not being compliant is better than breaking existing users.

Failing early is better than to execute code under nonsensical conditions.

On Tue, 20 Jun 2017 10:20, noreply@dev.gnupg.org said:

First of all, it mixes several unrelated changes into one commit.  This is bad practice.

No, these were all cleanups of another commit. I touched some of the
lines only because you seem to have accidently set your Emacs to 86
columns. Well, that can happen.

> Note that in mainproc.c SYMKEYS is unsigned and thus a greater than 0
>  condition is surprising because it leads to the assumption SYMKEYS
>  could be negative.

No, really not.  It leads to the assumption that SYMKEYS could be 0, which it very well could be.

The code intends to do a boolean check (with or without symkeys) and
that is why I had to investigate why you used > 0. The common reason to
do that is to take care of an error condition indicated by -1. Now, that
was not the case and thus an unsigned variable and a matching boolena
condition is the Right Thing.

> Better use a boolean test.

No, again, really not.  At best it is different to use a boolean
test, in no way is it better.  I would even argue, that your version
is worse because it implicitly "coerces" the integer value to a
"boolean", whereas my version does the semantically same thing but
does it explicitly.

One should always code with cone maintenance in mind. In 10 years a
maintenance engineer might read the code and will come to the same false
conclusions I had. This is a potential cause for a bug but at least it
slows down their work. Thus being able to quickly ready and understand
the code is paramount.

As an example: It costed Stephan an me quite some time to understand why
you did the is_compliance and is_allowed ting. It is indeed a clever
idea but the documentation did not properly explained it and your
comment on our question didn't do it either (granted, we could have
asked more explicitly).

Likewise for changing `ptr == NULL` to `!ptr`.

This is a matter of taste.  Until these things are explicitly stated
in the coding conventions, I feel that we should be allowed to have
different styles, and you patching over my commits like that is
unproductive, vcs noisy, and frankly a bit unfriendly.

Yes and no. The general advice is to look at existing code and mimic
that. Now, I wrote most of the code and I basically never use a
conditional operator when doing a boolean evaluation. I would have not
chnaged it to avoid commit noise but I touch the other part of the
condition anyway and thus I made it more readable (according to my
taste). That is definitely not unproductive becyuase it helps to read
the code. Now for the reasons why comparing to NULL or 0 is a bad idea:

• you may accidentally assign false to the variable. Please don't respond that gcc will likely catch that: Not all compilers can do that.

• it is immediately clear that this is a boolean expression. This is faster to read and understand because your brain does not need to evaluate the operator and the second term.

• With a segmented memoroy model you need to choose the right flavor of NULL (with the near or the far attribute cast). This often goes wrong and you will have a hard time to track down such bugs. Thus the long standing rule is to let the compiler to that and limit the use of NULL* to where they are really needed (certain assignments). Yes, I know that in a segmented memory model you also can't use memset(3) to clear pointers.

• Different coding styles are bad. The Go folks get this right by not allowing that. I know I will be the first one to curse about it but from a management POV it is the Right Thing.

> The assert could have lead to a regression for no good reason.

The good reason is that code is executing under conditions that the coder (me) had not in mind.  Using assertions shows that the state space is exhaustively covered by the conditionals.  Using assertions gives stronger guarantees.

Made up report from the FAA: “The crash of the 777 during takeoff at
Logan airport was caused by the left wing engine shutting down due to an
unexpected condition in its control software. A Boing speaker explained
that the company changed their software to favor correct software over
failsafe modes.”

> Not being compliant is better than breaking existing users.

Failing early is better than to execute code under nonsensical conditions.

Depends on the consequences. For new features this might be okay but
not for features already used in production code.

Shalom-Salam,

Werner

In rG3621dbe52584bc8b417f61b5370ebaa5598db956#44380, @werner wrote:

As an example: It costed Stephan an me quite some time to understand why
you did the is_compliance and is_allowed ting. It is indeed a clever
idea but the documentation did not properly explained it and your
comment on our question didn't do it either (granted, we could have
asked more explicitly).

I really do not understand that critique. First of all, the comments for these functions clearly state what the predicates compute. Then, I just re-read your e-mails, and neither you nor Stephan asked me anything remotely related to the semantics of these two sets of predicates.

> The assert could have lead to a regression for no good reason.

The good reason is that code is executing under conditions that the coder (me) had not in mind.  Using assertions shows that the state space is exhaustively covered by the conditionals.  Using assertions gives stronger guarantees.

Made up report from the FAA: “The crash of the 777 during takeoff at
Logan airport was caused by the left wing engine shutting down due to an
unexpected condition in its control software. A Boing speaker explained
that the company changed their software to favor correct software over
failsafe modes.”

I'm pretty sure that anyone in the aviation industry uses formal verification techniques, and these methods improve greatly with the use of assertions as they a) can be disproven by conterexample, b) state and document the intentions of the programmer, and c) help to reduce the state space that needs to be examined.

> Not being compliant is better than breaking existing users.

Failing early is better than to execute code under nonsensical conditions.

Depends on the consequences. For new features this might be okay but
not for features already used in production code.

This is a new feature.

On Wed, 21 Jun 2017 12:02, noreply@dev.gnupg.org said:

I really do not understand that critique.  First of all, the
comments for these functions clearly state what the predicates

(It was an example). Yes, the comments stated that but we had no idea
why this is needed. But as I said, we could have asked better but the
deadline was pressing on Monday evening.

I'm pretty sure that anyone in the aviation industry uses formal
verification techniques, and these methods improve greatly with the

That is not the point. To repeat my claim in the words of my driving
instructor: The writing on his tombstone will be “He had the right of
way”.

This is a new feature.

No it is not. It changes all basic operations.

Shalom-Salam,

Werner

In rG3621dbe52584bc8b417f61b5370ebaa5598db956#44388, @werner wrote:

On Wed, 21 Jun 2017 12:02, noreply@dev.gnupg.org said:

I really do not understand that critique.  First of all, the
comments for these functions clearly state what the predicates

(It was an example). Yes, the comments stated that but we had no idea
why this is needed.

Am I the only one who read "that document"?

I'm pretty sure that anyone in the aviation industry uses formal
verification techniques, and these methods improve greatly with the

That is not the point. To repeat my claim in the words of my driving
instructor: The writing on his tombstone will be “He had the right of
way”.

I want to clearly state why I added the assertion there, and why removing it in my opinion changes the semantics of that piece of code, and therefore why removing it is irresponsible.

The loop below the former assertion computes whether the decryption operation is in compliance with CO_DE_VS. It is in compliance, among other things, if every key used to encrypt the symmetric encryption key is in compliance. This is a conjunction of variable arity, possibly zero. The neutral element of that operation is true, hence a conjunction of aritiy zero yields true, but this is a vacuous truth. This is okay in the case of symmetric encryption, as stated by "that document", but it may or may not true in any other case. I don't know whether there could be such an case, or whether it is added in the future by someone not familiar by this particular piece of code. Therefore, I added that assertion to document and enforce the invariant, that we either use symmetric encryption, or there is at least one asymmetric key that we check. By removing the assertion, you increased the chance of that code malfunctioning, now or in the future.

Not being compliant is better than breaking existing users.

That is not the danger by the way. The danger is doing an operation that is not compliant while reporting it as compliant. I imagine the BSI caring greatly about this kind of erroro.

Also, a failing assertion is reported immediately, whereas noone will ever see that log_debug statement. Better to remove the two lines, they just clutter the code now.