Re: [Libguestfs] [PATCH 1/3] builder/virt-index-validate: try to cleanup in any occasion

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?

- Do we test this program under valgrind? If not, we probably should do.