From 56192221b7a26438a5e1810571d46ed25bc65557 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 21 Sep 2009 13:07:10 -0700 Subject: [PATCH 1/1] vconn-unix: Unlink Unix sockets for vconns at close and free memory. The make_unix_socket() function that Unix vconns use to create their bindings calls fatal_signal_add_file_to_unlink() to make sure that the binding socket gets unlinked from the file system if the process is killed by a fatal signal. However, this doesn't happen until the process is actually killed, even if the vconn that owns the socket is actually closed. This wasn't a problem when the vconn-unix code was written, because all of the unix vconns were created at process start time and never destroyed during the normal process runtime. However, these days the vswitch can create and destroy unix vconns at runtime depending on the contents of its configuration file, so it's better to clean up the file system and free the memory required to keep track of these sockets. This commit makes unix vconns and pvconns delete their files and free the memory used to track them when the (p)vconns are closed. This is only a very minor leak most of the time. Bug #1817. --- lib/vconn-stream.c | 49 ++++++++++++++++++++++++++++++++++++++++++---- lib/vconn-stream.h | 3 ++- lib/vconn-tcp.c | 4 ++-- lib/vconn-unix.c | 14 +++++++------ 4 files changed, 57 insertions(+), 13 deletions(-) diff --git a/lib/vconn-stream.c b/lib/vconn-stream.c index e2991ddfe..0551c9ebf 100644 --- a/lib/vconn-stream.c +++ b/lib/vconn-stream.c @@ -23,6 +23,7 @@ #include #include #include +#include "fatal-signal.h" #include "leak-checker.h" #include "ofpbuf.h" #include "openflow/openflow.h" @@ -44,6 +45,7 @@ struct stream_vconn struct ofpbuf *rxbuf; struct ofpbuf *txbuf; struct poll_waiter *tx_waiter; + char *unlink_path; }; static struct vconn_class stream_vconn_class; @@ -51,10 +53,20 @@ static struct vconn_class stream_vconn_class; static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 25); static void stream_clear_txbuf(struct stream_vconn *); +static void maybe_unlink_and_free(char *path); +/* Creates a new vconn named 'name' that will send and receive data on 'fd' and + * stores a pointer to the vconn in '*vconnp'. Initial connection status + * 'connect_status' is interpreted as described for vconn_init(). + * + * When '*vconnp' is closed, then 'unlink_path' (if nonnull) will be passed to + * fatal_signal_unlink_file_now() and then freed with free(). + * + * Returns 0 if successful, otherwise a positive errno value. (The current + * implementation never fails.) */ int new_stream_vconn(const char *name, int fd, int connect_status, - struct vconn **vconnp) + char *unlink_path, struct vconn **vconnp) { struct stream_vconn *s; @@ -64,6 +76,7 @@ new_stream_vconn(const char *name, int fd, int connect_status, s->txbuf = NULL; s->tx_waiter = NULL; s->rxbuf = NULL; + s->unlink_path = unlink_path; *vconnp = &s->vconn; return 0; } @@ -83,6 +96,7 @@ stream_close(struct vconn *vconn) stream_clear_txbuf(s); ofpbuf_delete(s->rxbuf); close(s->fd); + maybe_unlink_and_free(s->unlink_path); free(s); } @@ -252,6 +266,7 @@ struct pstream_pvconn int fd; int (*accept_cb)(int fd, const struct sockaddr *, size_t sa_len, struct vconn **); + char *unlink_path; }; static struct pvconn_class pstream_pvconn_class; @@ -263,16 +278,31 @@ pstream_pvconn_cast(struct pvconn *pvconn) return CONTAINER_OF(pvconn, struct pstream_pvconn, pvconn); } +/* Creates a new pvconn named 'name' that will accept new socket connections on + * 'fd' and stores a pointer to the vconn in '*pvconnp'. + * + * When a connection has been accepted, 'accept_cb' will be called with the new + * socket fd 'fd' and the remote address of the connection 'sa' and 'sa_len'. + * accept_cb must return 0 if the connection is successful, in which case it + * must initialize '*vconnp' to the new vconn, or a positive errno value on + * error. In either case accept_cb takes ownership of the 'fd' passed in. + * + * When '*pvconnp' is closed, then 'unlink_path' (if nonnull) will be passed to + * fatal_signal_unlink_file_now() and freed with free(). + * + * Returns 0 if successful, otherwise a positive errno value. (The current + * implementation never fails.) */ int new_pstream_pvconn(const char *name, int fd, - int (*accept_cb)(int fd, const struct sockaddr *, - size_t sa_len, struct vconn **), - struct pvconn **pvconnp) + int (*accept_cb)(int fd, const struct sockaddr *sa, + size_t sa_len, struct vconn **vconnp), + char *unlink_path, struct pvconn **pvconnp) { struct pstream_pvconn *ps = xmalloc(sizeof *ps); pvconn_init(&ps->pvconn, &pstream_pvconn_class, name); ps->fd = fd; ps->accept_cb = accept_cb; + ps->unlink_path = unlink_path; *pvconnp = &ps->pvconn; return 0; } @@ -282,6 +312,7 @@ pstream_close(struct pvconn *pvconn) { struct pstream_pvconn *ps = pstream_pvconn_cast(pvconn); close(ps->fd); + maybe_unlink_and_free(ps->unlink_path); free(ps); } @@ -327,3 +358,13 @@ static struct pvconn_class pstream_pvconn_class = { pstream_accept, pstream_wait }; + +/* Helper functions. */ +static void +maybe_unlink_and_free(char *path) +{ + if (path) { + fatal_signal_unlink_file_now(path); + free(path); + } +} diff --git a/lib/vconn-stream.h b/lib/vconn-stream.h index fd3d8bd46..91904fff5 100644 --- a/lib/vconn-stream.h +++ b/lib/vconn-stream.h @@ -26,10 +26,11 @@ struct pvconn; struct sockaddr; int new_stream_vconn(const char *name, int fd, int connect_status, - struct vconn **vconnp); + char *unlink_path, struct vconn **vconnp); int new_pstream_pvconn(const char *name, int fd, int (*accept_cb)(int fd, const struct sockaddr *, size_t sa_len, struct vconn **), + char *unlink_path, struct pvconn **pvconnp); #endif /* vconn-stream.h */ diff --git a/lib/vconn-tcp.c b/lib/vconn-tcp.c index 44f49f10d..b4e523435 100644 --- a/lib/vconn-tcp.c +++ b/lib/vconn-tcp.c @@ -58,7 +58,7 @@ new_tcp_vconn(const char *name, int fd, int connect_status, return errno; } - retval = new_stream_vconn(name, fd, connect_status, vconnp); + retval = new_stream_vconn(name, fd, connect_status, NULL, vconnp); if (!retval) { struct vconn *vconn = *vconnp; vconn_set_remote_ip(vconn, remote->sin_addr.s_addr); @@ -108,7 +108,7 @@ ptcp_open(const char *name UNUSED, char *suffix, struct pvconn **pvconnp) if (fd < 0) { return -fd; } else { - return new_pstream_pvconn("ptcp", fd, ptcp_accept, pvconnp); + return new_pstream_pvconn("ptcp", fd, ptcp_accept, NULL, pvconnp); } } diff --git a/lib/vconn-unix.c b/lib/vconn-unix.c index e2b6f7a37..f24c84640 100644 --- a/lib/vconn-unix.c +++ b/lib/vconn-unix.c @@ -47,20 +47,21 @@ static int unix_open(const char *name, char *suffix, struct vconn **vconnp) { const char *connect_path = suffix; - char bind_path[128]; + char *bind_path; int fd; - sprintf(bind_path, "/tmp/vconn-unix.%ld.%d", - (long int) getpid(), n_unix_sockets++); + bind_path = xasprintf("/tmp/vconn-unix.%ld.%d", + (long int) getpid(), n_unix_sockets++); fd = make_unix_socket(SOCK_STREAM, true, false, bind_path, connect_path); if (fd < 0) { VLOG_ERR("%s: connection to %s failed: %s", bind_path, connect_path, strerror(-fd)); + free(bind_path); return -fd; } return new_stream_vconn(name, fd, check_connection_completion(fd), - vconnp); + bind_path, vconnp); } struct vconn_class unix_vconn_class = { @@ -102,7 +103,8 @@ punix_open(const char *name UNUSED, char *suffix, struct pvconn **pvconnp) return error; } - return new_pstream_pvconn("punix", fd, punix_accept, pvconnp); + return new_pstream_pvconn("punix", fd, punix_accept, + xstrdup(suffix), pvconnp); } static int @@ -118,7 +120,7 @@ punix_accept(int fd, const struct sockaddr *sa, size_t sa_len, } else { strcpy(name, "unix"); } - return new_stream_vconn(name, fd, 0, vconnp); + return new_stream_vconn(name, fd, 0, NULL, vconnp); } struct pvconn_class punix_pvconn_class = { -- 2.43.0