Bug 34905 - using configure's --enable-abstract-sockets results in abstract sockets being disabled
Summary: using configure's --enable-abstract-sockets results in abstract sockets being...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-03-02 01:03 UTC by Karol Lewandowski
Modified: 2017-09-30 15:20 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] Fix abstract socket supports build checking (727 bytes, patch)
2013-12-07 09:28 UTC, Chengwei Yang
Details | Splinter Review
build: Remove unused substitution DBUS_PATH_OR_ABSTRACT (2.21 KB, patch)
2017-09-29 12:12 UTC, Simon McVittie
Details | Splinter Review
unix: Condition Linux-specific abstract sockets on __linux__ (9.56 KB, patch)
2017-09-29 12:13 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Karol Lewandowski 2011-03-02 01:03:32 UTC
configure.ac incorrectly handles --enable-abstract-sockets flag.

Decision if sockets shall be enabled is based on value of ac_cv_have_abstract_sockets which is unset when somebody passes the flag - it's  only set when enable_abstract_sockets == auto.

I don't grok m4 well, so I can't propose optimal solution.  However, surrounding above checks with sniplet below seems to work.

#### Abstract sockets

+ if test x$enable_abstract_sockets = xyes; then
+    ac_cv_have_abstract_sockets=yes
+ else

if test x$enable_abstract_sockets = xauto; then
AC_LANG_PUSH(C)

...

AC_LANG_POP(C)
fi

+ fi
Comment 1 Simon McVittie 2011-05-25 07:17:25 UTC
This is a bug, but I don't think that patch is right either: the right behaviour would be:

* --enable-abstract-sockets=auto (or no option): do the check; if
  supported, enable abstract sockets, else don't

* --enable-abstract-sockets[=yes]: do the check; if supported,
  enable abstract sockets, else configure fails and exits nonzero

* --enable-abstract-sockets=no or --disable-abstract-sockets: don't even
  bother with the check; unconditionally disable abstract sockets
Comment 2 Chengwei Yang 2013-12-07 09:28:42 UTC
Created attachment 90398 [details] [review]
[PATCH] Fix abstract socket supports build checking
Comment 3 Karol Lewandowski 2013-12-09 06:29:04 UTC
Comment on attachment 90398 [details] [review]
[PATCH] Fix abstract socket supports build checking

Review of attachment 90398 [details] [review]:
-----------------------------------------------------------------

+1

Indeed, this seems like right approach.

Thanks Chengwei!
Comment 4 Simon McVittie 2013-12-09 12:32:26 UTC
Comment on attachment 90398 [details] [review]
[PATCH] Fix abstract socket supports build checking

Review of attachment 90398 [details] [review]:
-----------------------------------------------------------------

