[syslog-ng] [PATCH 1/1] Add support for Unix sockets.

[Gergely Nagy]

Hi!

A quick review follows - keep in mind I still wasn't able to have a
closer look, and most of the things I comment on below are fairly minor,
but lets see my conclusion at the end!

Conrad Hoffmann <ch at bitfehler.net> writes:

[...]
> diff --git a/src/mongo-client.c b/src/mongo-client.c
> index ed931e4..a290b52 100644
> --- a/src/mongo-client.c
> +++ b/src/mongo-client.c
[...]
> +mongo_connection *
> +mongo_unix_connect (const char *path)
> +{
> +  int fd = -1;
> +  mongo_connection *conn;
> +  struct sockaddr_un remote;
> +
> +  if (!path || strlen (path) >= sizeof (remote.sun_path))
> +    {
> +      errno = path ? ENAMETOOLONG : EINVAL;
> +      return NULL;
> +    }
> +
> +  conn = g_new0 (mongo_connection, 1);

I'd move this to near the end of the function, as it is not used
until...

[...]

> +  conn->fd = fd;

...here. So moving the g_new0 just above this line would save us a few
g_free()s on the error paths.

Also, on the error paths, I'd leave errno alone, socket() and connect()
should set them appropriately, I hope. (At most, I'd save it and restore
it just before the return NULL).

> +mongo_connection *mongo_unix_connect (const char *path);

Hrm, I was wondering whether it would make sense to bake this into
mongo_connect() itself?

Rename the host argument to, say, address, and if port is -1, then do
what mongo_unix_connect() would do. (Behind the scenes,
mongo_unix_connect() and mongo_tcp_connect() would still be there, but
static).

That would also simplify the syslog-ng part, I think: path() would
simply set host(path) and port(-1). Both path() and host()/port() would
scream loudly if the other was already set before.

> +mongo_sync_connection *
> +mongo_sync_unix_connect (const gchar *path, gboolean slaveok)

Same idea here - baking it into mongo_sync_connect(), with port=-1 being
special.

Might aswell introduce something like #define MONGO_CONN_LOCAL -1, to
avoid magic numbers all over the place.

All in all, I like the functionality, there's even docs (yay!), and an
updated example. However, I'd do things a tiny bit differently, so I
propose the following: I'll merge this patch, and followup with a commit
or two that merges the _unix_connect() functions into the plain
_connect() ones.

If you don't have an objection, I'll go ahead with this plan on friday,
if all goes well.

-- 
|8]