Libguestfs - Mar 2014 - [PATCH] builder: allow to run website tests under valgrind

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

---
 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

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.asc
ACK.

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