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