[lxc-devel] [PATCH] allow multiple monitor clients
Serge Hallyn
serge.hallyn at ubuntu.com
Wed Apr 24 14:09:52 UTC 2013
Quoting Dwight Engen (dwight.engen at oracle.com):
> Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
> ---
> .gitignore | 1 +
> src/lxc/Makefile.am | 2 +
> src/lxc/lxc_console.c | 4 +-
> src/lxc/lxc_monitor.c | 2 +
> src/lxc/lxc_monitord.c | 381 +++++++++++++++++++++++++++++++++++++++++++++++++
> src/lxc/lxccontainer.c | 6 +-
> src/lxc/mainloop.c | 7 +-
> src/lxc/mainloop.h | 7 +-
> src/lxc/monitor.c | 192 ++++++++++++++++++-------
> src/lxc/monitor.h | 10 +-
> src/lxc/start.c | 4 +-
> src/lxc/utils.h | 26 ++++
> 12 files changed, 578 insertions(+), 64 deletions(-)
> create mode 100644 src/lxc/lxc_monitord.c
>
> diff --git a/.gitignore b/.gitignore
> index 905a2dc..c614a75 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -52,6 +52,7 @@ src/lxc/lxc-info
> src/lxc/lxc-init
> src/lxc/lxc-kill
> src/lxc/lxc-monitor
> +src/lxc/lxc-monitord
> src/lxc/lxc-netstat
> src/lxc/lxc-ps
> src/lxc/lxc-restart
> diff --git a/src/lxc/Makefile.am b/src/lxc/Makefile.am
> index ebeca466..1fa0fa8 100644
> --- a/src/lxc/Makefile.am
> +++ b/src/lxc/Makefile.am
> @@ -150,6 +150,7 @@ bin_PROGRAMS = \
> lxc-start \
> lxc-execute \
> lxc-monitor \
> + lxc-monitord \
> lxc-wait \
> lxc-console \
> lxc-freeze \
> @@ -181,6 +182,7 @@ lxc_freeze_SOURCES = lxc_freeze.c
> lxc_info_SOURCES = lxc_info.c
> lxc_init_SOURCES = lxc_init.c
> lxc_monitor_SOURCES = lxc_monitor.c
> +lxc_monitord_SOURCES = lxc_monitord.c
> lxc_restart_SOURCES = lxc_restart.c
> lxc_start_SOURCES = lxc_start.c
> lxc_stop_SOURCES = lxc_stop.c
> diff --git a/src/lxc/lxc_console.c b/src/lxc/lxc_console.c
> index 643c442..f6659f6 100644
> --- a/src/lxc/lxc_console.c
> +++ b/src/lxc/lxc_console.c
> @@ -241,7 +241,7 @@ Type <Ctrl+%1$c q> to exit the console, \
> goto out_mainloop_open;
> }
>
> - err = lxc_mainloop(&descr);
> + err = lxc_mainloop(&descr, -1);
> if (err) {
> ERROR("mainloop returned an error");
> goto out_mainloop_open;
> @@ -255,7 +255,7 @@ out_mainloop_open:
> out:
> /* Restore previous terminal parameter */
> tcsetattr(0, TCSAFLUSH, &oldtios);
> -
> +
> /* Return to line it is */
> printf("\n");
>
> diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> index 8c15869..0ca829f 100644
> --- a/src/lxc/lxc_monitor.c
> +++ b/src/lxc/lxc_monitor.c
> @@ -87,6 +87,8 @@ int main(int argc, char *argv[])
> return -1;
> }
>
> + lxc_monitord_spawn(my_args.lxcpath);
> +
> fd = lxc_monitor_open(my_args.lxcpath);
> if (fd < 0)
> return -1;
> diff --git a/src/lxc/lxc_monitord.c b/src/lxc/lxc_monitord.c
> new file mode 100644
> index 0000000..36763ae
> --- /dev/null
> +++ b/src/lxc/lxc_monitord.c
> @@ -0,0 +1,381 @@
> +/*
> + * lxc: linux Container library
> + *
> + * Copyright © 2012 Oracle.
> + *
> + * Authors:
> + * Dwight Engen <dwight.engen at oracle.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <signal.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/param.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <netinet/in.h>
> +#include <net/if.h>
> +
> +#include <lxc/af_unix.h>
> +#include <lxc/log.h>
> +#include <lxc/mainloop.h>
> +#include <lxc/monitor.h>
> +#include <lxc/utils.h>
> +
> +lxc_log_define(lxc_monitord, lxc);
> +
> +static struct lxc_monitor mon;
> +
> +static void lxc_monitord_cleanup(void);
> +
> +/*
> + * Defines the structure to store the monitor information
> + * @lxcpath : the path being monitored
> + * @fifofd : the file descriptor for publishers (containers) to write state
> + * @listenfd : the file descriptor for subscribers (lxc-monitors) to connect
> + * @clientfds : accepted client file descriptors
> + * @clientfds_cnt: the count of valid fds in clientfds
> + * @descr : the lxc_mainloop state
> + */
> +struct lxc_monitor {
> + const char *lxcpath;
> + int fifofd;
> + int listenfd;
> + int clientfds[1024];
Sorry, one more needed fix. You hardcode the # clientfds to 1024,
but you don't check for that when adding a clientfd. While fixing
that, unless you make the # fds dynamic, could you use a #define?
(Also a few more comments below.)
> + int clientfds_cnt;
> + struct lxc_epoll_descr descr;
> +};
> +
> +static void lxc_monitord_sockfd_remove(struct lxc_monitor *mon, int fd) {
> + int i;
> +
> + if (lxc_mainloop_del_handler(&mon->descr, fd))
> + CRIT("fd:%d not found in mainloop", fd);
> + close(fd);
> +
> + for (i = 0; i < mon->clientfds_cnt; i++) {
> + if (mon->clientfds[i] == fd)
> + break;
> + }
> + if (i >= mon->clientfds_cnt) {
> + CRIT("fd:%d not found in clients array", fd);
> + lxc_monitord_cleanup();
> + exit(EXIT_FAILURE);
> + }
> +
> + memmove(&mon->clientfds[i], &mon->clientfds[i+1],
> + (&mon->clientfds[mon->clientfds_cnt-1] - &mon->clientfds[i]) * sizeof(int));
Hm, could this just be:
memmove(&mon->clientfds[i], &mon->clientfds[i+1],
(mon->clientfds_cnt - i -1) * sizeof(int));
?
> + mon->clientfds_cnt--;
> +}
> +
...
> +int main(int argc, char *argv[])
> +{
> + int ret,pipefd;
> + char *lxcpath = argv[1];
> + char logpath[PATH_MAX];
> + sigset_t mask;
> +
> + ret = snprintf(logpath, sizeof(logpath), "%s/lxc-monitord.log",
> + lxcpath);
> + if (ret < 0 || ret >= sizeof(logpath))
> + return EXIT_FAILURE;
> +
> + ret = lxc_log_init(NULL, logpath, "NOTICE", "lxc-monitord", 0);
> + if (ret)
> + return ret;
> +
> + pipefd = atoi(argv[2]);
Someone could walk in and try to run this by hand to see what it
does - so a check on argc is I think warranted.
...
> +int lxc_monitord_spawn(const char *lxcpath)
> {
> - return close(fd);
> + pid_t pid1,pid2;
> + int pipefd[2];
> + char pipefd_str[10];
> +
> + char * const args[] = {
> + "/usr/bin/lxc-monitord",
> + (char *)lxcpath,
> + pipefd_str,
> + NULL,
> + };
> +
> + /* double fork to avoid zombies when monitord exits */
> + pid1 = fork();
> + if (pid1 < 0) {
> + SYSERROR("failed to fork");
> + return -1;
> + }
> +
> + if (pid1) {
> + waitpid(pid1, NULL, 0);
> + return 0;
> + }
> +
> + if (pipe(pipefd) < 0) {
> + SYSERROR("failed to create pipe");
> + exit(EXIT_FAILURE);
> + }
> +
> + pid2 = fork();
Please do check for < 0. As ppl use nested containers, I coudl see them
hitting their local pid max.
> + if (pid2) {
> + char c;
> + /* wait for daemon to create socket */
> + close(pipefd[1]);
> + /* sync with child, we're ignoring the return from read
> + * because regardless if it works or not, either way we've
> + * synced with the child process. the if-empty-statement
> + * construct is to quiet the warn-unused-result warning.
> + */
> + if (read(pipefd[0], &c, 1)) ;
> + close(pipefd[0]);
> + exit(EXIT_SUCCESS);
> + }
> +
> + umask(0);
> + if (setsid() < 0) {
> + SYSERROR("failed to setsid");
> + exit(EXIT_FAILURE);
> + }
> + close(0);
> + close(1);
> + close(2);
> + open("/dev/null", O_RDONLY);
> + open("/dev/null", O_RDWR);
> + open("/dev/null", O_RDWR);
> + close(pipefd[0]);
> + sprintf(pipefd_str, "%d", pipefd[1]);
> + execvp(args[0], args);
> + exit(EXIT_FAILURE);
> }
> diff --git a/src/lxc/monitor.h b/src/lxc/monitor.h
> index 8bef4c7..cd59ee8 100644
> --- a/src/lxc/monitor.h
> +++ b/src/lxc/monitor.h
> @@ -24,6 +24,9 @@
> #define __monitor_h
>
> #include <sys/param.h>
> +#include <sys/un.h>
> +
> +#include <lxc/conf.h>
>
> typedef enum {
> lxc_msg_state,
> @@ -32,11 +35,14 @@ typedef enum {
>
> struct lxc_msg {
> lxc_msg_type_t type;
> - char name[MAXPATHLEN];
> + char name[NAME_MAX+1];
> int value;
> };
>
> -void lxc_monitor_send_state(const char *name, lxc_state_t state,
> +extern int lxc_monitor_open(const char *lxcpath);
> +extern int lxc_monitor_sock_name(const char *lxcpath, struct sockaddr_un *addr);
> +extern void lxc_monitor_send_state(const char *name, lxc_state_t state,
> const char *lxcpath);
> +extern int lxc_monitord_spawn(const char *lxcpath);
>
> #endif
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index 0a0cc40..fd96d4f 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -390,7 +390,7 @@ int lxc_poll(const char *name, struct lxc_handler *handler)
> #endif
> }
>
> - return lxc_mainloop(&descr);
> + return lxc_mainloop(&descr, -1);
>
> out_mainloop_open:
> lxc_mainloop_close(&descr);
> @@ -808,7 +808,7 @@ int lxc_spawn(struct lxc_handler *handler)
> /* TODO - pass lxc.cgroup.dir (or user's pam cgroup) in for first argument */
> if ((handler->cgroup = lxc_cgroup_path_create(NULL, name)) == NULL)
> goto out_delete_net;
> -
> +
> if (lxc_cgroup_enter(handler->cgroup, handler->pid) < 0)
> goto out_delete_net;
>
> diff --git a/src/lxc/utils.h b/src/lxc/utils.h
> index 8954503..8e6a748 100644
> --- a/src/lxc/utils.h
> +++ b/src/lxc/utils.h
> @@ -32,4 +32,30 @@ extern int mkdir_p(const char *dir, mode_t mode);
> */
> extern const char *default_lxc_path(void);
>
> +/**
> + * BUILD_BUG_ON - break compile if a condition is true.
> + * @condition: the condition which the compiler should know is false.
> + *
> + * If you have some code which relies on certain constants being equal, or
> + * other compile-time-evaluated condition, you should use BUILD_BUG_ON to
> + * detect if someone changes it.
> + *
> + * The implementation uses gcc's reluctance to create a negative array, but
> + * gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
> + * to inline functions). So as a fallback we use the optimizer; if it can't
> + * prove the condition is false, it will cause a link error on the undefined
> + * "__build_bug_on_failed". This error message can be harder to track down
> + * though, hence the two different methods.
> + */
> +#ifndef __OPTIMIZE__
> +#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +#else
> +extern int __build_bug_on_failed;
> +#define BUILD_BUG_ON(condition) \
> + do { \
> + ((void)sizeof(char[1 - 2*!!(condition)])); \
> + if (condition) __build_bug_on_failed = 1; \
> + } while(0)
> +#endif
> +
> #endif
> --
> 1.8.1.4
>
More information about the lxc-devel
mailing list