[PATCH 0/1] [RESEND] libmongo-client: Add support for Unix sockets
Hey once more. Here is a first revision of my earlier patch. Changes to last version: - Better error handling/input checking (thanks to Balazs for the feedback) - Free everything that was allocated before bailing out after errors - Some style fixes, hope it's ok now Further feedback welcome! Conrad Hoffmann (1): Add support for Unix sockets. examples/mongo-dump.c | 22 +++++++++++++++++----- src/mongo-client.c | 39 +++++++++++++++++++++++++++++++++++++++ src/mongo-client.h | 12 ++++++++++++ src/mongo-sync.c | 23 +++++++++++++++++++++++ src/mongo-sync.h | 15 +++++++++++++++ 5 files changed, 106 insertions(+), 5 deletions(-) -- 1.7.11.4
Add a separate unix_connect function to both mongo-client and mongo-sync and demonstrate usage by adding this feature to the mongo-dump example program. Replica sets and Unix sockets can't work together so far. Signed-off-by: Conrad Hoffmann <ch@bitfehler.net> --- examples/mongo-dump.c | 22 +++++++++++++++++----- src/mongo-client.c | 39 +++++++++++++++++++++++++++++++++++++++ src/mongo-client.h | 12 ++++++++++++ src/mongo-sync.c | 23 +++++++++++++++++++++++ src/mongo-sync.h | 15 +++++++++++++++ 5 files changed, 106 insertions(+), 5 deletions(-) diff --git a/examples/mongo-dump.c b/examples/mongo-dump.c index d0fe04a..ec666f6 100644 --- a/examples/mongo-dump.c +++ b/examples/mongo-dump.c @@ -29,6 +29,7 @@ typedef struct { gchar *host; gint port; + gchar *path; gchar *db; gchar *coll; gchar *output; @@ -53,10 +54,19 @@ mongo_dump (config_t *config) gchar *error = NULL; int e; - VLOG ("Connecting to %s:%d/%s.%s...\n", config->host, config->port, - config->db, config->coll); + if (config->path) + { + VLOG ("Connecting to %s/%s.%s...\n", config->path,config->db, + config->coll); + conn = mongo_sync_unix_connect (config->path, config->slaveok); + } + else + { + VLOG ("Connecting to %s:%d/%s.%s...\n", config->host, config->port, + config->db, config->coll); + conn = mongo_sync_connect (config->host, config->port, config->slaveok); + } - conn = mongo_sync_connect (config->host, config->port, config->slaveok); if (!conn) { e = errno; @@ -156,7 +166,7 @@ main (int argc, char *argv[]) GError *error = NULL; GOptionContext *context; config_t config = { - NULL, 27017, NULL, NULL, NULL, NULL, FALSE, FALSE, FALSE + NULL, 27017, NULL, NULL, NULL, NULL, NULL, FALSE, FALSE, FALSE }; GOptionEntry entries[] = @@ -164,6 +174,8 @@ main (int argc, char *argv[]) { "host", 'h', 0, G_OPTION_ARG_STRING, &config.host, "Host to connect to", "HOST" }, { "port", 'p', 0, G_OPTION_ARG_INT, &config.port, "Port", "PORT" }, + { "path", 'P', 0, G_OPTION_ARG_STRING, &config.path, + "Path of Unix socket to connect to", "PATH" }, { "db", 'd', 0, G_OPTION_ARG_STRING, &config.db, "Database", "DB" }, { "collection", 'c', 0, G_OPTION_ARG_STRING, &config.coll, "Collection", "COLL" }, @@ -186,7 +198,7 @@ main (int argc, char *argv[]) exit (1); } - if (!config.host || !config.port || !config.db || + if (!((config.host && config.port) || config.path) || !config.db || !config.coll || !config.output) { gchar **nargv; 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 @@ -29,6 +29,7 @@ #include <arpa/inet.h> #include <sys/types.h> #include <sys/socket.h> +#include <sys/un.h> #include <netdb.h> #include <sys/uio.h> #include <netinet/in.h> @@ -109,6 +110,44 @@ mongo_connect (const char *host, int port) return conn; } +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); + + fd = socket (AF_UNIX, SOCK_STREAM, 0); + if (fd == -1) + { + g_free (conn); + errno = EADDRNOTAVAIL; + return NULL; + } + + remote.sun_family = AF_UNIX; + strncpy (remote.sun_path, path, sizeof (remote.sun_path)); + if (connect (fd, (struct sockaddr *)&remote, sizeof (remote)) == -1) + { + close (fd); + g_free (conn); + errno = EADDRNOTAVAIL; + return NULL; + } + + conn->fd = fd; + + return conn; +} + void mongo_disconnect (mongo_connection *conn) { diff --git a/src/mongo-client.h b/src/mongo-client.h index 28863e5..3917b05 100644 --- a/src/mongo-client.h +++ b/src/mongo-client.h @@ -50,6 +50,18 @@ typedef struct _mongo_connection mongo_connection; */ mongo_connection *mongo_connect (const char *host, int port); +/** Connect to a MongoDB server via a Unix socket. + * + * Connects to a single MongoDB server using a Unix Domain Socket. + * + * @param path is the path of the Unix socket. + * + * @returns A newly allocated mongo_connection object or NULL on + * error. It is the responsibility of the caller to free it once it is + * not used anymore. + */ +mongo_connection *mongo_unix_connect (const char *path); + /** Disconnect from a MongoDB server. * * @param conn is the connection object to disconnect from. diff --git a/src/mongo-sync.c b/src/mongo-sync.c index ab3ae16..ce26245 100644 --- a/src/mongo-sync.c +++ b/src/mongo-sync.c @@ -51,6 +51,29 @@ mongo_sync_connect (const gchar *host, gint port, return s; } +mongo_sync_connection * +mongo_sync_unix_connect (const gchar *path, gboolean slaveok) +{ + mongo_sync_connection *s; + mongo_connection *c; + + c = mongo_unix_connect (path); + if (!c) + return NULL; + s = g_realloc (c, sizeof (mongo_sync_connection)); + + s->slaveok = slaveok; + s->safe_mode = FALSE; + s->auto_reconnect = FALSE; + s->rs.seeds = NULL; + s->rs.hosts = NULL; + s->rs.primary = NULL; + s->last_error = NULL; + s->max_insert_size = MONGO_SYNC_DEFAULT_MAX_INSERT_SIZE; + + return s; +} + gboolean mongo_sync_conn_seed_add (mongo_sync_connection *conn, const gchar *host, gint port) diff --git a/src/mongo-sync.h b/src/mongo-sync.h index bc4f50e..515062a 100644 --- a/src/mongo-sync.h +++ b/src/mongo-sync.h @@ -67,6 +67,21 @@ mongo_sync_connection *mongo_sync_connect (const gchar *host, gint port, gboolean slaveok); +/** Synchronously connect to a MongoDB server via Unix socket. + * + * Sets up a synchronous connection to a MongoDB server. + * + * @param path is the path to the Unix Domain Socket to connect to. + * @param slaveok signals whether queries made against a slave are + * acceptable. + * + * @returns A newly allocated mongo_sync_connection object, or NULL on + * error. It is the responsibility of the caller to close and free the + * connection when appropriate. + */ +mongo_sync_connection *mongo_sync_unix_connect (const gchar *host, + gboolean slaveok); + /** Add a seed to an existing MongoDB connection. * * The seed list will be used for reconnects, prioritized before the -- 1.7.11.4
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]
Gergely Nagy <algernon@balabit.hu> writes:
+mongo_connection *mongo_unix_connect (const char *path);
Hrm, I was wondering whether it would make sense to bake this into mongo_connect() itself?
I thought this further.. I really want to merge the two, and make a few other parts of the library smarter, so that replica sets would work with unix sockets too, and even a mixed environment would be possible (even if that'd be terribly broken, but still! :P). With mongo_connect() supporting unix sockets, and mongo_util_parse_addr() being smarter and recognising them, we'd have pretty much everything covered for replica set support, I think. -- |8]
Hi,
Gergely Nagy <algernon@balabit.hu> writes: I thought this further.. I really want to merge the two, and make a few other parts of the library smarter, so that replica sets would work with unix sockets too, and even a mixed environment would be possible (even if that'd be terribly broken, but still! :P).
With mongo_connect() supporting unix sockets, and mongo_util_parse_addr() being smarter and recognising them, we'd have pretty much everything covered for replica set support, I think.
I agree that this would be a far superior solution, and if you want me to I would even try to help you a little. What I am not sure about is whether MongoDB actually supports replica sets with Unix sockets, but if not I guess it's about time to open a bug report for that ;) Conrad
"Conrad Hoffmann" <ch@bitfehler.net> writes:
I agree that this would be a far superior solution, and if you want me to I would even try to help you a little.
If you can do the 'baking in' before I get there on friday, that would be most appreciated :) Basically, the current mongo_(sync_)connect() would be renamed to mongo_tcp_(sync)_connect(), and made static. Then mongo_unix_connect() would be introduced, and mongo_connect() made into a wrapper that calls one of the other two, depending on the value of port. The "host" param would also need to be renamed to address. Once that's done, teach the address parser to recognise unix sockets, and return -1 in the port part in that case. I'm not entirely sure what logic should be used to figure out whether an address is a unix socket or not, though. This is probably the hardest part. If all that's done, have a look at mongo_tcp_connect() and mongo_unix_connect() to see if there's any code that can be factored out into a common function. I suspect there will be. And after that: see if it still works.
What I am not sure about is whether MongoDB actually supports replica sets with Unix sockets, but if not I guess it's about time to open a bug report for that ;)
I'm not entirely sure it supports that, but if it does, or ever will, libmongo-client will be prepared :P -- |8]
Gergely Nagy <algernon@balabit.hu> writes:
"Conrad Hoffmann" <ch@bitfehler.net> writes:
I agree that this would be a far superior solution, and if you want me to I would even try to help you a little.
If you can do the 'baking in' before I get there on friday, that would be most appreciated :)
I got around to do it today, and did some more investigation. Turns out, mongodb does not allow unix sockets in replicaset configs. But nevertheless, I took your patch, then changed it to merge it behind a common mongo_connect() (this way, mongo_sync_connect() did not need any change at all, except for a documentation update). I have not changed mongo_util_parse_addr(), because it's not needed right now. If and when the mongodb folk figure out whether they want to allow unix sockets in replicaset configs, we can update the parse_addr() function and things will just work. One thing of note, is that if you connect through a unix socket to a replicaset, and you force a reconnect, you may end up being connected via TCP instead. But that requires a few things to go bad, and I believe this behaviour is fine. I'll push the changes to github soon. -- |8]
On 08/16/2012 03:41 PM, Gergely Nagy wrote:
Gergely Nagy <algernon@balabit.hu> writes:
"Conrad Hoffmann" <ch@bitfehler.net> writes:
I agree that this would be a far superior solution, and if you want me to I would even try to help you a little.
If you can do the 'baking in' before I get there on friday, that would be most appreciated :)
I got around to do it today, and did some more investigation. Turns out, mongodb does not allow unix sockets in replicaset configs. But nevertheless, I took your patch, then changed it to merge it behind a common mongo_connect() (this way, mongo_sync_connect() did not need any change at all, except for a documentation update).
I have not changed mongo_util_parse_addr(), because it's not needed right now. If and when the mongodb folk figure out whether they want to allow unix sockets in replicaset configs, we can update the parse_addr() function and things will just work.
One thing of note, is that if you connect through a unix socket to a replicaset, and you force a reconnect, you may end up being connected via TCP instead. But that requires a few things to go bad, and I believe this behaviour is fine.
I'll push the changes to github soon.
Nice to hear. I was just about to see what I could get done tonight, but that's even better :) Thanks a lot!
participants (2)
-
Conrad Hoffmann
-
Gergely Nagy