[lxc-devel] [PATCH] fix fd leak in test-concurrent

[Dwight Engen]

On Fri, 7 Mar 2014 16:12:23 -0600
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:

> Quoting Dwight Engen (dwight.engen at oracle.com):
> > Opening a debug log for every thread at every iteration of
> > test-concurrent causes it to quickly run out of fd's because this
> > fd is leaked. Fix this by adding a new api: lxc_log_close().
> > 
> > As Caglar noted, the log handling is in general a bit "interesting"
> > because a logfile can be opened through the per-container api
> > c->set_config_item("lxc.logfile") but lxc_log_fd is now per-thread
> > data. It just so happens in test-concurrent that there is a 1:1
> > mapping of threads to logfiles.
> 
> Will at_exit work at thread exit?

I don't think it does. I thought a little about trying to do the close
automatically internal to lxc on the last ref in lxc_container_put()
but that doesn't really make sense. I guess that's what seems weird to
me, that opening the log is via a per-container api, but logging is
really per-process (or per-thread with Caglar's recent change).

> > Split out getting debug logs from quiet since I think they are
> > useful separately. If debug is specified, get a log of any mode,
> > not just during start.
> > 
> > Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
> 
> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> 
> > ---
> >  src/lxc/log.c          | 10 ++++++++++
> >  src/lxc/lxccontainer.h |  5 +++++
> >  src/tests/concurrent.c | 19 ++++++++++++++-----
> >  3 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/lxc/log.c b/src/lxc/log.c
> > index a9f98a4..d5b862e 100644
> > --- a/src/lxc/log.c
> > +++ b/src/lxc/log.c
> > @@ -368,6 +368,16 @@ extern int lxc_log_init(const char *name,
> > const char *file, return ret;
> >  }
> >  
> > +extern void lxc_log_close(void)
> > +{
> > +	if (lxc_log_fd == -1)
> > +		return;
> > +	close(lxc_log_fd);
> > +	lxc_log_fd = -1;
> > +	free(log_fname);
> > +	log_fname = NULL;
> > +}
> > +
> >  /*
> >   * This is called when we read a lxc.loglevel entry in a lxc.conf
> > file.  This
> >   * happens after processing command line arguments, which override
> > the .conf diff --git a/src/lxc/lxccontainer.h
> > b/src/lxc/lxccontainer.h index b9873eb..ba15ab7 100644
> > --- a/src/lxc/lxccontainer.h
> > +++ b/src/lxc/lxccontainer.h
> > @@ -865,6 +865,11 @@ int list_active_containers(const char
> > *lxcpath, char ***names, struct lxc_contai */
> >  int list_all_containers(const char *lxcpath, char ***names, struct
> > lxc_container ***cret); 
> > +/*!
> > + * \brief Close log file.
> > + */
> > +void lxc_log_close(void);
> > +
> >  #ifdef  __cplusplus
> >  }
> >  #endif
> > diff --git a/src/tests/concurrent.c b/src/tests/concurrent.c
> > index c4d2ad5..acabbed 100644
> > --- a/src/tests/concurrent.c
> > +++ b/src/tests/concurrent.c
> > @@ -29,6 +29,7 @@
> >  
> >  static int nthreads = 5;
> >  static int iterations = 1;
> > +static int debug = 0;
> >  static int quiet = 0;
> >  static int delay = 0;
> >  static const char *template = "busybox";
> > @@ -40,6 +41,7 @@ static const struct option options[] = {
> >      { "delay",       required_argument, NULL, 'd' },
> >      { "modes",       required_argument, NULL, 'm' },
> >      { "quiet",       no_argument,       NULL, 'q' },
> > +    { "debug",       no_argument,       NULL, 'D' },
> >      { "help",        no_argument,       NULL, '?' },
> >      { 0, 0, 0, 0 },
> >  };
> > @@ -54,6 +56,7 @@ static void usage(void) {
> >          "  -d, --delay=N                Delay in seconds between
> > start and stop\n" "  -m, --modes=<mode,mode,...>  Modes to run
> > (create, start, stop, destroy)\n" "  -q, --quiet
> > Don't produce any output\n"
> > +        "  -D, --debug                  Create a debug log\n"
> >          "  -?, --help                   Give this help list\n"
> >          "\n"
> >          "Mandatory or optional arguments to long options are also
> > mandatory or optional\n" @@ -81,6 +84,11 @@ static void
> > do_function(void *arguments) return;
> >      }
> >  
> > +    if (debug) {
> > +	c->set_config_item(c, "lxc.loglevel", "DEBUG");
> > +	c->set_config_item(c, "lxc.logfile", name);
> > +    }
> > +
> >      if (strcmp(args->mode, "create") == 0) {
> >          if (!c->is_defined(c)) {
> >              if (!c->create(c, template, NULL, NULL, 1, NULL)) {
> > @@ -91,10 +99,6 @@ static void do_function(void *arguments)
> >      } else if(strcmp(args->mode, "start") == 0) {
> >          if (c->is_defined(c) && !c->is_running(c)) {
> >              c->want_daemonize(c, true);
> > -            if (!quiet) {
> > -                c->set_config_item(c, "lxc.loglevel", "DEBUG");
> > -                c->set_config_item(c, "lxc.logfile", name);
> > -            }
> >              if (!c->start(c, false, NULL)) {
> >                  fprintf(stderr, "Starting the container (%s)
> > failed...\n", name); goto out;
> > @@ -127,6 +131,8 @@ static void do_function(void *arguments)
> >      args->return_code = 0;
> >  out:
> >      lxc_container_put(c);
> > +    if (debug)
> > +	lxc_log_close();
> >  }
> >  
> >  static void *concurrent(void *arguments)
> > @@ -148,7 +154,7 @@ int main(int argc, char *argv[]) {
> >  
> >      pthread_attr_init(&attr);
> >  
> > -    while ((opt = getopt_long(argc, argv, "j:i:t:d:m:q", options,
> > NULL)) != -1) {
> > +    while ((opt = getopt_long(argc, argv, "j:i:t:d:m:qD", options,
> > NULL)) != -1) { switch(opt) {
> >          case 'j':
> >              nthreads = atoi(optarg);
> > @@ -165,6 +171,9 @@ int main(int argc, char *argv[]) {
> >          case 'q':
> >              quiet = 1;
> >              break;
> > +	case 'D':
> > +	    debug = 1;
> > +	    break;
> >          case 'm': {
> >              char *mode_tok, *tok, *saveptr = NULL;
> >  
> > -- 
> > 1.8.5.3
> > 
> >