Sorry, this is not right either: --enable-abstract-sockets[=yes] should forcibly enable them (and ideally, fail the build if they aren't supported).

I'm becoming quite tempted to change all our checks for abstract socket support into "#ifdef __linux__". I thought I remembered something about the Linux design for them having been copied from somewhere else - possibly Solaris - but I can't find any references to that now, and there are quite a few references to them being entirely Linux-specific, so...

GLib's GIO library has this:

gboolean
g_unix_socket_address_abstract_names_supported (void)
{
#ifdef __linux__
  return TRUE;
#else
  return FALSE;
#endif
}

So we could potentially get rid of the configure.ac check, and just use #ifdef __linux__ - AC_TRY_RUN is a horrible way to check for features anyway (it can't work when cross-compiling). There's no point in trying to support Linux versions that didn't have these sockets, because they were new in Linux 2.2, which is ancient and very much unmaintained.

---------

If there are non-Linux platforms with abstract sockets, then what should happen is (pseudocode):

case "$enable_abstract_sockets" in:
    (yes)
        either use abstract sockets or stop with a a fatal error
    (auto)
        if abstract sockets are supported, use them, else don't
    (no)
        don't use abstract sockets

(It doesn't have to be implemented as a switch/case structure, I'm just using that as a way to describe what should happen.)

Normally, I'd say: do the check in the "yes" and "auto" cases. In the "yes" case, if we lack support for abstract sockets, that's a fatal error (like saying --enable-tests when you don't have all the tests' dependencies).

However, there's an additional complication because the check for whether we have abstract socket support is an AC_RUN_IFELSE, which can't work when cross-compiling - so it might be necessary to go for something like this

case "$enable_abstract_sockets" in:
    (yes)
        if we are cross-compiling (4th argument of AC_RUN_IFELSE):
            assume that abstract sockets work, and use them
        else if abstract sockets work:
            use them
        else:
            fatal error
    (auto)
        if we are cross-compiling (4th argument of AC_RUN_IFELSE):
            (non-fatal?) warning
            choose semi-arbitrarily whether to use them
        else if abstract sockets work:
            use them
        else:
            don't use them
    (no)
        don't use abstract sockets regardless

or even this:

case "$enable_abstract_sockets" in:
    (yes)
        assume that abstract sockets work, and use them
    (auto)
        if we are cross-compiling (4th argument of AC_RUN_IFELSE):
            (non-fatal?) warning
            choose semi-arbitrarily whether to use them
        else if abstract sockets work:
            use them
        else:
            don't use them
    (no)
        don't use abstract sockets regardless

At the moment, the "semi-arbitrary choice" when cross-compiling is to assume that abstract sockets don't work; but to avoid silent incompatibility, I wonder whether that should be changed to assuming that (abstract sockets work) == (kernel is Linux), or similar.
Comment 5 Chengwei Yang 2013-12-10 01:21:57 UTC
(In reply to comment #4)
> Comment on attachment 90398 [details] [review] [review]
> [PATCH] Fix abstract socket supports build checking
> 
> Review of attachment 90398 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Sorry, this is not right either: --enable-abstract-sockets[=yes] should
> forcibly enable them (and ideally, fail the build if they aren't supported).

Yes, this part of code already there, see several lines below. You'll find lines like

if test x$enable_abstract_sockets = xyes; then
    if test x$ac_cv_have_abstract_sockets = xno; then
        AC_MSG_ERROR([Abstract sockets explicitly required, and support not detected.])
    fi
fi

The below part of your comment seems was talking about another topics:

1. treat abstract socket as a Linux specific feature

2. fix cross-compile by removing AC_RUN_IFELSE, there are several places where AC_RUN_IFELSE been used.

For me, this simple patch does fix this issue in the way #comment1. And we can file tickets for the other topic. How do you think?
Comment 6 Simon McVittie 2016-07-01 15:53:55 UTC
(In reply to Chengwei Yang from comment #5)
> For me, this simple patch does fix this issue in the way #comment1. And we
> can file tickets for the other topic. How do you think?

No, I still don't think the patch attached here is a correct solution.
Comment 7 Matthew Hitchens 2017-09-29 03:42:48 UTC
(In reply to Simon McVittie from comment #4
> At the moment, the "semi-arbitrary choice" when cross-compiling is to assume
> that abstract sockets don't work; but to avoid silent incompatibility, I
> wonder whether that should be changed to assuming that (abstract sockets
> work) == (kernel is Linux), or similar.

I had a similar thought. How would you feel about removing the --enable/disable-abstract-sockets detection altogether in favor of calling uname() at runtime and checking that sysname is "Linux"? After all, abstract sockets are a non-portable Linux extension of AF_UNIX.

This solution has the benefit of removing any cross-compilation issues, and autotools can guarantee that <sys/utsname.h> contains uname() at compile time (in case we're building on a system that doesn't support it).

Thoughts?
Comment 8 Simon McVittie 2017-09-29 11:12:34 UTC
(In reply to Matthew Hitchens from comment #7)
> I had a similar thought. How would you feel about removing the
> --enable/disable-abstract-sockets detection altogether in favor of calling
> uname() at runtime and checking that sysname is "Linux"? After all, abstract
> sockets are a non-portable Linux extension of AF_UNIX.

Why runtime and not compile-time? We already test the definedness of __linux__ in a few places.

The only runtime test that would perhaps make sense is to try using an abstract socket and see what happens, falling back to an equivalently-named non-abstract socket on failure to bind() to an abstract one - there seems little point in complicating things by using uname(). But it would probably be simpler to keep this compile-time, which is what GLib does (the test is literally #ifdef __linux__).

I thought I dimly remembered Linux having borrowed the abstract socket API feature from one of the proprietary Unixes, but I can't find any references to that with Google, so it seems I'm misremembering and this is indeed original to Linux.
Comment 9 Simon McVittie 2017-09-29 12:12:33 UTC
Created attachment 134566 [details] [review]
build: Remove unused substitution DBUS_PATH_OR_ABSTRACT

This was presumably once used in constructs like
"unix:" DBUS_PATH_OR_ABSTRACT "=/var/run/dbus/foo", but git grep says
there are no remaining uses, so it can go.
Comment 10 Simon McVittie 2017-09-29 12:13:46 UTC
Created attachment 134567 [details] [review]
unix: Condition Linux-specific abstract sockets on  __linux__

This is nicer for cross-compiling, because AC_RUN_IFELSE can't work
there. In practice abstract sockets are supported on Linux since
2.2 (so, all relevant versions), and on no other platform; so it
seems futile to keep this complexity.

---

Alternative to Attachment #90398 [details].
Comment 11 Philip Withnall 2017-09-29 12:32:08 UTC
Comment on attachment 134566 [details] [review]
build: Remove unused substitution DBUS_PATH_OR_ABSTRACT

Review of attachment 134566 [details] [review]:
-----------------------------------------------------------------

♥ git grep

++
Comment 12 Philip Withnall 2017-09-29 12:42:20 UTC
Comment on attachment 134567 [details] [review]
unix: Condition Linux-specific abstract sockets on  __linux__

Review of attachment 134567 [details] [review]:
-----------------------------------------------------------------

I also can’t find any references to abstract sockets being available on anything but Linux. ++
Comment 13 Simon McVittie 2017-09-29 13:13:22 UTC
Thanks, patches applied for 1.11.18.
Comment 14 Matthew Hitchens 2017-09-30 15:20:27 UTC
(In reply to Simon McVittie from comment #8)
> (In reply to Matthew Hitchens from comment #7)
> > I had a similar thought. How would you feel about removing the
> > --enable/disable-abstract-sockets detection altogether in favor of calling
> > uname() at runtime and checking that sysname is "Linux"? After all, abstract
> > sockets are a non-portable Linux extension of AF_UNIX.
> 
> Why runtime and not compile-time? We already test the definedness of
> __linux__ in a few places.

Ah, much simpler. Most of my experience is in Java and the CLR, so sometimes my brain gravitates to runtime solutions. Compiling to machine language means making decisions earlier. Excited to see this fixed! Thanks.


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.