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@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]