Compat: pthread.h shim for FreeRTOS #978
Open
+137
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
projectgoav
force-pushed
the
compat/freertos
branch
from
addd6f4
to
7c75b1e
Compare
botovq
reviewed
Dec 15, 2023
There was a problem hiding this comment.
I made a first pass over your diff and it looks pretty good. Most of my comments are very nitpicky and stylistic. The main points:
- Please make the
libressl_prefixing completely straightforward, so we change things this usingsed, as far as possible. - The pthread API is special in that it returns an
errnovalue, not-1(there's also a bug in the windows shim). - I am fine with using the
NO_SYSLOGlogic, but it needs fixing for WIN32 on the autotools level.
Comment on lines
113
to
123
| xSemaphoreHandle x = mutex->handle; | ||
| if (x) { | ||
| if ( xSemaphoreGive(x) == pdTRUE ) { | ||
| return 0; | ||
| } | ||
| else { | ||
| return -1; | ||
| } | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
Suggested change
| xSemaphoreHandle x = mutex->handle; | |
| if (x) { | |
| if ( xSemaphoreGive(x) == pdTRUE ) { | |
| return 0; | |
| } | |
| else { | |
| return -1; | |
| } | |
| } | |
| return 0; | |
| if (mutex->handle == NULL) | |
| return 0; | |
| if (xSemaphoreGive(mutex->handle) == pdTRUE) | |
| return 0; | |
| return -1; |
There was a problem hiding this comment.
In mutex_lock we:
- Return -1 if the mutex handle is NULL, here we return 0. Should we be consistent here and return -1 as well?
- We compare
!= pdTRUEand final return is the happy path. Here, we compare== pdTRUEand the final return is a non-happy path. Should we be consistent and!= pdTRUEwith happy path the final return?
| @@ -14,7 +18,7 @@ | |||
|
|
|||
| #include <stdarg.h> | |||
|
|
|||
| #ifdef _WIN32 | |||
| #ifdef NO_SYSLOG | |||
There was a problem hiding this comment.
This seems fine if we fix autotools.
| pthread_once(pthread_once_t *once, void (*cb) (void)) | ||
| { | ||
| if (!once->is_initialized) { | ||
| return -1; |
There was a problem hiding this comment.
I think all return -1 should actually be an errno, such as EINVAL. This seems also incorrect in the windows pthread shim. I have not corrected that in my suggestions below.
|
Hi @projectgoav do you have build / test instructions for this branch? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #960.
Assumes that anyone wishing to build for FreeRTOS will define
FREERTOSand ensure that the FreeRTOS headers are added to the include path atFreeRTOS/___pthread_once
I wasn't sure if this has to be thread-safe. Can easily adjust this if required.
Syslog Support
I had to adjust the
compat/syslog.hslighty to account for more than just Windows not providing syslog. Please let me know if there is a better/cleaner/more LibreSSLy way of doing things like this.I liked making sure of the already defined
NO_SYSLOGwhich is used as part ofcrypto/compat/r_syslog.chence why this was picked.Sockets / Networking
We currently use the LWIP networking stack. My plan was to submit another PR with a compatibility shim for that. As a result, I don't know the status of building for the built-in FreeRTOS stack. It's my understanding it follows the standard socket API so might just work (TM)...