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

[Richard W.M. Jones]

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);
> -- 
> 1.8.3.1

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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v