Pino Toscano
2014-Mar-20 15:24 UTC
Re: [Libguestfs] [PATCH 1/3] builder/virt-index-validate: try to cleanup in any occasion
On Thursday 20 March 2014 14:15:29 Richard W.M. Jones wrote:> On Thu, Mar 20, 2014 at 02:48:11PM +0100, Pino Toscano wrote: > > Always close the file (ignoring its result) after a parsing, and > > cleanup the parse_context object before any exit(). > > > > This eases the debugging of memory issues in the actual parser. > > --- > > > > builder/index-validate.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/builder/index-validate.c b/builder/index-validate.c > > index 4b7fe93..fed0f81 100644 > > --- a/builder/index-validate.c > > +++ b/builder/index-validate.c > > @@ -62,6 +62,7 @@ main (int argc, char *argv[]) > > > > struct section *sections; > > struct parse_context context; > > FILE *in; > > > > + int ret; > > > > setlocale (LC_ALL, ""); > > bindtextdomain (PACKAGE, LOCALEBASEDIR); > > > > @@ -109,19 +110,22 @@ main (int argc, char *argv[]) > > > > exit (EXIT_FAILURE); > > > > } > > > > - if (do_parse (&context, in) != 0) { > > - fprintf (stderr, _("%s: '%s' could not be validated, see errors > > above\n"), + ret = do_parse (&context, in); > > + > > + if (fclose (in) == EOF) { > > + fprintf (stderr, _("%s: %s: error closing input file: %m > > (ignored)\n"),> > > program_name, input); > > > > - exit (EXIT_FAILURE); > > > > } > > > > - if (fclose (in) == EOF) { > > - fprintf (stderr, _("%s: %s: error reading input file: %m\n"), > > + if (ret != 0) { > > + parse_context_free (&context); > > + fprintf (stderr, _("%s: '%s' could not be validated, see errors > > above\n"),> > > program_name, input); > > > > exit (EXIT_FAILURE); > > > > } > > > > if (compat_1_24_1 && context.seen_comments) { > > > > + parse_context_free (&context); > > > > fprintf (stderr, _("%s: %s contains comments which will not > > work with virt-builder 1.24.1\n"),> > > program_name, input); > > > > exit (EXIT_FAILURE); > > > > @@ -134,6 +138,7 @@ main (int argc, char *argv[]) > > > > if (compat_1_24_0) { > > > > if (strchr (sections->name, '_')) { > > > > + parse_context_free (&context); > > > > fprintf (stderr, _("%s: %s: section [%s] has invalid > > characters which will not work with virt-builder > > 1.24.0\n"),> > > program_name, input, sections->name); > > > > exit (EXIT_FAILURE); > > > > @@ -144,6 +149,7 @@ main (int argc, char *argv[]) > > > > if (compat_1_24_0) { > > > > if (strchr (fields->key, '[') || > > > > strchr (fields->key, ']')) { > > > > + parse_context_free (&context); > > > > fprintf (stderr, _("%s: %s: section [%s], field '%s' has > > invalid characters which will not work with virt-builder > > 1.24.0\n"),> > > program_name, input, sections->name, > > fields->key); > > > > exit (EXIT_FAILURE); > > > > @@ -152,6 +158,7 @@ main (int argc, char *argv[]) > > > > if (compat_1_24_1) { > > > > if (strchr (fields->key, '.') || > > > > strchr (fields->key, ',')) { > > > > + parse_context_free (&context); > > > > fprintf (stderr, _("%s: %s: section [%s], field '%s' has > > invalid characters which will not work with virt-builder > > 1.24.1\n"),> > > program_name, input, sections->name, > > fields->key); > > > > exit (EXIT_FAILURE); > > > > @@ -162,6 +169,7 @@ main (int argc, char *argv[]) > > > > } > > > > if (compat_1_24_0 && !seen_sig) { > > > > + parse_context_free (&context); > > > > fprintf (stderr, _("%s: %s: section [%s] is missing a 'sig' > > field which will not work with virt-builder 1.24.0\n"),> > > program_name, input, sections->name); > > > > exit (EXIT_FAILURE); > > ACK. > > Two thoughts though: > > - Would a cleanup attribute be a better choice?Possibly, although it didn't seem worth to me, for such a simple testing tool.> - Do we test this program under valgrind? If not, we probably should > do.Worth doing, patch coming shortly. -- Pino Toscano
Pino Toscano
2014-Mar-20 15:25 UTC
[Libguestfs] [PATCH] builder: allow to run website tests under valgrind
--- builder/website/Makefile.am | 3 +++ builder/website/validate.sh | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/builder/website/Makefile.am b/builder/website/Makefile.am index a5b1bcf..6b609f6 100644 --- a/builder/website/Makefile.am +++ b/builder/website/Makefile.am @@ -45,3 +45,6 @@ CLEANFILES = *~ # Validates the index file. TESTS_ENVIRONMENT = $(top_builddir)/run --test TESTS = validate.sh + +check-valgrind: + $(MAKE) VG="$(top_builddir)/run @VG@" check diff --git a/builder/website/validate.sh b/builder/website/validate.sh index 1c535e7..bd9a4ed 100755 --- a/builder/website/validate.sh +++ b/builder/website/validate.sh @@ -19,6 +19,6 @@ export LANG=C set -e -../virt-index-validate $srcdir/index -../virt-index-validate $srcdir/index.asc +$VG ../virt-index-validate $srcdir/index +$VG ../virt-index-validate $srcdir/index.asc -- 1.8.3.1
Richard W.M. Jones
2014-Mar-20 15:32 UTC
Re: [Libguestfs] [PATCH] builder: allow to run website tests under valgrind
On Thu, Mar 20, 2014 at 04:25:07PM +0100, Pino Toscano wrote:> --- > builder/website/Makefile.am | 3 +++ > builder/website/validate.sh | 4 ++-- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/builder/website/Makefile.am b/builder/website/Makefile.am > index a5b1bcf..6b609f6 100644 > --- a/builder/website/Makefile.am > +++ b/builder/website/Makefile.am > @@ -45,3 +45,6 @@ CLEANFILES = *~ > # Validates the index file. > TESTS_ENVIRONMENT = $(top_builddir)/run --test > TESTS = validate.sh > + > +check-valgrind: > + $(MAKE) VG="$(top_builddir)/run @VG@" check > diff --git a/builder/website/validate.sh b/builder/website/validate.sh > index 1c535e7..bd9a4ed 100755 > --- a/builder/website/validate.sh > +++ b/builder/website/validate.sh > @@ -19,6 +19,6 @@ > export LANG=C > set -e > > -../virt-index-validate $srcdir/index > -../virt-index-validate $srcdir/index.asc > +$VG ../virt-index-validate $srcdir/index > +$VG ../virt-index-validate $srcdir/index.ascACK. I'm slightly surprised that virt-builder isn't being run under valgrind :-( Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org