posix: Implement mq_notify() #67988

awojasinski · 2024-01-23T09:25:23Z

The function was the last missing piece of the _POSIX_MESSAGE_PASSING
option group. Due to lack of signal subsystem in the Zephyr RTOS
the sigev_notify member of the sigevent structure that describes
the notification cannot be set to SIGEV_SIGNAL - this notification
type is not implemented, the function will return -1 and set errno
to ENOSYS.

mq_notify documentation:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_notify.html

Fixes #66958

ycsin

try not to modify unrelated lines, with the right config I think you can format just the modified lines in vscode

awojasinski · 2024-01-23T09:38:44Z

try not to modify unrelated lines, with the right config I think you can format just the modified lines in vscode

@ycsin just tried to fix some formatting issues but don't need to do that

ycsin · 2024-01-23T10:36:51Z

Looks like you need a rebase

doc/services/portability/posix/option_groups/index.rst

cfriedt · 2024-01-23T18:42:30Z

tests/posix/common/src/mqueue.c

+	struct mq_attr attrs;
+	struct sigevent not;
+	int32_t mode = 0777;
+	int flags = O_RDWR | O_CREAT;
+
+	attrs.mq_msgsize = MESSAGE_SIZE;
+	attrs.mq_maxmsg = MESG_COUNT_PERMQ;
+
+	not.sigev_notify_function = notify_function_basic;
+	not.sigev_notify_attributes = NULL;
+	not.sigev_notify = SIGEV_SIGNAL;
+	not.sigev_value.sival_ptr = (void *)&notification_executed;


Not a blocker, but attrs and not could be initialized where they are declared (we support C11 by default, so no need to use only C89 declarations). It might be a bit less verbose, but also, there would be no need to initialize anything to 0 / NULL explicitly.

Suggested change

struct mq_attr attrs;

struct sigevent not;

int32_t mode = 0777;

int flags = O_RDWR | O_CREAT;

attrs.mq_msgsize = MESSAGE_SIZE;

attrs.mq_maxmsg = MESG_COUNT_PERMQ;

not.sigev_notify_function = notify_function_basic;

not.sigev_notify_attributes = NULL;

not.sigev_notify = SIGEV_SIGNAL;

not.sigev_value.sival_ptr = (void *)&notification_executed;

struct mq_attr attrs = {

.mq_msgsize = MESSAGE_SIZE,

.mq_maxmsg = MESG_COUNT_PERMQ,

};

struct sigevent not = {

.sigev_notify_function = notify_function_basic,

.sigev_notify = SIGEV_SIGNAL,

.sigev_value.sival_ptr = (void *)&notification_executed,

};

int32_t mode = 0777;

int flags = O_RDWR | O_CREAT;

cfriedt · 2024-01-23T18:44:37Z

tests/posix/common/src/mqueue.c

+	mqd_t mqd;
+	struct mq_attr attrs;
+	struct sigevent not;
+	int32_t mode = 0777;
+	int flags = O_RDWR | O_CREAT;
+
+	attrs.mq_msgsize = MESSAGE_SIZE;
+	attrs.mq_maxmsg = MESG_COUNT_PERMQ;
+
+	not.sigev_notify = SIGEV_THREAD;
+	not.sigev_value.sival_int = (int)pthread_self();
+	not.sigev_notify_function = notify_function_thread;
+	not.sigev_notify_attributes = NULL;


Similar - again, not blocking, so if this does need a respin for any reason, it might be worth it.

Suggested change

mqd_t mqd;

struct mq_attr attrs;

struct sigevent not;

int32_t mode = 0777;

int flags = O_RDWR | O_CREAT;

attrs.mq_msgsize = MESSAGE_SIZE;

attrs.mq_maxmsg = MESG_COUNT_PERMQ;

not.sigev_notify = SIGEV_THREAD;

not.sigev_value.sival_int = (int)pthread_self();

not.sigev_notify_function = notify_function_thread;

not.sigev_notify_attributes = NULL;

mqd_t mqd;

struct mq_attr attrs = {

.mq_msgsize = MESSAGE_SIZE,

.mq_maxmsg = MESG_COUNT_PERMQ,

};

struct sigevent not = {

.sigev_notify = SIGEV_THREAD,

.sigev_value.sival_int = (int)pthread_self(),

.sigev_notify_function = notify_function_thread,

};

int32_t mode = 0777;

int flags = O_RDWR | O_CREAT;

cfriedt · 2024-01-23T18:46:20Z

tests/posix/common/src/mqueue.c

+	mqd_t mqd;
+	struct mq_attr attrs;
+	struct sigevent not;
+	int32_t mode = 0777;
+	int flags = O_RDWR | O_CREAT;
+
+	attrs.mq_msgsize = MESSAGE_SIZE;
+	attrs.mq_maxmsg = MESG_COUNT_PERMQ;
+
+	not.sigev_notify = SIGEV_NONE;
+	not.sigev_value.sival_ptr = (void *)&notification_executed;
+	not.sigev_notify_function = notify_function_basic;
+	not.sigev_notify_attributes = NULL;


