Bug 61176

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.