logger: move writer/sock fd to fd_entity struct 42/130742/4
authorRafal Pietruch <r.pietruch@samsung.com>
Tue, 23 May 2017 13:45:20 +0000 (15:45 +0200)
committerRafal Pietruch <r.pietruch@samsung.com>
Thu, 25 May 2017 11:48:00 +0000 (13:48 +0200)
writers and sockets fds are registered in epoll loop
their fds are placed in the common fd_entity struct.
adding/removing fds to logger's epoll are redesigned
to reflect this pattern

free logger extra readers possible memory leak bugfix
and style minor changes

Change-Id: Iccb7ac50658494451290fa98ad170247a141099e

src/logger/logger.c

index 2244739..db9f487 100644 (file)
@@ -72,6 +72,7 @@ enum { BUFFER_MIN = 0, BUFFER_MAX = 65535 };
 struct logger;
 struct writer;
 struct reader;
+struct log_buffer;
 
 /**
  * @brief Event handler
@@ -84,14 +85,15 @@ typedef void (*dispatch_event_t)(struct logger *server, struct epoll_event *even
 struct fd_entity {
        dispatch_event_t dispatch_event;
        struct epoll_event event;
+       int fd;
 };
 
 /**
  * @brief Service a writer
  * @details Specific handler that event is delegated to, according to writers type
  * @param[in] server The logger server
- * @param[in] wr The writer who sent the request
- * @param[in] event The relevant event
+ * @param[in] wr The writer to handle the request
+ * @param[in] event The relevant event on file descriptor
  * @return 0 on success, else -errno
  */
 typedef int (*service_writer_t)(struct logger* server, struct writer* wr, struct epoll_event* event);