Same - just a suggestion, not blocking, so you can ignore if there is no respin required.

Suggested change

mqd_t mqd;

struct mq_attr attrs;

struct sigevent not;

int32_t mode = 0777;

int flags = O_RDWR | O_CREAT;

attrs.mq_msgsize = MESSAGE_SIZE;

attrs.mq_maxmsg = MESG_COUNT_PERMQ;

not.sigev_notify = SIGEV_NONE;

not.sigev_value.sival_ptr = (void *)&notification_executed;

not.sigev_notify_function = notify_function_basic;

not.sigev_notify_attributes = NULL;

mqd_t mqd;

struct mq_attr attrs = {

.mq_msgsize = MESSAGE_SIZE,

.mq_maxmsg = MESG_COUNT_PERMQ,

};

struct sigevent not = {

.sigev_notify = SIGEV_NONE,

.sigev_value.sival_ptr = (void *)&notification_executed,

.sigev_notify_function = notify_function_basic,

};

int32_t mode = 0777;

int flags = O_RDWR | O_CREAT;

cfriedt · 2024-01-23T18:47:20Z

tests/posix/common/src/mqueue.c

@@ -97,4 +96,205 @@ ZTEST(mqueue, test_mqueue)
 	zassert_false(mq_unlink(queue), "Not able to unlink Queue");
 }

+bool notification_executed;


Not blocking either, so only if a respin is needed.

Suggested change

bool notification_executed;

static bool notification_executed;

cfriedt · 2024-01-23T18:52:42Z

lib/posix/mqueue.c

+	mqueue_object *mqueue = (mqueue_object *)arg;
+	struct sigevent *sevp = &mqueue->not;
+
+	pthread_detach(pthread_self());


Aside from calling pthread_detach(), I think it might be necessary to use pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL). Otherwise, calling pthread_cancel() might have no (immediate) effect. It's a bit confusing, but asynchronous (in pthreads) means with respect to cancellation points.

#67871 for reference, since there was a similar recent issue in timer.c

It's kind of tricky as well, because calling pthread_detach() on an already detached thread is undefined behaviour which might eventually result in an assertion.

To avoid that, it would be a good idea to call pthread_attr_setdetachstate() in mq_notify() when sigev_notify_attributes is not NULL. Otherwise, in mq_notify_thread(), call pthread_detach() only if sevp->sigev_notify_attributes is NULL.

tests/posix/common/src/mqueue.c

cfriedt

No need to check the attr detachstate before setting it - otherwise basically done 👍

cfriedt · 2024-01-25T01:01:38Z

lib/posix/mqueue.c

+		ret = pthread_attr_getdetachstate(notification->sigev_notify_attributes,
+						  &detachstate);
+		if (ret != 0) {
+			errno = ret;
+			return -1;
+		}
+		if (detachstate != PTHREAD_CREATE_DETACHED) {


No need to check what the attr detachstate is before setting it

Suggested change

ret = pthread_attr_getdetachstate(notification->sigev_notify_attributes,

&detachstate);

if (ret != 0) {

errno = ret;

return -1;

}

if (detachstate != PTHREAD_CREATE_DETACHED) {

ret = pthread_attr_getdetachstate(notification->sigev_notify_attributes,

&detachstate);

if (ret != 0) {

errno = ret;

return -1;

}

if (detachstate != PTHREAD_CREATE_DETACHED) {

That's the way posix/timer.c do it so maybe it should be changed there as well?

cfriedt · 2024-01-25T01:01:56Z

lib/posix/mqueue.c

+				errno = ret;
+				return -1;
+			}
+		}


Suggested change

}

tests/posix/common/src/mqueue.c

The function was the last missing piece of the `_POSIX_MESSAGE_PASSING` option group. Due to lack of signal subsystem in the Zephyr RTOS the `sigev_notify` member of the `sigevent` structure that describes the notification cannot be set to `SIGEV_SIGNAL` - this notification type is not implemented, the function will return -1 and set `errno` to `ENOSYS`. `mq_notify` documentation: https://pubs.opengroup.org/onlinepubs/9699919799/functions/mq_notify.html Fixes zephyrproject-rtos#66958 Signed-off-by: Adam Wojasinski <[email protected]>

cfriedt

LGTM - thanks @awojasinski 👍

tests/posix/common/src/mqueue.c

Adds test cases testing following features: - function error handling - basic notification type - thread notification type - adding notification to non-empty queue Signed-off-by: Adam Wojasinski <[email protected]>

Updates in documentation support for `mq_notify` API in `_POSIX_MESSAGE_PASSING` group option. Signed-off-by: Adam Wojasinski <[email protected]>

ycsin

cfriedt · 2024-01-25T18:43:01Z

@awojasinski - if you're feeling generous, we could also use mq_timedreceive() and mq_timedsend() for the _POSIX_TIMEOUTS subprofiling option 😃

awojasinski · 2024-01-26T07:40:44Z

