mq on kqueue broken after upgrade to FreeBSD 11

[Lewis Donzis]

I had posted this on the userland forum, but it was suggested that I re-post it to this mailing list, so here goes…

We have applications that are using kevent() with, among other things, POSIX message queues, so they can wake up when data is ready.

As such, we were using __mq_oshandle() to get a handle from an mqd_t, suitable for passing to kevent().

However, this function appears to have been removed from publication in FreeBSD 11.0, although it still exists in the source. I saw some discussion about it being a mistake to have been available previously, but plenty of other __mq_*() functions are still public.

So the question is, how is one supposed to get a kevent() on a POSIX message queue, now that the fd is no longer available?

(I realize that one could argue that it was never supported, but it does (did) work, and why else would the kernel uipc_mqueue.c have code for supporting it? And that's not to mention that __mq_oshandle() is still declared in /usr/include/mqueue.h)

Thanks,
lew

[Konstantin Belousov]

I had posted this on the userland forum, but it was suggested that I re-post it to this mailing list, so here goes???
We have applications that are using kevent() with, among other things, POSIX message queues, so they can wake up when data is ready.
As such, we were using __mq_oshandle() to get a handle from an mqd_t, suitable for passing to kevent().
However, this function appears to have been removed from publication in FreeBSD 11.0, although it still exists in the source. I saw some discussion about it being a mistake to have been available previously, but plenty of other __mq_*() functions are still public.
So the question is, how is one supposed to get a kevent() on a POSIX message queue, now that the fd is no longer available?
(I realize that one could argue that it was never supported, but it does (did) work, and why else would the kernel uipc_mqueue.c have code for supporting it? And that's not to mention that __mq_oshandle() is still declared in /usr/include/mqueue.h)

Where was a discussion about the function presence being the mistake ?

In r291439, symbol versioning for librt was fixed, and apparently
__mq_oshandle() is not present in the global symbols list for librt.
I suspect that this is an erronous ommission, since the function'
declaration is present in the mqueue.h header and it is used by some
mqueue tests.

As such, I believe that exporting it is the intended option there.
The following patch should fix the problem for you.

I also exported __timer_oshandle, but there is no similar definition
in header.

diff --git a/lib/librt/Symbol.map b/lib/librt/Symbol.map
index 161bb76..5482488 100644
--- a/lib/librt/Symbol.map
+++ b/lib/librt/Symbol.map
@@ -18,11 +18,13 @@ FBSD_1.0 {
mq_unlink;
mq_send;
mq_receive;
+ __mq_oshandle;
timer_create;
timer_delete;
timer_gettime;
timer_settime;
timer_getoverrun;
+ __timer_oshandle;
};

FBSDprivate_1.0 {

[Lewis Donzis]

Post by Konstantin Belousov
Where was a discussion about the function presence being the mistake ?

I think it was here: https://lists.freebsd.org/pipermail/freebsd-current/2015-November/058706.html

which was just about a year ago. Perhaps I’m reading it wrong, but it seems like the implication is that removing the symbol from being exported was a "fix", where DE says "Why do the tests in tests/sys/mqueue/ try to use non-public APIs?" and then later, "symbol versioning for librt was broken and leaking symbols that shouldn't have been leaked."

Post by Konstantin Belousov
In r291439, symbol versioning for librt was fixed, and apparently
__mq_oshandle() is not present in the global symbols list for librt.
I suspect that this is an erronous ommission, since the function'
declaration is present in the mqueue.h header and it is used by some
mqueue tests.
As such, I believe that exporting it is the intended option there.
The following patch should fix the problem for you.

That makes sense, and appreciate the patch, but just to be clear, does your change get committed so that we won’t have to re-apply it after future updates/upgrades?

Thanks,
lew

[Konstantin Belousov]

Post by Lewis Donzis

Post by Konstantin Belousov
Where was a discussion about the function presence being the mistake ?

I think it was here: https://lists.freebsd.org/pipermail/freebsd-current/2015-November/058706.html
which was just about a year ago. Perhaps I???m reading it wrong, but it seems like the implication is that removing the symbol from being exported was a "fix", where DE says "Why do the tests in tests/sys/mqueue/ try to use non-public APIs?" and then later, "symbol versioning for librt was broken and leaking symbols that shouldn't have been leaked."

I added Daniel to Cc:. I think that the issue you referenced is somewhat
different. The r291439 commit restored symbol versioning, i.e. before it,
all symbols were accessible. Right now we are discussing the merits of
making one symbol accessible, which was removed from the export table as
a side effect of the fix. In other words, if at the time of r291439 the
symbol was present in the public export list, your code would not note
the fix.

Post by Lewis Donzis

Post by Konstantin Belousov
In r291439, symbol versioning for librt was fixed, and apparently
__mq_oshandle() is not present in the global symbols list for librt.
I suspect that this is an erronous ommission, since the function'
declaration is present in the mqueue.h header and it is used by some
mqueue tests.
As such, I believe that exporting it is the intended option there.
The following patch should fix the problem for you.

That makes sense, and appreciate the patch, but just to be clear, does your change get committed so that we won???t have to re-apply it after future updates/upgrades?

As I stated, my opinion is that this symbol can be usefully exported.
Its name is in implementation-private namespace, and there are uses
where access to the mqueue fd (or to the timer id) gives more flexibility
and significantly reduces the amount of code.

Unless there appear strong objections against the export, I will commit
the patch, sure.

[Lewis Donzis]

Post by Konstantin Belousov
Unless there appear strong objections against the export, I will commit
the patch, sure.

Thanks for the detailed explanation. Is there any chance that will be included in the final 11.0 build, or is it too late for that?

lew

[Alexander Kabaev]

On Fri, 30 Sep 2016 18:20:06 +0300

Post by Konstantin Belousov

Post by Lewis Donzis

On Sep 30, 2016, at 4:45 AM, Konstantin Belousov
function presence being the mistake ?

https://lists.freebsd.org/pipermail/freebsd-current/2015-November/058706.html
which was just about a year ago. Perhaps I???m reading it wrong,
but it seems like the implication is that removing the symbol from
being exported was a "fix", where DE says "Why do the tests in
tests/sys/mqueue/ try to use non-public APIs?" and then later,
"symbol versioning for librt was broken and leaking symbols that
shouldn't have been leaked."

I added Daniel to Cc:. I think that the issue you referenced is
somewhat different. The r291439 commit restored symbol versioning,
i.e. before it, all symbols were accessible. Right now we are
discussing the merits of making one symbol accessible, which was
removed from the export table as a side effect of the fix. In other
words, if at the time of r291439 the symbol was present in the public
export list, your code would not note the fix.

Post by Lewis Donzis

In r291439, symbol versioning for librt was fixed, and apparently
__mq_oshandle() is not present in the global symbols list for
librt. I suspect that this is an erronous ommission, since the
function' declaration is present in the mqueue.h header and it is
used by some mqueue tests.
As such, I believe that exporting it is the intended option there.
The following patch should fix the problem for you.

That makes sense, and appreciate the patch, but
just to be clear, does your change get committed so that we won???t
have to re-apply it after future updates/upgrades?

As I stated, my opinion is that this symbol can be usefully exported.
Its name is in implementation-private namespace, and there are uses
where access to the mqueue fd (or to the timer id) gives more
flexibility and significantly reduces the amount of code.
Unless there appear strong objections against the export, I will
commit the patch, sure.

No objection, but possible suggestion: if the primary use of this
symbol is for tests and nothing else, maybe it does belong in
FBSDprivate_1.0 FBSDprivate_1.0 section instead?

[Lewis Donzis]

Post by Alexander Kabaev
No objection, but possible suggestion: if the primary use of this
symbol is for tests and nothing else, maybe it does belong in
FBSDprivate_1.0 FBSDprivate_1.0 section instead?

The primary use for us is definitely not testing — it appears to be the only way to get a handle that can be used with kevent(). In that regard, it would be even nicer if it was a regular mq_*() function, even if it had to be non-portable, rather than beginning with “__” which makes it look internal only.

lew

[Konstantin Belousov]

Post by Alexander Kabaev
No objection, but possible suggestion: if the primary use of this
symbol is for tests and nothing else, maybe it does belong in
FBSDprivate_1.0 FBSDprivate_1.0 section instead?

Good question. The symbols are useful for real-world code, not only for
the tests. But I think that we should mark symbol as non-portable. Usual
approach of adding _np suffix seems to be the right thing to do there.

What about the following ?

diff --git a/include/mqueue.h b/include/mqueue.h
index 788d0a1..297e8d0 100644
--- a/include/mqueue.h
+++ b/include/mqueue.h
@@ -50,7 +50,9 @@ ssize_t mq_timedreceive(mqd_t, char *__restrict, size_t,
int mq_timedsend(mqd_t, const char *, size_t, unsigned,
const struct timespec *);
int mq_unlink(const char *);
-int __mq_oshandle(mqd_t mqd);
+#if __BSD_VISIBLE
+int mq_oshandle_np(mqd_t mqd);
+#endif /* __BSD_VISIBLE */

__END_DECLS
#endif
diff --git a/include/time.h b/include/time.h
index 14d6044..c172538 100644
--- a/include/time.h
+++ b/include/time.h
@@ -194,6 +194,7 @@ char *timezone(int, int); /* XXX XSI conflict */
void tzsetwall(void);
time_t timelocal(struct tm * const);
time_t timegm(struct tm * const);
+int timer_oshandle_np(timer_t timerid);
#endif /* __BSD_VISIBLE */

#if __POSIX_VISIBLE >= 200809 || defined(_XLOCALE_H_)
diff --git a/lib/librt/Symbol.map b/lib/librt/Symbol.map
index 161bb76..8fbca9c 100644
--- a/lib/librt/Symbol.map
+++ b/lib/librt/Symbol.map
@@ -25,6 +25,11 @@ FBSD_1.0 {
timer_getoverrun;
};

+FBSD_1.5 {
+ mq_oshandle_np;
+ timer_oshandle_np;
+};
+
FBSDprivate_1.0 {
_aio_read;
_aio_write;
@@ -56,6 +61,7 @@ FBSDprivate_1.0 {
__mq_unlink;
__mq_send;
__mq_receive;
+ __mq_oshandle_np;
_timer_create;
_timer_delete;
_timer_gettime;
@@ -66,4 +72,5 @@ FBSDprivate_1.0 {
__timer_gettime;
__timer_settime;
__timer_getoverrun;
+ __timer_oshandle_np;
};
diff --git a/lib/librt/mq.c b/lib/librt/mq.c
index 750e969..60704a4 100644
--- a/lib/librt/mq.c
+++ b/lib/librt/mq.c
@@ -78,6 +78,7 @@ __weak_reference(__mq_send_cancel, mq_send);
__weak_reference(__mq_send, _mq_send);
__weak_reference(__mq_receive_cancel, mq_receive);
__weak_reference(__mq_receive, _mq_receive);
+__weak_reference(__mq_oshandle_np, mq_oshandle_np);

mqd_t
__mq_open(const char *name, int oflag, mode_t mode,
@@ -273,7 +274,7 @@ __mq_unlink(const char *path)
}

int
-__mq_oshandle(mqd_t mqd)
+__mq_oshandle_np(mqd_t mqd)
{

return (mqd->oshandle);
diff --git a/lib/librt/timer.c b/lib/librt/timer.c
index 90269c2..fc1379a 100644
--- a/lib/librt/timer.c
+++ b/lib/librt/timer.c
@@ -63,6 +63,7 @@ __weak_reference(__timer_settime, timer_settime);
__weak_reference(__timer_settime, _timer_settime);
__weak_reference(__timer_getoverrun, timer_getoverrun);
__weak_reference(__timer_getoverrun, _timer_getoverrun);
+__weak_reference(__timer_oshandle_np, timer_oshandle_np);

typedef void (*timer_func)(union sigval val, int overrun);

@@ -176,7 +177,7 @@ __timer_settime(timer_t timerid, int flags,
}

int
-__timer_oshandle(timer_t timerid)
+__timer_oshandle_np(timer_t timerid)
{

return (timerid->oshandle);
diff --git a/tests/sys/mqueue/Makefile b/tests/sys/mqueue/Makefile
index ce5033c..251c497 100644
--- a/tests/sys/mqueue/Makefile
+++ b/tests/sys/mqueue/Makefile
@@ -10,8 +10,8 @@ CFLAGS+= -I${SRCTOP}/tests

PROGS+= mqtest1
PROGS+= mqtest2
-#PROGS+= mqtest3
-#PROGS+= mqtest4
+PROGS+= mqtest3
+PROGS+= mqtest4
PROGS+= mqtest5

LIBADD+= rt
diff --git a/tests/sys/mqueue/mqtest3.c b/tests/sys/mqueue/mqtest3.c
index c4b849e..7325572 100644
--- a/tests/sys/mqueue/mqtest3.c
+++ b/tests/sys/mqueue/mqtest3.c
@@ -62,9 +62,10 @@ main(void)
buf = malloc(attr.mq_msgsize);
for (j = 0; j < LOOPS; ++j) {
FD_ZERO(&set);
- FD_SET(__mq_oshandle(mq), &set);
+ FD_SET(mq_oshandle_np(mq), &set);
alarm(3);
- status = select(__mq_oshandle(mq)+1, &set, NULL, NULL, NULL);
+ status = select(mq_oshandle_np(mq) + 1, &set, NULL,
+ NULL, NULL);
if (status != 1)
err(1, "child process: select()");
status = mq_receive(mq, buf, attr.mq_msgsize, &prio);
@@ -94,8 +95,9 @@ main(void)
}
alarm(3);
FD_ZERO(&set);
- FD_SET(__mq_oshandle(mq), &set);
- status = select(__mq_oshandle(mq)+1, NULL, &set, NULL, NULL);
+ FD_SET(mq_oshandle_np(mq), &set);
+ status = select(mq_oshandle_np(mq) + 1, NULL, &set,
+ NULL, NULL);
if (status != 1)
err(1, "select()");
status = mq_send(mq, buf, attr.mq_msgsize, PRIO);
diff --git a/tests/sys/mqueue/mqtest4.c b/tests/sys/mqueue/mqtest4.c
index 474d212..fff04c0c 100644
--- a/tests/sys/mqueue/mqtest4.c
+++ b/tests/sys/mqueue/mqtest4.c
@@ -57,7 +57,7 @@ main(void)
mq = mq_open(MQNAME, O_RDWR);
if (mq == (mqd_t)-1)
err(1, "child: mq_open");
- EV_SET(&kev, __mq_oshandle(mq), EVFILT_READ, EV_ADD, 0, 0, 0);
+ EV_SET(&kev, mq_oshandle_np(mq), EVFILT_READ, EV_ADD, 0, 0, 0);
status = kevent(kq, &kev, 1, NULL, 0, NULL);
if (status == -1)
err(1, "child: kevent");
@@ -89,7 +89,7 @@ main(void)

signal(SIGALRM, sighandler);
kq = kqueue();
- EV_SET(&kev, __mq_oshandle(mq), EVFILT_WRITE, EV_ADD, 0, 0, 0);
+ EV_SET(&kev, mq_oshandle_np(mq), EVFILT_WRITE, EV_ADD, 0, 0, 0);
status = kevent(kq, &kev, 1, NULL, 0, NULL);
if (status == -1)
err(1, "kevent");

[Lewis Donzis]

Post by Konstantin Belousov

Post by Alexander Kabaev
No objection, but possible suggestion: if the primary use of this
symbol is for tests and nothing else, maybe it does belong in
FBSDprivate_1.0 FBSDprivate_1.0 section instead?

Good question. The symbols are useful for real-world code, not only for
the tests. But I think that we should mark symbol as non-portable. Usual
approach of adding _np suffix seems to be the right thing to do there.
What about the following ?
…

Not that I have any say in this, but that seems reasonable to me. The functions are relevant and necessary for anything that needs a handle for a POSIX message queue, so your change would be preferable to having them appear as internal functions. (And then they could be documented, too.)

Speaking of which, not sure if this is the appropriate place to post this, but the mq_open documentation needs a little tweaking, too.

mq_open(2)’s man page says:

SEE ALSO
mq_close(2), mq_getattr(2), mq_receive(2), mq_send(2), mq_setattr(2),
mq_timedreceive(3), mq_timedsend(3), mq_unlink(3), mqueuefs(5)

However, both mq_timedreceive and mq_timedsend are in section 2, not section 3.

And, mq_unlink should probably be in section 2, but in any event, that file appears to be missing.

lew

[Jilles Tjoelker]

Post by Konstantin Belousov

Post by Alexander Kabaev
No objection, but possible suggestion: if the primary use of this
symbol is for tests and nothing else, maybe it does belong in
FBSDprivate_1.0 FBSDprivate_1.0 section instead?

Good question. The symbols are useful for real-world code, not only for
the tests. But I think that we should mark symbol as non-portable. Usual
approach of adding _np suffix seems to be the right thing to do there.
What about the following ?

The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor. Also, the __ versions should not be
exported since they are not used outside the library (they can be
exported if and when needed).

[Konstantin Belousov]

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor. Also, the __ versions should not be
exported since they are not used outside the library (they can be
exported if and when needed).

diff --git a/include/mqueue.h b/include/mqueue.h
index 788d0a1..e1c0f27 100644
--- a/include/mqueue.h
+++ b/include/mqueue.h
@@ -50,7 +50,9 @@ ssize_t mq_timedreceive(mqd_t, char *__restrict, size_t,
int mq_timedsend(mqd_t, const char *, size_t, unsigned,
const struct timespec *);
int mq_unlink(const char *);
-int __mq_oshandle(mqd_t mqd);
+#if __BSD_VISIBLE
+int mq_getfd_np(mqd_t mqd);
+#endif /* __BSD_VISIBLE */

__END_DECLS
#endif
diff --git a/include/time.h b/include/time.h
index 14d6044..c172538 100644
--- a/include/time.h
+++ b/include/time.h
@@ -194,6 +194,7 @@ char *timezone(int, int); /* XXX XSI conflict */
void tzsetwall(void);
time_t timelocal(struct tm * const);
time_t timegm(struct tm * const);
+int timer_oshandle_np(timer_t timerid);
#endif /* __BSD_VISIBLE */

#if __POSIX_VISIBLE >= 200809 || defined(_XLOCALE_H_)
diff --git a/lib/librt/Symbol.map b/lib/librt/Symbol.map
index 161bb76..fef3c15 100644
--- a/lib/librt/Symbol.map
+++ b/lib/librt/Symbol.map
@@ -25,6 +25,11 @@ FBSD_1.0 {
timer_getoverrun;
};

+FBSD_1.5 {
+ mq_getfd_np;
+ timer_oshandle_np;
+};
+
FBSDprivate_1.0 {
_aio_read;
_aio_write;
diff --git a/lib/librt/mq.c b/lib/librt/mq.c
index 750e969..513fa72 100644
--- a/lib/librt/mq.c
+++ b/lib/librt/mq.c
@@ -272,8 +272,9 @@ __mq_unlink(const char *path)
return __sys_kmq_unlink(path);
}

+#pragma weak mq_getfd_np
int
-__mq_oshandle(mqd_t mqd)
+mq_getfd_np(mqd_t mqd)
{

return (mqd->oshandle);
diff --git a/lib/librt/timer.c b/lib/librt/timer.c
index 90269c2..b5f775c 100644
--- a/lib/librt/timer.c
+++ b/lib/librt/timer.c
@@ -175,8 +175,9 @@ __timer_settime(timer_t timerid, int flags,
flags, value, ovalue);
}

+#pragma weak timer_oshandle_np
int
-__timer_oshandle(timer_t timerid)
+timer_oshandle_np(timer_t timerid)
{

return (timerid->oshandle);
diff --git a/tests/sys/mqueue/Makefile b/tests/sys/mqueue/Makefile
index ce5033c..251c497 100644
--- a/tests/sys/mqueue/Makefile
+++ b/tests/sys/mqueue/Makefile
@@ -10,8 +10,8 @@ CFLAGS+= -I${SRCTOP}/tests

PROGS+= mqtest1
PROGS+= mqtest2
-#PROGS+= mqtest3
-#PROGS+= mqtest4
+PROGS+= mqtest3
+PROGS+= mqtest4
PROGS+= mqtest5

LIBADD+= rt
diff --git a/tests/sys/mqueue/mqtest3.c b/tests/sys/mqueue/mqtest3.c
index c4b849e..3e20c4d 100644
--- a/tests/sys/mqueue/mqtest3.c
+++ b/tests/sys/mqueue/mqtest3.c
@@ -62,9 +62,10 @@ main(void)
buf = malloc(attr.mq_msgsize);
for (j = 0; j < LOOPS; ++j) {
FD_ZERO(&set);
- FD_SET(__mq_oshandle(mq), &set);
+ FD_SET(mq_getfd_np(mq), &set);
alarm(3);
- status = select(__mq_oshandle(mq)+1, &set, NULL, NULL, NULL);
+ status = select(mq_getfd_np(mq) + 1, &set, NULL,
+ NULL, NULL);
if (status != 1)
err(1, "child process: select()");
status = mq_receive(mq, buf, attr.mq_msgsize, &prio);
@@ -94,8 +95,9 @@ main(void)
}
alarm(3);
FD_ZERO(&set);
- FD_SET(__mq_oshandle(mq), &set);
- status = select(__mq_oshandle(mq)+1, NULL, &set, NULL, NULL);
+ FD_SET(mq_getfd_np(mq), &set);
+ status = select(mq_getfd_np(mq) + 1, NULL, &set,
+ NULL, NULL);
if (status != 1)
err(1, "select()");
status = mq_send(mq, buf, attr.mq_msgsize, PRIO);
diff --git a/tests/sys/mqueue/mqtest4.c b/tests/sys/mqueue/mqtest4.c
index 474d212..b0b3952 100644
--- a/tests/sys/mqueue/mqtest4.c
+++ b/tests/sys/mqueue/mqtest4.c
@@ -57,7 +57,7 @@ main(void)
mq = mq_open(MQNAME, O_RDWR);
if (mq == (mqd_t)-1)
err(1, "child: mq_open");
- EV_SET(&kev, __mq_oshandle(mq), EVFILT_READ, EV_ADD, 0, 0, 0);
+ EV_SET(&kev, mq_getfd_np(mq), EVFILT_READ, EV_ADD, 0, 0, 0);
status = kevent(kq, &kev, 1, NULL, 0, NULL);
if (status == -1)
err(1, "child: kevent");
@@ -89,7 +89,7 @@ main(void)

signal(SIGALRM, sighandler);
kq = kqueue();
- EV_SET(&kev, __mq_oshandle(mq), EVFILT_WRITE, EV_ADD, 0, 0, 0);
+ EV_SET(&kev, mq_getfd_np(mq), EVFILT_WRITE, EV_ADD, 0, 0, 0);
status = kevent(kq, &kev, 1, NULL, 0, NULL);
if (status == -1)
err(1, "kevent");

[Jilles Tjoelker]

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor. Also, the __ versions should not be
exported since they are not used outside the library (they can be
exported if and when needed).

Please rename timer_oshandle_np() to timer_getfd_np() as well.

Looks ready to commit otherwise.

[Konstantin Belousov]

Post by Jilles Tjoelker

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor. Also, the __ versions should not be
exported since they are not used outside the library (they can be
exported if and when needed).

Please rename timer_oshandle_np() to timer_getfd_np() as well.

The timer handle is not an fd, so I kept the old name. Using
_getfd there is IMO more confusing than the neutral _handle for
mq.

Post by Jilles Tjoelker
Looks ready to commit otherwise.

I do not think that we need to provide compat symbols.

[Jilles Tjoelker]

Post by Konstantin Belousov

Post by Jilles Tjoelker

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor. Also, the __ versions should not be
exported since they are not used outside the library (they can be
exported if and when needed).

Please rename timer_oshandle_np() to timer_getfd_np() as well.

The timer handle is not an fd, so I kept the old name. Using
_getfd there is IMO more confusing than the neutral _handle for
mq.

Oh right, the timers are not file descriptors.

[Lewis Donzis]

Post by Jilles Tjoelker

Post by Konstantin Belousov

Post by Jilles Tjoelker

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor. Also, the __ versions should not be
exported since they are not used outside the library (they can be
exported if and when needed).

Please rename timer_oshandle_np() to timer_getfd_np() as well.

The timer handle is not an fd, so I kept the old name. Using
_getfd there is IMO more confusing than the neutral _handle for
mq.

Oh right, the timers are not file descriptors.

Technically, neither are mqueues. The only thing "the int" can be used for is select(), poll(), and kevent(). You can’t (or at least shouldn’t) pass it to any of the other system calls that accept fds.

lew

[Konstantin Belousov]

Technically, neither are mqueues. The only thing "the int" can be used for is select(), poll(), and kevent(). You can???t (or at least shouldn???t) pass it to any of the other system calls that accept fds.

Technically, mqueues are file descriptors. FWIW, allowed operations (in
the sense of doing something instead of returning errors) are, besides
polling, also stat, chmod, chown. They are enumerable as normal elements
of the process' file descriptor table, inherited on fork, and you can
and should close(2) them.

[Lewis Donzis]

Post by Konstantin Belousov

Technically, neither are mqueues. The only thing "the int" can be used for is select(), poll(), and kevent(). You can???t (or at least shouldn???t) pass it to any of the other system calls that accept fds.

Technically, mqueues are file descriptors. FWIW, allowed operations (in
the sense of doing something instead of returning errors) are, besides
polling, also stat, chmod, chown. They are enumerable as normal elements
of the process' file descriptor table, inherited on fork, and you can
and should close(2) them.

That's a very good point, I hadn't considered those other functions, and it’s clear that an fd is allocated and stored in the mqd_t. But using close() instead of mq_close() wouldn't delete the sigevent or free the memory that was allocated by mq_open(). In other words, I don't understand why you'd ever want/need to use close() on the underlying fd.

lew

[Konstantin Belousov]

Post by Lewis Donzis
That's a very good point, I hadn't considered those other functions,
and it???s clear that an fd is allocated and stored in the mqd_t.
But using close() instead of mq_close() wouldn't delete the sigevent
or free the memory that was allocated by mq_open(). In other words,
I don't understand why you'd ever want/need to use close() on the
underlying fd.

I really have troubles giving any useful interpretation to your question.
OS provides the kernel service which backs the posix message queue
implementation in userspace, as a file descriptor. To release resources
designated by the file descriptor, it must be closed, as in, close(2)
must be called. Librt does this in mq_close(3).

Why should I need to show a case of using close(2) on kernel mq
descriptor (perhaps besides librt) ? And how this changes or augments
the fact that kmq is file descriptor ?

[Lewis Donzis]

Post by Konstantin Belousov

Post by Lewis Donzis
That's a very good point, I hadn't considered those other functions,
and it???s clear that an fd is allocated and stored in the mqd_t.
But using close() instead of mq_close() wouldn't delete the sigevent
or free the memory that was allocated by mq_open(). In other words,
I don't understand why you'd ever want/need to use close() on the
underlying fd.

I really have troubles giving any useful interpretation to your question.
OS provides the kernel service which backs the posix message queue
implementation in userspace, as a file descriptor. To release resources
designated by the file descriptor, it must be closed, as in, close(2)
must be called. Librt does this in mq_close(3).
Why should I need to show a case of using close(2) on kernel mq
descriptor (perhaps besides librt) ? And how this changes or augments
the fact that kmq is file descriptor ?

Sorry, perhaps I wasn’t clear. As I read it, you suggested that you "can and should call close()" on the fd in an mqd_t, and my point was simply that such practice would obviously be bad because it would fail to release resources allocated by mq_open(). So yes, the correct way to close an mqueue is via mq_close(), i.e., we’re not meant to circumvent the librt functions.

The problem is that it’s not exactly clean that we have mq_*() functions for some operations, whereas for others, we’re required to convert the mqd_t to an fd. Of course, this is no fault of FreeBSD, and perhaps the POSIX folks should have come up with a better way to address this. For example, they could have specified a portable way to get the underlying fd from an mqd_t and specified what can and cannot be done with the fd.

lew

[Konstantin Belousov]

Sorry, perhaps I wasn???t clear. As I read it, you suggested that you
"can and should call close()" on the fd in an mqd_t, and my point was
simply that such practice would obviously be bad because it would
fail to release resources allocated by mq_open(). So yes, the correct
way to close an mqueue is via mq_close(), i.e., we???re not meant to
circumvent the librt functions.
The problem is that it???s not exactly clean that we have mq_*()
functions for some operations, whereas for others, we???re required to
convert the mqd_t to an fd. Of course, this is no fault of FreeBSD,
and perhaps the POSIX folks should have come up with a better way to
address this. For example, they could have specified a portable way to
get the underlying fd from an mqd_t and specified what can and cannot
be done with the fd.

POSIX does not and can not mandate the implementation. Existence of the
backing fd is pure implementation detail. Access to the fd through
mq_getfd_np() is some good will service from the implementation which
is supposed to make additional things possible besides the guarantees
of the POSIX spec.

kqueue(2) is not part of the POSIX, same as the use of mq for the
multiplexed wait (select/poll/kqueue). POSIX intent is that app
code get notifications by events.

[Alexander Kabaev]

On Sun, 2 Oct 2016 00:07:22 +0300

Post by Konstantin Belousov

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to
clarify it returns a file descriptor. Also, the __ versions should
not be exported since they are not used outside the library (they
can be exported if and when needed).

diff --git a/include/mqueue.h b/include/mqueue.h
index 788d0a1..e1c0f27 100644
--- a/include/mqueue.h
+++ b/include/mqueue.h
@@ -50,7 +50,9 @@ ssize_t mq_timedreceive(mqd_t, char
*__restrict, size_t, int mq_timedsend(mqd_t, const char *,
size_t, unsigned, const struct timespec *);
int mq_unlink(const char *);
-int __mq_oshandle(mqd_t mqd);
+#if __BSD_VISIBLE
+int mq_getfd_np(mqd_t mqd);
+#endif /* __BSD_VISIBLE */
__END_DECLS
#endif
diff --git a/include/time.h b/include/time.h
index 14d6044..c172538 100644
--- a/include/time.h
+++ b/include/time.h
@@ -194,6 +194,7 @@ char *timezone(int, int); /* XXX XSI
conflict */ void tzsetwall(void);
time_t timelocal(struct tm * const);
time_t timegm(struct tm * const);
+int timer_oshandle_np(timer_t timerid);
#endif /* __BSD_VISIBLE */
#if __POSIX_VISIBLE >= 200809 || defined(_XLOCALE_H_)
diff --git a/lib/librt/Symbol.map b/lib/librt/Symbol.map
index 161bb76..fef3c15 100644
--- a/lib/librt/Symbol.map
+++ b/lib/librt/Symbol.map
@@ -25,6 +25,11 @@ FBSD_1.0 {
timer_getoverrun;
};
+FBSD_1.5 {
+ mq_getfd_np;
+ timer_oshandle_np;
+};
+
FBSDprivate_1.0 {
_aio_read;
_aio_write;
diff --git a/lib/librt/mq.c b/lib/librt/mq.c
index 750e969..513fa72 100644
--- a/lib/librt/mq.c
+++ b/lib/librt/mq.c
@@ -272,8 +272,9 @@ __mq_unlink(const char *path)
return __sys_kmq_unlink(path);
}
+#pragma weak mq_getfd_np
int
-__mq_oshandle(mqd_t mqd)
+mq_getfd_np(mqd_t mqd)
{
return (mqd->oshandle);
diff --git a/lib/librt/timer.c b/lib/librt/timer.c
index 90269c2..b5f775c 100644
--- a/lib/librt/timer.c
+++ b/lib/librt/timer.c
@@ -175,8 +175,9 @@ __timer_settime(timer_t timerid, int flags,
flags, value, ovalue);
}
+#pragma weak timer_oshandle_np
int
-__timer_oshandle(timer_t timerid)
+timer_oshandle_np(timer_t timerid)
{
return (timerid->oshandle);
diff --git a/tests/sys/mqueue/Makefile b/tests/sys/mqueue/Makefile
index ce5033c..251c497 100644
--- a/tests/sys/mqueue/Makefile
+++ b/tests/sys/mqueue/Makefile
@@ -10,8 +10,8 @@ CFLAGS+= -I${SRCTOP}/tests
PROGS+= mqtest1
PROGS+= mqtest2
-#PROGS+= mqtest3
-#PROGS+= mqtest4
+PROGS+= mqtest3
+PROGS+= mqtest4
PROGS+= mqtest5
LIBADD+= rt
diff --git a/tests/sys/mqueue/mqtest3.c b/tests/sys/mqueue/mqtest3.c
index c4b849e..3e20c4d 100644
--- a/tests/sys/mqueue/mqtest3.c
+++ b/tests/sys/mqueue/mqtest3.c
@@ -62,9 +62,10 @@ main(void)
buf = malloc(attr.mq_msgsize);
for (j = 0; j < LOOPS; ++j) {
FD_ZERO(&set);
- FD_SET(__mq_oshandle(mq), &set);
+ FD_SET(mq_getfd_np(mq), &set);
alarm(3);
- status = select(__mq_oshandle(mq)+1, &set,
NULL, NULL, NULL);
+ status = select(mq_getfd_np(mq) + 1, &set,
NULL,
+ NULL, NULL);
if (status != 1)
err(1, "child process: select()");
status = mq_receive(mq, buf,
}
alarm(3);
FD_ZERO(&set);
- FD_SET(__mq_oshandle(mq), &set);
- status = select(__mq_oshandle(mq)+1, NULL,
&set, NULL, NULL);
+ FD_SET(mq_getfd_np(mq), &set);
+ status = select(mq_getfd_np(mq) + 1, NULL,
&set,
+ NULL, NULL);
if (status != 1)
err(1, "select()");
status = mq_send(mq, buf, attr.mq_msgsize,
PRIO); diff --git a/tests/sys/mqueue/mqtest4.c
b/tests/sys/mqueue/mqtest4.c index 474d212..b0b3952 100644
--- a/tests/sys/mqueue/mqtest4.c
+++ b/tests/sys/mqueue/mqtest4.c
@@ -57,7 +57,7 @@ main(void)
mq = mq_open(MQNAME, O_RDWR);
if (mq == (mqd_t)-1)
err(1, "child: mq_open");
- EV_SET(&kev, __mq_oshandle(mq), EVFILT_READ, EV_ADD,
0, 0, 0);
+ EV_SET(&kev, mq_getfd_np(mq), EVFILT_READ, EV_ADD,
0, 0, 0); status = kevent(kq, &kev, 1, NULL, 0, NULL);
if (status == -1)
err(1, "child: kevent");
@@ -89,7 +89,7 @@ main(void)
signal(SIGALRM, sighandler);
kq = kqueue();
- EV_SET(&kev, __mq_oshandle(mq), EVFILT_WRITE,
EV_ADD, 0, 0, 0);
+ EV_SET(&kev, mq_getfd_np(mq), EVFILT_WRITE, EV_ADD,
0, 0, 0); status = kevent(kq, &kev, 1, NULL, 0, NULL);
if (status == -1)
err(1, "kevent");

This looks good to me, thank you. If there is a desire to keep old
names exported, we can provide them as compat symbols too, so that old
binaries keep working. Software built from sources is better off
adapting new names which I think are quite a bit better.

[Lewis Donzis]

We had a discussion last October about __mq_oshandle() no longer being visible in FreeBSD 11, and I think the result was that it should be put back in the Symbol.map. There was also some discussion about renaming it.

At the moment (11.0-RELEASE-p10), the function is still missing from librt.so.1, but is present in librt.a. What seems odd is that running “make” in /usr/src/lib/librt produces a librt.so.1 that *does* have this function.

I’m curious as to how it’s possible that librt.a has the function but the released librt.so.1 does not have it, yet rebuilding from source does include the function?

Thanks,
lew

[Konstantin Belousov]

Post by Lewis Donzis
We had a discussion last October about __mq_oshandle() no longer being visible in FreeBSD 11, and I think the result was that it should be put back in the Symbol.map. There was also some discussion about renaming it.
At the moment (11.0-RELEASE-p10), the function is still missing from librt.so.1, but is present in librt.a. What seems odd is that running ???make??? in /usr/src/lib/librt produces a librt.so.1 that *does* have this function.
I???m curious as to how it???s possible that librt.a has the function but the released librt.so.1 does not have it, yet rebuilding from source does include the function?

The versioning on librt was broken some time ago, but I think it was fixed
before 11.0. Anyway, it works for me in HEAD and stable/11.
librt.a, as any static library, does not support versioning so any symbol
from the library can be accessed by linked code. This is one of the reasons
why static linking is discouraged.

It was decided that a symbol in the app-useable namespace must be officially
exported from librt, and we export mq_getfd_np() for your purposes. Again,
I am not sure was it done before 11.0 or after, but the symbol is definitely
present in stable/11 and in upcoming 11.1.

[Lewis Donzis]

Ah, yes, I see that it's in 11.0-STABLE, but I suppose it's not in 11.0-RELEASE.

Thanks for that, we’ll either update to STABLE or just wait another month for 11.1-RELEASE.

Thanks,
lew

Post by Konstantin Belousov

Post by Lewis Donzis
We had a discussion last October about __mq_oshandle() no longer being visible in FreeBSD 11, and I think the result was that it should be put back in the Symbol.map. There was also some discussion about renaming it.
At the moment (11.0-RELEASE-p10), the function is still missing from librt.so.1, but is present in librt.a. What seems odd is that running ???make??? in /usr/src/lib/librt produces a librt.so.1 that *does* have this function.
I???m curious as to how it???s possible that librt.a has the function but the released librt.so.1 does not have it, yet rebuilding from source does include the function?

The versioning on librt was broken some time ago, but I think it was fixed
before 11.0. Anyway, it works for me in HEAD and stable/11.
librt.a, as any static library, does not support versioning so any symbol
from the library can be accessed by linked code. This is one of the reasons
why static linking is discouraged.
It was decided that a symbol in the app-useable namespace must be officially
exported from librt, and we export mq_getfd_np() for your purposes. Again,
I am not sure was it done before 11.0 or after, but the symbol is definitely
present in stable/11 and in upcoming 11.1.

[Lewis Donzis]

Post by Jilles Tjoelker
The idea is good, but perhaps call the function mq_getfd_np() to clarify
it returns a file descriptor.

I have no objection, but for what it’s worth, there could be code out there that uses __mq_oshandle() simply because it’s been around for a while and has been the only way to use poll(), select(), or kevent() with an mqueue ever since an mqd_t changed from an integer to a pointer.

In our case, we have a half-dozen or so source files that reference __mq_oshandle(), but we don’t mind changing them if it’s better in the long run. I’m merely pointing out that it’s incompatible with previous FreeBSD versions.

Thanks,
lew

[Ngie Cooper]

Post by Konstantin Belousov

Post by Alexander Kabaev
No objection, but possible suggestion: if the primary use of this
symbol is for tests and nothing else, maybe it does belong in
FBSDprivate_1.0 FBSDprivate_1.0 section instead?

Good question. The symbols are useful for real-world code, not only for
the tests. But I think that we should mark symbol as non-portable. Usual
approach of adding _np suffix seems to be the right thing to do there.
What about the following ?

The change proposed below looks good to me, at first glance :)..

Thank you for proposing/doing this!

-Ngie