Issues for general cleanup?

[boaks]

As mentioned in add ccm - comment on too many whitespace change the current code seems the have a mixed up format.

Therefore I would like to collect some input for a general cleanup.

From my side:

• whitespace format
• license update to EPL v2.0
• logging:
  • check messages (e.g. calls dtls_debug_dump already with a ":" at the end of the name),
  • remove eols ("\n") from the messages and add that to the print functions in dtls_debug.

Such general cleanups usually "locks the code for some time", so maybe it's also worth to get some feedback, what should be done before (e.g. merging selected open PRs).

[obgm]

One task would be to adjust existing code to follow the basic coding style outlined in CONTRIBUTING and enforce contributions to follow that style. In another project, I am currently testing a clang-format specification that does almost the right thing. (The problem with these tools is that you need to write a lot of annotations where you want to deviate from the strict formatting rules.)

[obgm]

Regarding license update: What would be the reasons for undergoing the enormous pain of changing the license model? I presume you would not do this unless there is something entirely wrong with the existing EPL v1.0/EDL v1.0.

[boaks]

I may be wrong, but I assume, a new release requires a 2.0.

enormous pain

Because the difference in the license? It mainly enables the user to combine it with code under GPL, if they want/need to do so.

Or because the editing? Tinydtls is tiny, the editing will be done fast. For some coorporate user it may be also important to have a "SPDX-License-Identifier: EPL-?.0" in order to apply scanners. That would anyway cause the edit.

[obgm]

I may be wrong, but I assume, a new release requires a 2.0.

Okay, more reading to do for release preparation.

The pain arises from reading legal stuff: All tinydtls users will have to make their lawyers process the new license. It is not the editing that makes license changes difficult but the changed contents of the license. I have not yes compared the two documents or read discussions that may have commented on the changes but then again, this is exactly what I meant with "pain".

[boaks]

EPL-v1.0-FAQ

EPL-v2.0-FAQ

Diffs

EPL-1.0 has been deprecated

Yes, some may require to read it. But many people and companies are common to EPL 2.0 and therefore I consider, that processing it by lawyers is a rare exception. EPL 2.0 is not intended to make the use harder, it's intended to make it easier.

[boaks]

In another project, I am currently testing a clang-format specification that does almost the right thing. (The problem with these tools is that you need to write a lot of annotations where you want to deviate from the strict formatting rules.)

As long as the tool is used manually, that should not cause too much trouble. I would propose to go with one commit with the applied format, and one commit (maybe one per file), which need manual corrections of that. I would propose, to spare out the included third-party stuff as, "aes/rijndael" or "sha2/sha2".

[boaks]

If agreed, we may also update the copyright times for the EPL files.

[boaks]

My first experience with ".clang-format" using VS Code is promising. It's also possible to apply a change only on a change section. FMPOV, using it manually and not automated, will make it possible to have a couple of "custom formatted sections" (hopefully not too much) while the main parts will be formatted accordingly.

(When it gets clear, how many section requires a custom format, we may consider again to use the "protection comments".)

[boaks]

Follow the coding conventions, e.g.:

