Add define guards to avoid multi-inclusion

[liwg06]

This header files are included by multiple other headers.
It's better to add define guards to prevent multi-inclusion.

CLA: trivial

Signed-off-by: Weiguo Li liwg06@foxmail.com

Checklist

• documentation is added or updated
• tests are added or updated

[mspncp]

Thank you for your contribution @liwg06. I have one little request.

[mspncp]

While reviewing your changes, I noticed that the path for md32_common.h is outdated in two comments:

~/src/openssl/master$ git grep -F '../md32_common'

crypto/sha/sha1dgst.c:/* The implementation is in ../md32_common.h */
crypto/sha/sha512.c: *   implementations, ../md32_common.h;

It should be crypto/md32_common.h instead of ../md32_common.h. Would you mind correcting them (in a separate commit) while you are at it?

[mspncp]

For the record: I agree that this is a trivial change.

[liwg06]

Current commit is updated by your advice. @mspncp
With pleasure, fix outdated comments at another commit #17670

[mspncp]

LGTM

[t8m]

I am not sure this would be acceptable with CLA: trivial. Could you please consider signing the CLA?

[paulidale]

Style NIT: we indent preprocessor directives, so all preprocessor lines inside the new #ifndef should gain a space.

[mspncp]

That's true in principle, but there are other locations where include guards were added afterwards and not the entire indentation of the preprocessor directives was adjusted. (I checked the fact, because I had the same idea.) That's why I didn't want to bother the submitter by requesting it.

[mspncp]

But sure, if @liwg06 would volunteer to fix it, that would be great!

[liwg06]

I clone the repo again to dismiss the intermediate results of local compilation, then search the current practice for define guard:
grep -rn ./openssl/ --include=*.h -e "#ifndef.*_H$" -A 1|grep "# define "|wc -l
271
grep -rn ./openssl/ --include=*.h -e "#ifndef.*_H$" -A 1|grep "#define "|wc -l
8
It shows that (may be not accurate) 271 define guards are followed by "# define xxx", and 8 by "#define xxx", so this patch consistent with the majority.

I'm not sure whether I got what you talk about. @paulidale @mspncp
Is it change pattern A to pattern B?
patern A:

#ifndef OSSL_XXX_H
# define OSSL_XXX_H
# pragma once

patern B:

#ifndef OSSL_XXX_H
#define OSSL_XXX_H
#pragma once 

If so, I'll update the commit.

[mspncp]

It is pattern A.

More precisely: we indent conditional blocks like C-blocks, using spaces between # and the directive. Since you added an extra #ifndef ... #endif block, all enclosed preprocessor directives would need an extra space of indentation:

#ifndef OSSL_XXX_H
# if FOO
#  if BAR 
#   define FOOBAR
#  endif
# endif
#endif

[mspncp]

But as you already noticed, there are places in our source code where the indentation is not as it should be.

[t8m]

@liwg06 What we are asking for is to indent the preprocessor directives further in the source code if you are adding another #ifndef around it.

[liwg06]

Got it.
I use the script to indent the line begien with "#" in target files at first, and then add the define guard.

for filename in $(ls \
 include/crypto/md32_common.h \
 include/internal/der.h \
 include/internal/tsan_assist.h \
 providers/implementations/include/prov/ciphercommon.h \
 providers/implementations/include/prov/ciphercommon_aead.h \
 providers/implementations/include/prov/ciphercommon_ccm.h \
 providers/implementations/include/prov/ciphercommon_gcm.h ) ; \
do cat $filename | sed 's/^#/# /' > $filename.new.h ; mv $filename.new.h $filename ; done

The commit is updated, I will check the CI result later.

[t8m]

Although the change is probably not really copyrightable, could you please sign the CLA and drop the CLA: trivial? https://www.openssl.org/policies/cla.html

[liwg06]

It may not really need to sign the CLA, since the fix is really trivial. Thanks for your remind again. @t8m

[t8m]

It does not matter whether the fix is trivial or not. We require a regular CLA for commits that are beyond simple typo fixes and other trivial one to two line changes.

[mspncp]

I sent a slight disagreement whether this kind of change can be considered trivial (in the legal sense). After ignoring whitespace differences all that remains is the addition of include guards. IMHO this is a trivial change.

I suggest that the OTC discuss this next tuesday.

@liwg06 please be so kind and be a little patient with us :-)

[mspncp]

After ignoring whitespace differences ...

Note: all these whitespace diffs were caused by @paulidale's and my request to reindent the preprocessor directives. The original pull request was as simple as b0f6117787cd61425987cbda55c294fd06de492e.

[t8m]

I sent a slight disagreement whether this kind of change can be considered trivial (in the legal sense).

I am not really disputing the fact, that this kind of change is trivial (i.e., uncopyrightable in the legal sense). I am saying that we do not accept such large changes (even if the whitespace changes were not there) with CLA: trivial.

[mspncp]

I agree that this is the current practice, but it is not an immutable rule. IMHO it could (and should) be governed by an explicit OTC policy.

[mspncp]

I sent a slight disagreement...

Ups, of course I meant to write scent, not sent 😊

[t8m]

IMO what is acceptable with CLA: trivial and what not is rather an OMC topic.

[liwg06]

I have singed the CLA and updated the commit log.
Sorry to cause a controversy... @t8m @mspncp

[liwg06]

Since I removed the "CLA: trivial" in commit log, CI test failed at cla-check case.

[mspncp]

Don't worry about it, just wait till the CLA is registered. Then the ticket will be closed and reopenend and the CLA bot will be content.

[mspncp]

Sorry to cause a controversy... @t8m @mspncp

No need to worry. With disagreement I just meant that we are having different opinions, not that we are having a quarrel. :-)

[mspncp]

Don't worry about it, just wait till the CLA is registered. Then the ticket will be closed and reopenend and the CLA bot will be content.

In fact, you fulfilled all your requirements. All you need to do now is sit back and relax until your pull request gets merged.

[mspncp]

I removed the OTC hold since you sent a CLA. Even if we discuss it next week, it won't affect your pull request anymore.

[liwg06]

'◡'

[mspncp]

Ping @openssl/committers for second review. (You might also want to review with whitespace-diffs hidden. A large part of the diffs is caused by reindentation of the preprocessor directives.)

[openssl-machine]

This pull request is ready to merge

[mspncp]

Merged to master in 3d27ac8. Thank you!