@@ -107,9 +109,7 @@ typedef int (*service_writer_t)(struct logger* server, struct writer* wr, struct
 typedef int (*service_socket_t)(struct logger *server, struct writer *wr, struct dlog_control_msg *msg);
 
 struct writer {
-       struct fd_entity   _entity;
-       int                working_fd;
-       struct epoll_event event;
+       struct fd_entity   fd_entity;
        struct log_buffer* buf_ptr;
        int                readed;
        service_writer_t   service_writer;
@@ -138,11 +138,8 @@ struct reader {
        service_reader_t   service_reader;
 };
 
-struct log_buffer;
-
 struct sock_data {
-       struct fd_entity   _entity;
-       int                fd;
+       struct fd_entity   fd_entity;
        struct log_buffer* buf_ptr;
        struct epoll_event event;
        service_socket_t   service_socket;
@@ -152,7 +149,6 @@ struct log_buffer {
        struct sock_data   sock_wr;
        struct sock_data   sock_ctl;
        list_head          readers;
-
        int                id;
        int                size;
        int                lines;
@@ -214,7 +210,7 @@ static void dispatch_event_sock(struct logger *server, struct epoll_event *event
 static void reader_free(struct reader* reader);
 static void logger_free(struct logger* l);
 static int parse_args(int argc, char ** argv, struct buffering * b);
-static int socket_initialize(struct sock_data *sock, struct log_buffer *buffer, service_socket_t service_socket, struct socket_config_data *data, int epoll_fd);
+static int socket_initialize(struct sock_data *sock, struct log_buffer *buffer, service_socket_t service_socket, struct socket_config_data *data);
 static void fd_set_flags(int fd, int mask);
 static void fd_clear_flags(int fd, int mask);
 static void help();
@@ -413,28 +409,65 @@ failure:
 }
 
 /**
- * @brief Add FD to epoll
- * @details Adds given FD to given epoll
- * @param[in] epollfd epoll file descriptor to add the other FD to
- * @param[in] fd The file descriptor to add to the epoll
- * @param[in] event The event to return to epoll
+ * @brief Add given FD entity to logger event notification
+ * @details Uses epoll mechanism for notification of events
+ * @param[in] logger server owning epoll main loop
+ * @param[in] fd_entity file descriptor entity
  * @return 0 on success, -errno on failure
  */
-static inline int add_fd_loop(int epollfd, int fd, struct epoll_event* event)
+static inline int add_fd_entity(struct logger *logger, struct fd_entity *fd_entity)
 {
-       return epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, event) < 0 ? -errno : 0;
+       assert(logger);
+       assert(fd_entity);
+       return epoll_ctl(logger->epollfd, EPOLL_CTL_ADD, fd_entity->fd, &fd_entity->event) < 0 ? -errno : 0;
 }
 
 /**
- * @brief Remove FD from epoll
- * @details Remove given FD from given epoll
- * @param[in] epollfd The file descriptor of epoll
- * @param[in] fd The FD to remove from epoll
+ * @brief Remove given FD entity from logger event notification
+ * @details Uses epoll mechanism for notification of events
+ * @param[in] logger server owning epoll main loop
+ * @param[in] fd_entity file descriptor entity
  * @return 0 on success, else -errno
  */
-static inline int remove_fd_loop(int epollfd, int fd)
+static inline int remove_fd_entity(struct logger *logger, struct fd_entity *fd_entity)
+{
+       assert(logger);
+       assert(fd_entity);
+       return epoll_ctl(logger->epollfd, EPOLL_CTL_DEL, fd_entity->fd, NULL) < 0 ? -errno : 0;
+}
+
+/**
+ * @brief Init fd entity to call given function with user data on event
+ * @details Initializes dispatch_event callback and user_data pointer
+ * @param[in] fd_entity file descriptor entity to initialize
+ * @param[in] dispatch_event callback function to dispatch an event to
+ * @param[in] user_data pointer to data to place in event structure when dispatch event is called
+ */
+static void init_fd_entity(struct fd_entity *fd_entity, dispatch_event_t dispatch_event, void *user_data)
+{
+       assert(fd_entity);
+       *fd_entity = (struct fd_entity) {
+               .dispatch_event = dispatch_event,
+               .event = (struct epoll_event) {
+                       .data = (epoll_data_t) { .ptr = user_data },
+                       .events = 0
+               },
+               .fd = -1
+       };
+}
+
+/**
+ * @brief Set file descriptor to epoll entity for ready to read events
+ * @details Initializes fd and events mask in epoll entity
+ * @param[in] fd_entity file descriptor entity to initialize
+ * @param[in] fd file descriptor to set the entity with
+ */
+static void set_read_fd_entity(struct fd_entity *fd_entity, int fd)
 {
-       return epoll_ctl(epollfd, EPOLL_CTL_DEL, fd, NULL) < 0 ? -errno : 0;
+       assert(fd_entity);
+       assert(fd >= 0);
+       fd_entity->event.events = EPOLLIN;
+       fd_entity->fd = fd;
 }
 
 /**
@@ -453,18 +486,13 @@ static int writer_create(struct writer **writer, int fd, struct log_buffer *log_
        if (!w)
                return -ENOMEM;
 
-       *w = (struct writer) {
-               ._entity = (struct fd_entity) { .dispatch_event = dispatch_event_writer },
-               .working_fd = fd,
-               .event = (struct epoll_event) {
-                       .data = (epoll_data_t) { .ptr = w },
-                       .events = EPOLLIN
-               },
-               .buf_ptr = log_buffer,
-               .readed = 0,
-               .service_writer = service_writer,
-               .service_socket = service_socket
-       };
+       init_fd_entity(&w->fd_entity, dispatch_event_writer, w);
+       set_read_fd_entity(&w->fd_entity, fd);
+       w->buf_ptr = log_buffer;
+       w->readed = 0;
+       w->service_writer = service_writer;
+       w->service_socket = service_socket;
+
        *writer = w;
        return 0;
 }
@@ -559,44 +587,69 @@ static int create_syslog_writer(struct writer ** writer, struct log_buffer *log_
  * @details Deallocate a writer
  * @param[in] w The writer to deallocate
  */
-static void writer_free(struct writerw)
+static void writer_free(struct writer *w)
 {
-       close(w->working_fd);
+       assert(w);
+       close(w->fd_entity.fd);
        free(w);
 }
 
 /**
+ * @brief Close socket that was initialized using socket_initialize function
+ * @details Closes socket entity fd
+ * @param[in] sock socet data to close
+ */
+static void socket_close(struct sock_data *sock)
+{
+       assert(sock);
+       if (sock->fd_entity.fd >= 0)
+               close(sock->fd_entity.fd);
+}
+
+/**
  * @brief Create buffer
  * @details Allocate a buffer structure
  * @param[out] lb The newly-allocated buffer
  * @param[in] buf_id The buffer ID
  * @param[in] data Buffer config data
- * @param[in] epoll_fd The epoll FD the sockets belong to
  * @return 0 on success, -errno on failure
  */
-static int buffer_create(struct log_buffer ** lb, int buf_id, struct buffer_config_data * data, int epoll_fd)
+static int buffer_create(struct log_buffer **log_buffer, int buf_id, struct buffer_config_data *data)
 {
-       *lb = calloc(1, sizeof(struct log_buffer) + data->size);
+       assert(data);
+       struct log_buffer *lb = calloc(1, sizeof(struct log_buffer) + data->size);
 
-       if (!*lb)
+       if (!lb)
                return -ENOMEM;
 
-       (*lb)->id = buf_id;
-       (*lb)->size = data->size;
-       (*lb)->sock_ctl.fd = -1;
-       (*lb)->sock_wr.fd = -1;
+       lb->id = buf_id;
+       lb->size = data->size;
 
        int r;
-       r = socket_initialize(&(*lb)->sock_ctl, *lb, service_writer_handle_req_ctrl, &data->ctl_socket, epoll_fd);
-       if (r < 0)
+       r = socket_initialize(&lb->sock_ctl, lb, service_writer_handle_req_ctrl, &data->ctl_socket);
+       if (r < 0) {
+               free(lb);
                return r;
-       r = socket_initialize(&(*lb)->sock_wr, *lb, service_writer_handle_req_pipe, &data->write_socket, epoll_fd);
-       if (r < 0)
+       }
+
+       r = socket_initialize(&lb->sock_wr, lb, service_writer_handle_req_pipe, &data->write_socket);
+       if (r < 0) {
+               socket_close(&lb->sock_ctl);
+               free(lb);
                return r;
+       }
 
+       *log_buffer = lb;
        return 0;
 }
 
+/**
+ * @brief Free reader given in ptr
+ * @details Use this function when deleting all readers in the list
+ * @param[in] ptr pointer to the reader
+ * @param[in] user_data pointer to the logger to remove readers waiting for an event
+ * @return always nonzero (the reader is to be removed)
+ */
 static int cond_reader_free(void *ptr, void *user_data)
 {
        struct reader* reader = (struct reader*)ptr;
@@ -611,15 +664,12 @@ static int cond_reader_free(void *ptr, void *user_data)
  */
 static void buffer_free(struct log_buffer* buffer)
 {
-       if (!buffer)
-               return;
+       assert(buffer);
 
        list_remove_if(&buffer->readers, NULL, cond_reader_free);
 
-       if (buffer->sock_ctl.fd >= 0)
-               close(buffer->sock_ctl.fd);
-       if (buffer->sock_wr.fd >= 0)
-               close(buffer->sock_wr.fd);
+       socket_close(&buffer->sock_ctl);
+       socket_close(&buffer->sock_wr);
 
        free(buffer);
 }
@@ -924,8 +974,9 @@ static int send_pipe(int socket, int wr_pipe, int type)
  */
 static void writer_close_fd(struct logger* server, struct writer* wr)
 {
-       remove_fd_loop(server->epollfd, wr->working_fd);
-       close(wr->working_fd);
+       assert(wr);
+       remove_fd_entity(server, &wr->fd_entity);
+       close(wr->fd_entity.fd);
 }
 
 /**
@@ -1053,7 +1104,6 @@ static int parse_command_line(const char* cmdl, struct writer* wr, struct reader
        char *tok;
        char *tok_sv;
        int retval = 0;
-       int wr_socket_fd = wr ? wr->working_fd : -1;
        struct reader * reader;
        struct stat stat_buf;
        int silence = 0;
@@ -1225,7 +1275,8 @@ static int parse_command_line(const char* cmdl, struct writer* wr, struct reader
                        goto cleanup;
                }
                reader->file.fd = fds[1];
-               retval = send_pipe(wr_socket_fd, fds[0], DLOG_FLAG_READ);
+               assert(wr);
+               retval = send_pipe(wr->fd_entity.fd, fds[0], DLOG_FLAG_READ);
                if (retval)
                        goto cleanup;
        }
@@ -1420,29 +1471,31 @@ static int service_writer_handle_req_pipe(struct logger* server, struct writer*
        if (pipe2(pipe_fd, O_CLOEXEC | O_NONBLOCK) < 0)
                return -errno;
 
-       assert(wr);
-       wr->event.data.ptr = wr;
-       wr->event.events = EPOLLIN;
        fd_clear_flags(pipe_fd[1], O_NONBLOCK);
        fcntl(pipe_fd[1], F_SETPIPE_SZ, PIPE_REQUESTED_SIZE);
 
        assert(server);
-       r = add_fd_loop(server->epollfd, pipe_fd[0], &wr->event);
+       assert(wr);
+
+       struct fd_entity pipe_entity = wr->fd_entity;
+       set_read_fd_entity(&pipe_entity, pipe_fd[0]);
+       r = add_fd_entity(server, &pipe_entity);
        if (r < 0)
                goto err_close;
 
-       r = send_pipe(wr->working_fd, pipe_fd[1], DLOG_FLAG_WRITE);
+       r = send_pipe(wr->fd_entity.fd, pipe_fd[1], DLOG_FLAG_WRITE);
        if (r < 0)
                goto err_remove;
 
        writer_close_fd(server, wr);
        wr->service_writer = service_writer_pipe;
-       wr->working_fd = pipe_fd[0];
+       wr->fd_entity = pipe_entity;
        wr->readed = 0;
        return 0;
 
 err_remove:
-       remove_fd_loop(server->epollfd, pipe_fd[0]);
+       remove_fd_entity(server, &pipe_entity);
+
 err_close:
        close(pipe_fd[0]);
        close(pipe_fd[1]);
@@ -1467,7 +1520,7 @@ static int service_writer_socket(struct logger* server, struct writer* wr, struc
 
        assert(event);
        if (event->events & EPOLLIN)
-               r = read(wr->working_fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
+               r = read(wr->fd_entity.fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
 
        if ((r == 0 || r == -1) && event->events & EPOLLHUP)
                return -EINVAL;
@@ -1487,7 +1540,7 @@ static int service_writer_socket(struct logger* server, struct writer* wr, struc
                if (r <= 0)
                        return r;
 
-               r = read(wr->working_fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
+               r = read(wr->fd_entity.fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
        } while (r > 0 || ((wr->readed >= sizeof(msg->length) && wr->readed >= msg->length)));
 
        return (r >= 0 || errno == EAGAIN)  ? 0 : r;
@@ -1504,7 +1557,7 @@ static int service_writer_socket(struct logger* server, struct writer* wr, struc
 static int service_writer_pipe(struct logger* server, struct writer* wr, struct epoll_event* event)
 {
        if (event->events & EPOLLIN) {
-               int r = read(wr->working_fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
+               int r = read(wr->fd_entity.fd, wr->buffer + wr->readed, LOG_MAX_SIZE - wr->readed);
 
                if (r == -1 && errno == EAGAIN)
                        return 0;
@@ -1545,7 +1598,7 @@ static int service_writer_kmsg(struct logger* server, struct writer* wr, struct
        if (!(event->events & EPOLLIN))
                return 0;
 
-       int r = read(wr->working_fd, wr->buffer, LOG_MAX_SIZE - 1);
+       int r = read(wr->fd_entity.fd, wr->buffer, LOG_MAX_SIZE - 1);
 
        if (r == -1 && (errno == EAGAIN || errno == EPIPE))
                return 0;
@@ -1595,7 +1648,7 @@ static int service_writer_syslog(struct logger* server, struct writer* wr, struc
        if (!(event->events & EPOLLIN))
                return 0;
 
-       int r = read(wr->working_fd, wr->buffer, LOG_MAX_SIZE - 1);
+       int r = read(wr->fd_entity.fd, wr->buffer, LOG_MAX_SIZE - 1);
 
        if (r == -1 && (errno == EAGAIN || errno == EPIPE))
                return 0;
@@ -1643,25 +1696,23 @@ static void service_all_readers(struct logger* server, int force_push)
  * @param[in] buffer The buffer to whom the socket belongs
  * @param[in] type Type of the socket
  * @param[in] data Socket config data
- * @param[in] epoll_fd Epoll FD to manage the socket
  * @return 0 on success, else -errno
  */
-int socket_initialize(struct sock_data *sock, struct log_buffer *buffer, service_socket_t service_socket, struct socket_config_data *data, int epoll_fd)
+static int socket_initialize(struct sock_data *sock, struct log_buffer *buffer, service_socket_t service_socket, struct socket_config_data *data)
 {
-       if ((sock->fd = listen_fd_create(data->path, data->permissions)) < 0)
-               return sock->fd;
+       assert(data);
+       int sock_fd = listen_fd_create(data->path, data->permissions);
+       if (sock_fd < 0)
+               return sock_fd;
 
        int r = change_owners(data->path, data->owner, data->group);
        if (r < 0)
                return r;
 
-       sock->_entity.dispatch_event = dispatch_event_sock;
+       init_fd_entity(&sock->fd_entity, dispatch_event_sock, sock);
        sock->service_socket = service_socket;
-       sock->event.data.ptr = sock;
        sock->buf_ptr = buffer;
-       sock->event.events = EPOLLIN;
-
-       add_fd_loop(epoll_fd, sock->fd, &sock->event);
+       set_read_fd_entity(&sock->fd_entity, sock_fd);
 
        return 0;
 }
@@ -1678,7 +1729,7 @@ static void logger_add_writer(struct logger* l, struct writer* wr)
        assert(wr);
 
        list_add(&l->writers, wr);
-       add_fd_loop(l->epollfd, wr->working_fd, &wr->event);
+       add_fd_entity(l, &wr->fd_entity);
 }
 
 /**
@@ -1699,16 +1750,21 @@ static int logger_create(struct logger_config_data *data, struct logger *l)
        l->buffering = data->buffering;
 
        int i;
-       for (i = 0; i < LOG_ID_MAX; i++) {
+       for (i = 0; i < LOG_ID_MAX; i++)
                if (data->is_buffer_enabled[i]) {
-                       int r = buffer_create(l->buffers + i, i, data->buffers + i, l->epollfd);
+                       int r = buffer_create(&l->buffers[i], i, data->buffers + i);
                        if (r < 0)
                                return r;
                }
-       }
+
+       for (i = 0; i < LOG_ID_MAX; i++)
+               if (l->buffers[i]) {
+                       add_fd_entity(l, &l->buffers[i]->sock_ctl.fd_entity);
+                       add_fd_entity(l, &l->buffers[i]->sock_wr.fd_entity);
+               }
 
        /* TODO: make writers creation optional/configurable */
-       int (*writers_factories[LOG_ID_MAX])(struct writer ** writer, struct log_buffer *log_buf) = {
+       int (*writers_factories[LOG_ID_MAX])(struct writer **writer, struct log_buffer *log_buf) = {
                [LOG_ID_KMSG] = create_kmsg_writer,
                [LOG_ID_SYSLOG] = create_syslog_writer,
        };
@@ -1718,6 +1774,7 @@ static int logger_create(struct logger_config_data *data, struct logger *l)
                        int r = writers_factories[i](&wr, l->buffers[i]);
                        if (r < 0)
                                return r;
+
                        logger_add_writer(l, wr);
                }
 
@@ -1735,13 +1792,17 @@ static int cond_writer_free(void *ptr, void *user_data)
  * @details Deallocate the logger and its auxiliary structures
  * @param[in] l The logger server
  */
-static void logger_free(struct loggerl)
+static void logger_free(struct logger *l)
 {
+       assert(l);
+
        list_remove_if(&l->writers, NULL, cond_writer_free);
+       list_remove_if(&l->extra_readers, l, cond_reader_free);
 
        int j;
        for (j = 0; j < LOG_ID_MAX; j++)
-               buffer_free(l->buffers[j]);
+               if (l->buffers[j])
+                       buffer_free(l->buffers[j]);
 
        if (l->epollfd >= 0)
                close(l->epollfd);
@@ -1777,7 +1838,9 @@ static void dispatch_event_sock(struct logger *server, struct epoll_event *event
 {
        assert(event);
        struct sock_data const * const sock = (struct sock_data const * const) event->data.ptr;
-       int sock_pipe = accept4(sock->fd, NULL, NULL, SOCK_NONBLOCK);
+
+       assert(sock);
+       int sock_pipe = accept4(sock->fd_entity.fd, NULL, NULL, SOCK_NONBLOCK);
        if (sock_pipe < 0)
                return;