<html>
<head>
<base href="https://bugs.freedesktop.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW --- - Unix spool is located at /var/spool/sockets/X11/... or /usr/spool/sockets/X11/... under HPUX"
href="https://bugs.freedesktop.org/show_bug.cgi?id=69118#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW --- - Unix spool is located at /var/spool/sockets/X11/... or /usr/spool/sockets/X11/... under HPUX"
href="https://bugs.freedesktop.org/show_bug.cgi?id=69118">bug 69118</a>
from <span class="vcard"><a class="email" href="mailto:psychon@znc.in" title="Uli Schlachter <psychon@znc.in>"> <span class="fn">Uli Schlachter</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=85472" name="attach_85472" title="Clean up code for picking path for UNIX socket files">attachment 85472</a> <a href="attachment.cgi?id=85472&action=edit" title="Clean up code for picking path for UNIX socket files">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=69118&attachment=85472'>[review]</a>
Clean up code for picking path for UNIX socket files
Review of <span class=""><a href="attachment.cgi?id=85472" name="attach_85472" title="Clean up code for picking path for UNIX socket files">attachment 85472</a> <a href="attachment.cgi?id=85472&action=edit" title="Clean up code for picking path for UNIX socket files">[details]</a></span> <a href='page.cgi?id=splinter.html&bug=69118&attachment=85472'>[review]</a>:
-----------------------------------------------------------------
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 @@
<span class="quote">> +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;</span >
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 @@
<span class="quote">> file = malloc(filelen);
> if(file == NULL)
> return -1;
>
> + for(i = 0; i < socket_path_cnt; ++i) {</span >
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 @@
<span class="quote">> #ifdef HAVE_LAUNCHD
> + if(strncmp(socket_paths[i], "/tmp/launch", 11) == 0)
> + actual_filelen = snprintf(file, filelen, "%s%c%d", socket_paths[i], ':', display);</span >
Pfft, why did you move the ':' out via a new %c argument? This is just a
constant character...
@@ -240,4 @@
<span class="quote">>
> - if (fd < 0 && !protocol && *host == '\0') {
> - unsigned short port = X_TCP_PORT + display;
> - fd = _xcb_open_tcp(host, protocol, port);</span >
This code is lost in the new version, isn't it?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the QA Contact for the bug.</li>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>