Created attachment 75191 [details] [review] Add support for systems without syslog.h I've been working on porting D-bus to the QNX operating system. In addition to the previously-filed bugs #60339 and #60340, I have just a couple more minor patches that are necessary to make it work. In retrospect, it would probably have been better to just file all of this under one bug, instead of splitting it out across multiple tickets. I'll at least post my remaining patches together here. * The first patch just adds a HAVE_SYSLOG_H variable around the logging code, since there is no syslog.h on QNX. This leaves the QNX port without any logging, but at least the code builds and runs. * The second fixes a couple problematic header includes. The sys/fcntl.h -> fcntl.h one should be pretty uncontroversial. The stdint.h-before-inotify.h one is really gross, though, and I apologize for inflicting such an awful thing on your codebase. I'm not really sure what else to do, though--QNX's copy of inotify.h is literally just broken, and will fail to compile unless you've included stdint.h beforehand. If anyone has a better idea of how to deal with this, I'm all ears. * The final point is one I'll need some input on. QNX has the ability to pass file descriptors across Unix-domain sockets just fine, but it seems to impose some arbitrary size limitations when doing so. Specifically, if the cmsg_control field is longer than 2016 bytes, then a call to recvmsg() will just outright fail with EMSGSIZE. This means that the current behavior, which allocates space for 1024 file descriptors in dbus/dbus-message.c and bus/config-parser.c, will prevent socket calls from working on QNX, because that causes the control field to be too big. I can re-hardcode the numbers down to something like 256, and then everything works fine, but this obviously isn't ideal. Would there be some way to make these limits a bit more easily-configurable, so that different OS'es can specify different limits? Or should I just put a big ugly #ifdef __QNX__ in there?
Created attachment 75192 [details] [review] Header fixes for QNX
Comment on attachment 75192 [details] [review] Header fixes for QNX Review of attachment 75192 [details] [review]: ----------------------------------------------------------------- This should be two patches. Whenever you write a commit message with bullet points in, ask yourself whether that's the case :-) ::: bus/dir-watch-inotify.c @@ +29,5 @@ > #include <fcntl.h> > +#ifdef __QNX__ > +/* QNX's inotify is broken, and requires stdint.h to be manually included first */ > +#include <stdint.h> > +#endif Ugh... but OK I suppose. Actually, I'd prefer this to be unconditional, although still with the comment. I think it would be worth adding a comment /* Be careful, this file is not Linux-only: QNX also uses it */ since until 5 minutes ago, I had no idea any other platform had inotify... ::: dbus/sd-daemon.c @@ +32,4 @@ > #include <sys/stat.h> > #include <sys/socket.h> > #include <sys/un.h> > +#include <fcntl.h> This part needs to go upstream to systemd, which is where that file comes from; then when it has been merged, ask me to update the entire file from systemd. It's one of the few files in systemd that's designed to be portable. I was slightly concerned about <fcntl.h> being portable, but we use it from other files already, so, fine.
Comment on attachment 75191 [details] [review] Add support for systems without syslog.h Review of attachment 75191 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +764,5 @@ > dnl needed on darwin for NAME_MAX > AC_CHECK_HEADERS(sys/syslimits.h) > > +dnl QNX doesn't have syslog.h > +AC_CHECK_HEADERS(syslog.h) We already have an AC_CHECK_HEADERS for syslog.h, since commit a4e9dc67, so this part should be unnecessary. ::: dbus/dbus-sysdeps-util-unix.c @@ +472,4 @@ > void > _dbus_system_logv (DBusSystemLogSeverity severity, const char *msg, va_list args) > { > +#ifdef HAVE_SYSLOG_H If you don't have a syslog.h, you should probably log to stderr instead, in the hope that that goes somewhere useful? I would suggest hoisting the vsyslog() call up, so it's something like this: #ifdef HAVE_SYSLOG_H int flags; switch (severity) ... } vsyslog (...); #endif #if !defined(HAVE_SYSLOG_H) || !HAVE_DECL_LOG_PERROR { /* ... use vfprintf ... */ } #endif if (...) exit (1); (Now that I look at that code, I notice commit e98107548 was incomplete: HAVE_DECL_LOG_PERROR is always defined to 0 or 1, despite the usual Autoconf convention of being defined to 1 or undefined, so the previous implementation of this function was wrong. Thanks, Autoconf. I think the version I suggest here is right.)
(In reply to comment #0) > QNX has the ability to > pass file descriptors across Unix-domain sockets just fine, but it seems to > impose some arbitrary size limitations when doing so. Ugh. > Would there be some way to make these limits a bit > more easily-configurable, so that different OS'es can specify different > limits? Or should I just put a big ugly #ifdef __QNX__ in there? I believe the limit used by the dbus-daemon is what's in session.conf or system.conf (as appropriate), with the limits in the config parser being the defaults. The system bus currently uses those defaults, but: bus/session.conf.in: <limit name="max_message_unix_fds">4096</limit> D-Bus clients can change the limit with dbus_connection_set_max_message_unix_fds(), although the default is, again, 1024. The hard limit, DBUS_MAXIMUM_MESSAGE_UNIX_FDS, is defined by the protocol (and is ridiculously large, about 10**9 - you'll never actually reach it). You could perhaps define DBUS_DEFAULT_MESSAGE_UNIX_FDS from configure.ac, setting it to 1024 on normal platforms and 256 on QNX? Something like this: AS_CASE([$host_os], [qnx*], [default_message_unix_fds=256], [*], [default_message_unix_fds=1024]) AC_DEFINE_UNQUOTED([DBUS_DEFAULT_MESSAGE_UNIX_FDS], [$default_message_unix_fds], [Default for dbus_connection_get_max_message_unix_fds()]) If you do that, make sure it gets a value under CMake as well. I don't mind if cmake/config.h.cmake just hard-codes "#define DBUS_DEFAULT_MESSAGE_UNIX_FDS 1024", I don't think anyone is likely to use the CMake build system on weird platforms (it's mostly there for Windows' benefit).
(In reply to comment #4) > bus/session.conf.in: > <limit name="max_message_unix_fds">4096</limit> I wouldn't object to setting this to @default_message_unix_fds@ too (you'd have to AC_SUBST it if you do that) - the session bus configuration is designed to be "nearly unlimited", but anyone who's sending more than 1024 fds in a single message is doing something very strange indeed. Or you could make this a separate parameter, I suppose.
Created attachment 75198 [details] [review] Fix inotify usage for QNX Yeah I was as surprised as you to find inotify.h on there--it looks like they just lifted the entire API and implemented it on their system. Would have been nice if they'd gotten the header includes right, but oh well. I've split out the inotify part of that header patch, and made the changes you mentioned. I'll submit the systemd part of the change to that project (although I doubt anybody's actually going to be using systemd on QNX, we might as well get it right anyway).
Comment on attachment 75198 [details] [review] Fix inotify usage for QNX Review of attachment 75198 [details] [review]: ----------------------------------------------------------------- Seems reasonable, I'll apply it soon.
(In reply to comment #6) > I'll submit the systemd part of the change to that project > (although I doubt anybody's actually going to be using systemd on QNX, we > might as well get it right anyway). systemd itself is explicitly Linux-only, but sd-daemon.h is meant to be portable. I notice that in master, we've updated that file, and it's now: #ifdef __BIONIC__ #include <linux/fcntl.h> #else #include <sys/fcntl.h> #endif From <http://lists.freedesktop.org/archives/intel-gfx/2012-February/015070.html> it appears that Android (Bionic libc) also has <fcntl.h>; POSIX requires it, and dbus (which is fairly portable) requires it in other Unix-specific files. So, this ugly conditional can probably be dropped in favour of doing the right thing. (Yay!)
Where are you generating patches relative to? For a port to an operating system where D-Bus has clearly never been tested before, I think I'd prefer it if you based things on master (which will be 1.7.0 when I get round to releasing it).
Created attachment 75210 [details] [review] Add support for systems without syslog.h
Created attachment 75211 [details] [review] Make default number of Unix fds configurable
I had been working off of master, but the checkout was a couple weeks old. I've reset to today's tip for these patches. Actually, dbus apparently has been used on QNX before--some contacts here tell me that Harman Inc. used it quite heavily on their QNX-based automotive head units. I found a bunch of old mailing list posts by a guy there named Glenn Schmottlach, who clearly got it working, although I couldn't ever find any patches posted, so all of these patches are my own attempts to fix the build/runtime errors that I've come across. I haven't yet done super-extensive testing, so you may be seeing me again, but these patches seem to be sufficient to at least get it building and running in a basic capacity.
(In reply to comment #11) > Make default number of Unix fds configurable This patch doesn't touch session.conf.in. Does that mean that "dbus-daemon --session" is unusable on QNX (because it can't receive the fds), unless you edit session.conf? Fixing that would probably be easiest done via AC_SUBST - session.conf is configure-generated. I don't think I really mind if the limit has to drop from 4096 to 1024 in the process - user processes on my Debian laptop are soft-limited to 1024 fds per process and hard-limited to 4096, and that's for the entire process, not just one message! I would expect messages with more than one fd to be vanishingly rare. Cc'ing Colin and Thiago to double-check that that functionality change is OK, though.
(In reply to comment #13) > (In reply to comment #11) > > Make default number of Unix fds configurable While I'm nitpicking that patch, I would be inclined to make the first line of the commit message something like "Set default maximum number of Unix fds according to OS", and make this change: - a hardcoded constant, to a variable which is determined at + a hardcoded constant, to a macro which is determined at (It's still a constant in C terms; what you're changing is that it becomes an OS-dependent constant.)
Comment on attachment 75198 [details] [review] Fix inotify usage for QNX Applied for 1.7.0, thanks.
Comment on attachment 75210 [details] [review] Add support for systems without syslog.h Applied for 1.7.0, thanks.
Created attachment 75323 [details] [review] Set default maximum number of Unix fds according to OS Thus far I have only been using the system bus (this is an embedded system with no clear concept of a session login), but you're right that it makes sense to get the session configuration fixed as well.
Just thought I'd ping this real quick. I believe my previously uploaded patch addressed the remaining concerns that you mentioned. Is there anything else necessary before this can go in?
Patch applied for 1.7.2, thanks. I believe this leaves: 1. The fd-passing stuff on Bug #60340 2. The fcntl.h in sd-daemon.h (I opened systemd Bug #63423) Anything else?
That should do it, as far as I can tell. Thanks for opening the systemd bug, BTW--I did indeed say that I would do that, and then promptly forgot all about it. :)
(In reply to comment #20) > That should do it, as far as I can tell. Closing, then.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.