@awojasinski - if you're feeling generous, we could also use mq_timedreceive() and mq_timedsend() for the _POSIX_TIMEOUTS subprofiling option 😃

@cfriedt sure I'll take a look in my spare time

zephyrbot added the area: POSIX POSIX API Library label Jan 23, 2024

zephyrbot requested review from cfriedt and ycsin January 23, 2024 09:25

zephyrbot assigned cfriedt Jan 23, 2024

ycsin requested changes Jan 23, 2024

View reviewed changes

awojasinski force-pushed the posix_mqnotify branch from a2ef878 to 427b84b Compare January 23, 2024 09:34

awojasinski force-pushed the posix_mqnotify branch from 427b84b to c15e5fe Compare January 23, 2024 09:46

awojasinski force-pushed the posix_mqnotify branch 2 times, most recently from 514134a to 579e0cd Compare January 23, 2024 13:19

awojasinski requested a review from ycsin January 23, 2024 15:29

cfriedt reviewed Jan 23, 2024

View reviewed changes

doc/services/portability/posix/option_groups/index.rst Show resolved Hide resolved

awojasinski force-pushed the posix_mqnotify branch from 579e0cd to 3c861d4 Compare January 23, 2024 18:20

cfriedt reviewed Jan 23, 2024

View reviewed changes

tests/posix/common/src/mqueue.c Outdated Show resolved Hide resolved

awojasinski force-pushed the posix_mqnotify branch from 3c861d4 to 561cbee Compare January 24, 2024 08:35

cfriedt reviewed Jan 25, 2024

View reviewed changes

ycsin requested changes Jan 25, 2024

View reviewed changes

awojasinski force-pushed the posix_mqnotify branch from 561cbee to 2fa4e3e Compare January 25, 2024 07:57

awojasinski requested a review from ycsin January 25, 2024 09:24

cfriedt previously approved these changes Jan 25, 2024

View reviewed changes

ycsin previously approved these changes Jan 25, 2024

View reviewed changes

tests/posix/common/src/mqueue.c Outdated Show resolved Hide resolved

awojasinski added 2 commits January 25, 2024 15:14

tests: posix: Add test cases for mq_notify()

3168c5d

Adds test cases testing following features: - function error handling - basic notification type - thread notification type - adding notification to non-empty queue Signed-off-by: Adam Wojasinski <[email protected]>

doc: posix: Update POSIX supported API documentation

21bae31

Updates in documentation support for `mq_notify` API in `_POSIX_MESSAGE_PASSING` group option. Signed-off-by: Adam Wojasinski <[email protected]>

awojasinski dismissed stale reviews from ycsin and cfriedt via 21bae31 January 25, 2024 14:14

awojasinski force-pushed the posix_mqnotify branch from 2fa4e3e to 21bae31 Compare January 25, 2024 14:14

ycsin approved these changes Jan 25, 2024

View reviewed changes

cfriedt approved these changes Jan 25, 2024

View reviewed changes

carlescufi merged commit 2e7a1f9 into zephyrproject-rtos:main Jan 25, 2024

awojasinski deleted the posix_mqnotify branch January 26, 2024 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

posix: Implement mq_notify() #67988

posix: Implement mq_notify() #67988

awojasinski commented Jan 23, 2024

ycsin left a comment

awojasinski commented Jan 23, 2024

ycsin commented Jan 23, 2024

cfriedt Jan 23, 2024

cfriedt Jan 23, 2024

cfriedt Jan 23, 2024

cfriedt Jan 23, 2024

cfriedt Jan 23, 2024 •

edited

Loading

cfriedt left a comment

cfriedt Jan 25, 2024

awojasinski Jan 25, 2024

cfriedt Jan 25, 2024

cfriedt left a comment

ycsin left a comment •

edited

Loading

cfriedt commented Jan 25, 2024

awojasinski commented Jan 26, 2024

	bool notification_executed;
	static bool notification_executed;

posix: Implement mq_notify() #67988

posix: Implement mq_notify() #67988

Conversation

awojasinski commented Jan 23, 2024

ycsin left a comment

Choose a reason for hiding this comment

awojasinski commented Jan 23, 2024

ycsin commented Jan 23, 2024

cfriedt Jan 23, 2024

Choose a reason for hiding this comment

cfriedt Jan 23, 2024

Choose a reason for hiding this comment

cfriedt Jan 23, 2024

Choose a reason for hiding this comment

cfriedt Jan 23, 2024

Choose a reason for hiding this comment

cfriedt Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

cfriedt left a comment

Choose a reason for hiding this comment

cfriedt Jan 25, 2024

Choose a reason for hiding this comment

awojasinski Jan 25, 2024

Choose a reason for hiding this comment

cfriedt Jan 25, 2024

Choose a reason for hiding this comment

cfriedt left a comment

Choose a reason for hiding this comment

ycsin left a comment • edited Loading

Choose a reason for hiding this comment

cfriedt commented Jan 25, 2024

awojasinski commented Jan 26, 2024

cfriedt Jan 23, 2024 •

edited

Loading

ycsin left a comment •

edited

Loading