Bug 69118

I was looking at how libX11 handled this pre-xcb and alanc knows the answer:

<alanc> http://cgit.freedesktop.org/xorg/lib/libxtrans/tree/Xtranssock.c#n210
<alanc> ah, looks like ajax cleaned out the HPUX definitions yers ago: 
http://cgit.freedesktop.org/xorg/lib/libxtrans/commit/Xtranssock.c?id=339ddc413559d4cb117a72f87b2a70dae6911c32

Comment on attachment 85462 [details]
Also looks for unix socket in /{usr,var}/spool/sockets/X11/...

Interesting approach.  A few comments:

Please drop "sep", and just make the separator part of the base string.  You can then just use an array of strings rather than an array of structures.

Please wrap the "spool" paths you're adding in an appropriate ifdef for HP-UX; they aren't appropriate for other platforms.  You might 

Please turn this into two separate patches: one patch that has no functional effect but just converts to the "bases" approach, and one that adds the new bases you need.

Note that this seems like a sensible cleanup even if not adding the HP-UX paths; please feel free to add the cleanup regardless.

Created attachment 85472 [details] [review]
Clean up code for picking path for UNIX socket files

Just the reorganization without the HPUX paths

Created attachment 85473 [details] [review]
Add /usr/spool/sockets/X11/ to search path for unix sockets

HPUX socket path support

Comment on attachment 85472 [details] [review]
Clean up code for picking path for UNIX socket files

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

Ok, I give up. According to the commit message, I would expect no behavior changes from this patch, but this code and the patch are complicated enough that I am not convinced about this.

Any ideas on how this could be made easier to review? Perhaps splitting up into more individual patches? (Sorry!)

::: src/xcb_util.c
@@ +81,5 @@
> +static int _xcb_add_socket_path(const char** socket_paths, int max_cnt, int* cnt,
> +				const char* socket_path)
> +{
> +    if (*cnt >= max_cnt)
> +	return -1;

I know I will eventually regret this, but I would prefer this to be an assert() (mostly because the return value of this function is completely ignored).

@@ +255,5 @@
>      file = malloc(filelen);
>      if(file == NULL)
>          return -1;
>  
> +    for(i = 0; i < socket_path_cnt; ++i) {

This is another behavior change compared to the old code, isn't it? The old code only tried a single place and then gave up (at least sometimes, this is all hard to follow).

@@ +260,3 @@
>  #ifdef HAVE_LAUNCHD
> +	if(strncmp(socket_paths[i], "/tmp/launch", 11) == 0)
> +	    actual_filelen = snprintf(file, filelen, "%s%c%d", socket_paths[i], ':', display);

Pfft, why did you move the ':' out via a new %c argument? This is just a constant character...

@@ -240,4 @@
>  
> -    if (fd < 0 && !protocol && *host == '\0') {
> -	    unsigned short port = X_TCP_PORT + display;
> -	    fd = _xcb_open_tcp(host, protocol, port);

This code is lost in the new version, isn't it?

Created attachment 85836 [details] [review]
Simpler patch. Only changes path for HPUX no cleanup.

Pushed, thanks.