Add first .clang-tidy file

[joholl]

Add a .clang-tidy file with some of the less opinionated check. Fix the linting issues (one commit per check).

For linting, I don't call clang-tidy directly, but the official python script for multi-core support. You can add -fix for automatic fixing.

make -j compile_commands.json
run-clang-tidy-15 -header-filter $PWD $(find src include -name '*.c' -o -name '*.h') | ansi2txt | tee clang-tidy.log

For now, I focused on src/, but not test/

Fixes: #2826

[joholl]

@JuergenReppSIT @AndreasFuchsTPM cirrus keeps failing on me, but otherwise the CI is happy. What do you think of this?

[AndreasFuchsTPM]

Seams to dislike leading _
This is especially bad in the cases of API incompatibilities.

[joholl]

@AndreasFuchsTPM TLDR; it is undefined behavior since those identifiers are reserved (for stdlib and compiler i guess).

Here's what the C standard says (section 7.1.3):

• All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
• All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

And further:

If the program declares or defines an identifier in a context in which it is reserved [...] the behavior is undefined.

Clang-tidy checks that via rule bugprone-reserved-identifier.

_PRIVATE

_PRIVATE might not really be a "private" type, but just a utility type for defining TPM2B_PRIVATE. While not useful to the user either, we could make it public (but we should mangle the name somehow).

struct TPM2B_PRIVATE {
    UINT16 size;
    BYTE buffer[sizeof(PRIVATE)];
};

If we really wanted to make it private, we would need to jump through hoops because we have a sizeof(_PRIVATE) in our public header (e.g. create a configure-time macro which provides the size of PRIVATE and use that instead).

Suggestion: rename it TPM_OPAQUE_PRIVATE (and adding docs telling users it is private). Technically, this is a breaking API change. I would argue, using this type as a user is fishy in any case. In my understanding this is supposed to be an opaque blob. We also don't use it in the TSS (except as an argument to sizeof() in the json (de)serialization code.

An alternative is leaving it as-is and technically introducing undefined behavior in every user's namespace.

_TSS2_SYS_OPAQUE_CONTEXT_BLOB

So this is really a private type. I'd argue the OPAQUE portion of the name already makes it clear what this is.

Suggestion: Rename to TSS2_SYS_OPAQUE_CONTEXT_BLOB and writing a docstring telling users to not use it.

__ptr

It is technically legal to have a leading underscore (as _foo) in a limited scope, so e.g. as a parameter (but not on file scope). We probably just want to rename that to ptr. This has no API implications anyway.

Suggestion: rename to ptr

[AndreasFuchsTPM]

Understood.
__ptr -> ptr no problem. Agreed.

For the rest: I guess they make sense, BUT we'd need to roll 3.0 for this.
Thankfully, it does not change ABI but only API, so we don't have to bump the .so-Version.

The big question is: Is it worth to go to 3.0 over this ?

[JuergenReppSIT]

The big question is: Is it worth to go to 3.0 over this ?

Maybe we could first split the PR and extract the "real" bugfxes?

[joholl]

@JuergenReppSIT Alright, I split the changes. The API braking changes are currently on joholl/tpm2-tss@clang_tidy_api_changes. I'm going to submit a PR once this is merged. We could roll that out when we roll a new major version anyway due to another API change (@AndreasFuchsTPM I assume you meant 5.0).

Any more feedback?

[AndreasFuchsTPM]

@joholl Scan-Build complains about

../src/tss2-fapi/api/Fapi_GetTpmBlobs.c:221:32: warning: Result of 'malloc' is converted to a pointer of type 'uint8_t', which is incompatible with sizeof operand type 'TPM2B_PUBLIC'
                *tpm2bPublic = malloc(sizeof(TPM2B_PUBLIC));
                               ^~~~~~ ~~~~~~~~~~~~~~~~~~~~