Code review comment for lp:~schuster/mysql-proxy/remove_unix_socket

Am 19.04.2010 um 10:45 schrieb Michael Schuster:

> On 04/19/10 09:39, Jan Kneschke wrote:
>> Am 19.04.2010 um 08:48 schrieb Michael Schuster:
>>
>>> Michael Schuster has proposed merging lp:~schuster/mysql-proxy/remove_unix_socket into lp:mysql-proxy.
>>>
>>> Requested reviews:
>>> MySQL Proxy Developers (mysql-proxy-developers)
>>>
>>> --
>>> https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652
>>> Your team MySQL Proxy Developers is subscribed to branch lp:mysql-proxy.
>>> === modified file 'src/network-address.c'
>>> --- src/network-address.c 2010-04-07 12:44:32 +0000
>>> +++ src/network-address.c 2010-04-19 06:48:19 +0000
>>> @@ -45,6 +45,8 @@
>>> #include <string.h>
>>> #include <errno.h>
>>> #include <fcntl.h>
>>> +#include <glib.h>
>>> +#include <glib/gstdio.h>
>>>
>>> #include "network-address.h"
>>> #include "glib-ext.h"
>>> @@ -63,10 +65,33 @@
>>> }
>>>
>>> void network_address_free(network_address *addr) {
>>> +
>>> if (!addr) return;
>>>
>>> +#ifndef WIN32
>>> + /*
>>> + * if the name we're freeing starts with a '/', we're
>>> + * looking at a unix socket which needs to be removed
>>> + */
>>> + if (addr->fail_errno == 0 && addr->name != NULL &&
>>> + addr->name->str != NULL) {
>>> + gchar *name;
>>> + int ret;
>>> +
>>> + name = addr->name->str;
>>> + if (name[0] == '/' && g_access(name, 0) == 0) {
>>> + ret = g_remove(name);
>>
>> Only try to remove it if we can to hide the error-msg (aka noise) for it ?
>
> sorry, don't understand the question :-(

I tried to figure out the reason for the g_access() and assumed it is to check if the g_remove() could succeed without a permission problem.

Problem with access() is that it creates a race-condition before the the permission check and the g_remove(). Instead just unlink() it and check for the 'errno != EPERM' to see if we can't delete the socket as we drop the privileges.

>>> +
>>> + if (ret == 0)
>>> + g_debug("%s removing socket %s successful",
>>> + G_STRLOC, name);
>>> + else
>>> + g_debug("%s removing socket %s failed: %s (%d)",
>>> + G_STRLOC, name, strerror(errno));
>>
>> ... in that case the g_debug() is perhaps better a g_critical() ?
>
> ok.

The whole patch does unlink() any file that is set as address. In the case of a network_socket_new() + network_address_set_address() + network_socket_connect() + network_socket_free() the socket-file should not be unlinked as it we didn't listen() on it.

Instead of ->fail_errno use a gboolean to track if you want to unlink the socket file on free and only set it to true in the case of a successful network_socket_bind().

> Michael
> --
> https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652
> Your team MySQL Proxy Developers is subscribed to branch lp:mysql-proxy.