static inline uint8_t
contains_cipher_suite(...args...) {

(The function name starts on its own line, the opening brace is on the same line as the last formal parameter.)

[mrdeep1]

As a comment, 74 of the git tracked files have lines that end in white space. Things code wise need to be stable before this is changed.

It can get a bit annoying, but I just now

$ cp .git/hooks/pre-commit.sample .git/hooks/pre-commit

which will then trap any files I am trying to commit with whitespace errors (have been doing this for some time in libcoap)

[boaks]

In my experience, it easier to do such cleanup on a "code-freeze" period. Even if I don't know, when we will be ready with the pending PRs :-).

[boaks]

Discussing the use/design of macros in PR #217 showed, that we also need some rules for macros. Any proposals?

[mrdeep1]

Any passed-in parameters must be wrapped with ( ) within the the macro definition as the passed-in parameter could be more than just a single variable (e.g. operation on the variable, or sum of 2 variables).

[boaks]

Any macro definition of "calculated values" must also be wrapped with ( ).

I'm not sure, if the rules should only be applied to API macros or also to internal macros.

[mrdeep1]

It should be a matter of principle for both API and internal only macros.

[huelsenfruchtzwerg]

Some pain points/things that could be cleaned up from my experience from using tinydtls on zephyr:

• the platform abstraction is quite messy and not very flexible right now, making it e.g. impossible to use tinydtls without malloc unless you're running on contiki for some reason. some api modifications (e.g. make the user allocate the context himself) + splitting platform abstractions out in their own conditionally included header (with the option for the user to provide his own platform header and implementation) would be helpful here.
• read and write being handled via a callback makes it hard to reason about when they will actually be called/the data will be available without checking the code, and requires some trickery to find out the correct context to pass the data. handling these as return values (since they in fact will only ever be invoked when calling into dtls, e.g. from dtls_handle_message) would simplify things a lot
• minor, but quite fresh: get_ecdsa_key being a callback and expecting it to allocate the memory for the key can be impractical/require unnecessary static memory allocations. Since the key is only ever required at two points and only in that functions scope, it would (for my specific usecase) at least be better if it was allocated on the stack in tinydtls. ideally it would support both ways.

Addressing these would require some quite extensive changes and break API, so I'd understand if this goes beyond the scope of your planned cleanup. FWIW I'd be interested in tackling these if there's interest.

[boaks]

Thanks for your message.

For now tinydtls uses several mechanisms for platform abstraction and none consequently. That needs a cleanup. But for also now my feeling is, that we need to focus on the pending PRs and smaller polish stuff.

About the read/write handling. One reason for that API is, that in some cases during the handshake, one call to "dtls_handle_message" may cause several calls to the write API for several handshake messages.
The correct context should be defined with the arguments of that API.

  int (*write)(struct dtls_context_t *ctx,  session_t *session, uint8 *buf, size_t len);

The dtls_context_t offers an app pointer for your application context.

About get_ecdsa_key. Since two weeks I also work more on using it. I guess, the point for that decision was, that the keys need anyway to be stored/kept somewhere. And the struct dtls_ecdsa_key_t to which a pointer to is returned, is a set of pointers (to the keys), not the keys itself. I also guess, it's easier to pass a pointer to that struct in and have that passed struct filled, than passing back a point to the static struct. But I need to check for the consequences.

Anyway, though you use zephyr, I guess I should contribute a small update about the includes.

[huelsenfruchtzwerg]

The dtls_context_t offers an app pointer for your application context.

The problem is when the required context is on the stack (e.g. receiving a msg and wanting to decode it to a buffer on the stack or passed in from somewhere else as a pointer again - e.g. when building a socket abstraction on top of tinydtls), then you need to update your app context to point to your specific buffer every time. It works, but is quite confusing to parse and not very intuitive.

About the read/write handling. One reason for that API is, that in some cases during the handshake, one call to "dtls_handle_message" may cause several calls to the write API for several handshake messages.

I see. But that could also be handled by a return value like EAGAIN, iirc that's also how e.g. mbedtls does it.

I guess, the point for that decision was, that the keys need anyway to be stored/kept somewhere.

Not necessarily in a way that can be used directly with tinydtls though (e.g. in zephyrs tls_credential subsystem). So instead of storing a copy statically somewhere you could only load it on demand to the stack for the two times it's required. But yeah, as I mentioned, this is pretty minor.

[boaks]

The problem is when the required context is on the stack

Yes, that case isn't a easy one with the current API.

Not necessarily in a way that can be used directly with tinydtls though (e.g. in zephyrs tls_credential subsystem).

Yes, supporting such an API is also not easy.

Let's try to get the PRs done and see, what we can agree with the other committers.

[boaks]

get_ecdsa_key

I spend some time in investigation to overcome that. On the server-side it is possible to pass pointer to the buffers in dtls_handshake_parameters_ecdsa_t (the eph stuff is used a step later, so other_eph_pub_y for priv_key and the other_pub_x/other_pub_y for pub_key_x/pub_key_y). On the client-side the other_ are already filled. The other_pub_x/other_pub_y may be reused and so it requires only 32 bytes more on the stack.

@obgm @mrdeep1

What do you think? The current design of get_ecdsa_key differs from that of get_psk_info, but changing it, will break the API.

[mrdeep1]

If the API is changed, with the potential of different versions of TinyDTLS to dynamically link against would be a nightmare for me (with libcoap) without having an API version change away from 0.8.6 that can be picked up in the PACKAGE_VERSION variable.

Some devices have constrained stack sizes, but I don't think an extra 32 bytes should be a significant issue.

For GnuTLS and wolfSSL, functions are called to define the RPK keys from user application, rather than having them acquired using a callback handler.

[boaks]

Maybe, adding an second "callback" as extension and document to migrate?

(I guess, the callbacks are used in order to keep the data in the library only for a short time.)

[mrdeep1]

Migrating document is not useful for me as there may be an existing libtinydtls.so file that could be pre or post this API change on system A where the application executable was built on a different server and then installed on system A via some package manager.

[boaks]

an second "callback" as extension

that should not affect existing code.

[mrdeep1]

that should not affect existing code.

Adding an extra callback to dtls_handler_t(which will have to be at the end so as not to disturb the binary API) will have a random value in any pre addition of callback compiled external code. So, the new tinydtls code cannot check for the presence of this callback or not. For the app, there needs to be a run-time indication of some sort as to what is the actual tinydtls version is using something like dtls_package_version(), or the app needs to tell tinydtls that this functionality is supported or not.