[dm-crypt] Coding/style guidelines for cryptsetup?

[dm-crypt] Coding/style guidelines for cryptsetup?

[Schneider, Robert]

Hi,

What are the coding and style guidelines when you want to propose a patch to cryptsetup?

I don't quite get a clear picture from the source code for some things, and I couldn't find documentation on it inside the repo.
I can write a clang-format ruleset if I know the rules...

Here are some things I've noticed while reading the source code, and some remaining questions:

- mostly restricted to C90, e.g. variable declarations at top of functions; but there some use of C99 features like designators in array initializations
  -> do we have to declare variables at the top of functions?
  -> are // style comments allowed?
- would adding -pedantic in GCC be acceptable? (not sure how that would work in autotools)
- tabs are used for indentation
- unclear if spaces for alignment are OK
  -> tab == 8 spaces?
- how to indent when wrapping lines, e.g. for function parameters? 2 tabs? Align at ( with spaces?
  -> sometimes, function declarations put every parameter on its own line
- top-level declarations like functions and structs have { on new line, if/loops/etc inside functions have { on same line, else on same line as }, if/else without braces is allowed
- one space between if/while/for/.. and the (
- documentation of API via doxygen (@ style, not \ style)
- internal documentation via plain comments, no text on first line and all lines starting with aligned *
- max line length?
- the * for pointers binds to the variable name (void *p instead of void* p)
- multiple declarations in the same statement are allowed
- functions with external linkage have a prefix like LUKS2_
- types are not typedef'ed -> struct crypt_device instead of crypt_device_t
- there are file comments with copyright notices
  -> how to create a new file?


Thanks,
Robert
_______________________________________________
dm-crypt mailing list -- dm-crypt@saout.de
To unsubscribe send an email to dm-crypt-leave@saout.de

[dm-crypt] Re: Coding/style guidelines for cryptsetup?

[Milan Broz]

Hi,

On 06/05/2021 13:19, Schneider, Robert wrote:
> Hi,
> 
> What are the coding and style guidelines when you want to propose a patch to cryptsetup?

Code style is very similar to Linux kernel code style.

I will add some doc later (also with "how to contribute docs") etc.

> 
> I don't quite get a clear picture from the source code for some things, and I couldn't find documentation on it inside the repo.
> I can write a clang-format ruleset if I know the rules...

If a formatting exception is more readable, then just format the code differently.

IMO source code is for people, not for machines enforcing rulesets.
(This ends with dealing with patches reshuffling spaces only, we do not need that.)

Anyway, if you plan to write something more sophisticated, better report issue
and discuss it first, please.

Some notes below, but these are not set in stone.

> Here are some things I've noticed while reading the source code, and some remaining questions:
> 
> - mostly restricted to C90, e.g. variable declarations at top of functions; but there some use of C99 features like designators in array initializations
>   -> do we have to declare variables at the top of functions?

yes

>   -> are // style comments allowed?

Just for temporary comments (like TODO etc). Definitely prefer C /**/ comments.

> - would adding -pedantic in GCC be acceptable? (not sure how that would work in autotools)

Not for now. I use many extra warnings for tests, but pedantic is producing really garbage sometimes.
Better spent time with running static analyzers (scan-build, coverity, LGTM, ...)

Both gcc a clang compilation should be clean.

> - tabs are used for indentation

yes, tabs everywhere, even in test scripts.

> - unclear if spaces for alignment are OK
>   -> tab == 8 spaces?

as kernel - default is 8

> - how to indent when wrapping lines, e.g. for function parameters? 2 tabs? Align at ( with spaces?
>   -> sometimes, function declarations put every parameter on its own line

This is not strict, just keep it readable (same as other in the same file).
This (and comments) is the only part where tabs and spaces are mixed.
Definitely it is not unified now.

> - top-level declarations like functions and structs have { on new line, if/loops/etc inside functions have { on same line, else on same line as }, if/else without braces is allowed
> - one space between if/while/for/.. and the (> - documentation of API via doxygen (@ style, not \ style)
> - internal documentation via plain comments, no text on first line and all lines starting with aligned *

All above - yes, very similar to kernel codestyle.

> - max line length?

Lively discussion on this topic is in progress :-)

I would say: if the code is readable, prefer old-school 80 chars.
If longer line helps, no problem but try to exceed 100-110 chars.

Do not reformat embedded foreign code (currently mainly Argon2 crypto lib).

> - the * for pointers binds to the variable name (void *p instead of void* p)
> - multiple declarations in the same statement are allowed

yes

> - functions with external linkage have a prefix like LUKS2_

LUKS2 is internal prefix (for sub-format dirs, similar to LUKS/TCRYPT,VERITY, ...),

Libcryptsetup uses  crypt_* prefix for exported objects.

> - types are not typedef'ed -> struct crypt_device instead of crypt_device_t

not everywhere, something was based on API evolution and it makes no sense to change
it just for the typedef cleanups.

> - there are file comments with copyright notices
>   -> how to create a new file?

Copyright note should always mention authors of that file.
(Otherwise it no longer makes much sense, but we keep that for tracking authors.)

I try to license new code with LGPL 2.1 or any later, but most of code is still GPL2 or any later.
(Cannot fix it globally, there was not agreement of former authors to switch to LGPL.)

Adding file - just copy header with proper license (GPL or LGPL only as above) and copyright.
There are some internal headers with function prototypes, use these.

But why do you need to add source code files?

Milan
_______________________________________________
dm-crypt mailing list -- dm-crypt@saout.de
To unsubscribe send an email to dm-crypt-leave@saout.de

[dm-crypt] Re: Coding/style guidelines for cryptsetup?

[Schneider, Robert]

Thank you very much!

> IMO source code is for people, not for machines enforcing rulesets.

Quite a refreshing take on that topic. I've also seen rulesets being enforced to stop discussions.

> Anyway, if you plan to write something more sophisticated, better report issue
> and discuss it first, please.

I've drafted an atomic header restore. Not entirely finished yet, but the basic functionality seems to be working. Yes I'm that guy with the overengineered transactions ;)

> >   -> do we have to declare variables at the top of functions?
> 
> yes

I'm curious what the rationale is, if you don't mind. I just have not seen this yet in post-C90 code.

> > - would adding -pedantic in GCC be acceptable? (not sure how that would work in autotools)
>
> Not for now. I use many extra warnings for tests, but pedantic is producing really garbage sometimes.

Ah, I thought that would influence pointer conversion warnings, but it doesn't. I usually write C++ and I was bitten in cryptsetup code by making silly typing mistakes like passing a void* to a void** parameter.

> But why do you need to add source code files?

I would like to add some LUKS2 unit tests. There's already unit-utils-io and api-test-2 where I could add them. Api-test-2 unfortunately has some start-up time due to device initializations, and also needs to be run as root because of that. Unit-utils-io only links against libutils_io at the moment, not against libcryptsetup.


Thanks again,
Robert
_______________________________________________
dm-crypt mailing list -- dm-crypt@saout.de
To unsubscribe send an email to dm-crypt-leave@saout.de