[lxc-devel] [PATCH] lxc-copy: cleanup

[Christian Brauner]

On Jan 29, 2016 19:00, "Serge Hallyn" <serge.hallyn at ubuntu.com> wrote:
>
> Quoting Christian Brauner (christian.brauner at mailbox.org):
> > Signed-off-by: Christian Brauner <christian.brauner at mailbox.org>
>
> So on this free_mnts() thing.  If you (as this patch makes it look
> like you were risking) run it twice, you'll presumably crash.  The
> callers always pass in the same global variables.  So I think that
> (a) free_mnts() should take no arguments and just use the globals,
> and then (b) free_mnts() should set mnt_table to NULL and
> mnt_table_size = 0.
>
> Does that make sense or am I misreading something?

Yup. I wanted to do it without using globals in the first place but
my_parser() cannot have additional arguments since it is typdef that way in
arguments.h. I can't work on it right now. But I'll get back to you right
after the fosdem.

>
> > ---
> >  src/lxc/lxc_copy.c | 44 ++++++++++++++++++--------------------------
> >  1 file changed, 18 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/lxc/lxc_copy.c b/src/lxc/lxc_copy.c
> > index 9812176..5919bf6 100644
> > --- a/src/lxc/lxc_copy.c
> > +++ b/src/lxc/lxc_copy.c
> > @@ -240,41 +240,36 @@ static int mk_rand_ovl_dirs(struct mnts *mnts,
unsigned int num, struct lxc_argu
> >       char upperdir[MAXPATHLEN];
> >       char workdir[MAXPATHLEN];
> >       unsigned int i;
> > -     int ret = 0;
> > -     struct mnts *m;
> > +     int ret;
> > +     struct mnts *m = NULL;
> >
> > -     for (i = 0; i < num; i++) {
> > -             m = mnts + i;
> > +     for (i = 0, m = mnts; i < num; i++, m++) {
> >               if ((m->mnt_type == LXC_MNT_OVL) || (m->mnt_type ==
LXC_MNT_AUFS)) {
> >                       ret = snprintf(upperdir, MAXPATHLEN,
"%s/%s/delta#XXXXXX",
> >                                       arg->newpath, arg->newname);
> >                       if (ret < 0 || ret >= MAXPATHLEN)
> > -                             goto err;
> > +                             return -1;
> >                       if (!mkdtemp(upperdir))
> > -                             goto err;
> > +                             return -1;
> >                       m->upper = strdup(upperdir);
> >                       if (!m->upper)
> > -                             goto err;
> > +                             return -1;
> >               }
> >
> >               if (m->mnt_type == LXC_MNT_OVL) {
> >                       ret = snprintf(workdir, MAXPATHLEN,
"%s/%s/work#XXXXXX",
> >                                       arg->newpath, arg->newname);
> >                       if (ret < 0 || ret >= MAXPATHLEN)
> > -                             goto err;
> > +                             return -1;
> >                       if (!mkdtemp(workdir))
> > -                             goto err;
> > +                             return -1;
> >                       m->workdir = strdup(workdir);
> >                       if (!m->workdir)
> > -                             goto err;
> > +                             return -1;
> >               }
> >       }
> >
> >       return 0;
> > -
> > -err:
> > -     free_mnts(mnt_table, mnt_table_size);
> > -     return -1;
> >  }
> >
> >  static char *construct_path(char *path, bool as_prefix)
> > @@ -343,7 +338,6 @@ static char *set_mnt_entry(struct mnts *m)
> >       return mntentry;
> >
> >  err:
> > -     free_mnts(mnt_table, mnt_table_size);
> >       free(mntentry);
> >       return NULL;
> >  }
> > @@ -378,7 +372,6 @@ static int do_clone_ephemeral(struct lxc_container
*c,
> >       int ret = 0;
> >       bool bret = true;
> >       struct lxc_container *clone;
> > -     struct mnts *n;
> >
> >       lxc_attach_options_t attach_options = LXC_ATTACH_OPTIONS_DEFAULT;
> >       attach_options.env_policy = LXC_ATTACH_CLEAR_ENV;
> > @@ -408,13 +401,13 @@ static int do_clone_ephemeral(struct
lxc_container *c,
> >               goto err;
> >
> >       /* allocate and set mount entries */
> > -     for (i = 0; i < mnt_table_size; i++) {
> > +     struct mnts *n = NULL;
> > +     for (i = 0, n = mnt_table; i < mnt_table_size; i++, n++) {
> >               char *mntentry = NULL;
> > -             n = mnt_table + i;
> > -             if ((mntentry = set_mnt_entry(n))) {
> > +             mntentry = set_mnt_entry(n);
> > +             if (mntentry)
> >                       bret = clone->set_config_item(clone,
"lxc.mount.entry", mntentry);
> > -                     free(mntentry);
> > -             }
> > +             free(mntentry);
> >               if (!mntentry || !bret)
> >                       goto err;
> >       }
> > @@ -432,7 +425,7 @@ static int do_clone_ephemeral(struct lxc_container
*c,
> >       }
> >
> >       if (!clone->start(clone, 0, NULL)) {
> > -             if (!(clone->lxc_conf->ephemeral == 1))
> > +             if (clone->lxc_conf->ephemeral != 1)
> >                       goto err;
> >               goto put;
> >       }
> > @@ -492,17 +485,16 @@ static int do_clone_task(struct lxc_container *c,
enum task task, int flags,
> >  static void free_mnts(struct mnts *m, unsigned int num)
> >  {
> >       unsigned int i;
> > -     struct mnts *n;
> > +     struct mnts *n = NULL;
> >
> > -     for (i = 0; i < num; i++) {
> > -             n = m + i;
> > +     for (i = 0, n = m; i < num; i++, n++) {
> >               free(n->src);
> >               free(n->dest);
> >               free(n->options);
> >               free(n->upper);
> >               free(n->workdir);
> >       }
> > -     free(m);
> > +     free(n);
> >  }
> >
> >  /* we pass fssize in bytes */
> > --
> > 2.7.0
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20160129/cd601118/attachment.html>