Bug 34905

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

Created attachment 90398 [details] [review]
[PATCH] Fix abstract socket supports build checking

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 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.

(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?

(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.

(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?

(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.

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.

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 on attachment 134566 [details] [review]
build: Remove unused substitution DBUS_PATH_OR_ABSTRACT

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

♥ git grep

++

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. ++

Thanks, patches applied for 1.11.18.

(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.