thr3ads.net - Libguestfs - [Libguestfs] [PATCH 1/3] builder/virt-index-validate: try to cleanup in any occasion [Mar 2014]

If this information is useful, please help other people find it:
Share via:

Pino Toscano

2014-Mar-20 13:48 UTC

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

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

Pino Toscano

2014-Mar-20 13:48 UTC

head link

[Libguestfs] [PATCH 2/3] builder: rename and make public the section/field free functions

They will be needed also elsewhere.
---
 builder/index-struct.c | 19 ++++++++-----------
 builder/index-struct.h | 10 ++++++++++
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/builder/index-struct.c b/builder/index-struct.c
index f32534c..eacca6c 100644
--- a/builder/index-struct.c
+++ b/builder/index-struct.c
@@ -24,9 +24,6 @@
 
 #include "index-struct.h"
 
-static void free_section (struct section *section);
-static void free_field (struct field *field);
-
 void
 parse_context_init (struct parse_context *context)
 {
@@ -36,25 +33,25 @@ parse_context_init (struct parse_context *context)
 void
 parse_context_free (struct parse_context *context)
 {
-  free_section (context->parsed_index);
+  section_free (context->parsed_index);
 }
 
-static void
-free_section (struct section *section)
+void
+section_free (struct section *section)
 {
   if (section) {
-    free_section (section->next);
+    section_free (section->next);
     free (section->name);
-    free_field (section->fields);
+    field_free (section->fields);
     free (section);
   }
 }
 
-static void
-free_field (struct field *field)
+void
+field_free (struct field *field)
 {
   if (field) {
-    free_field (field->next);
+    field_free (field->next);
     free (field->key);
     free (field->subkey);
     free (field->value);
diff --git a/builder/index-struct.h b/builder/index-struct.h
index 7e16637..3edd06d 100644
--- a/builder/index-struct.h
+++ b/builder/index-struct.h
@@ -53,4 +53,14 @@ extern void parse_context_init (struct parse_context *state);
 /* Free the content of a parse_context.  The actual pointer is not freed. */
 extern void parse_context_free (struct parse_context *state);
 
+/* Free the content of a section, recursively freeing also its fields.
+ * The actual pointer is not freed.
+ */
+extern void section_free (struct section *section);
+
+/* Free the content of a field, recursively freeing also its next field.
+ * The actual pointer is not freed.
+ */
+extern void field_free (struct field *field);
+
 #endif /* INDEX_STRUCT_H */
-- 
1.8.3.1

Pino Toscano

2014-Mar-20 13:48 UTC

head link

[Libguestfs] [PATCH 3/3] builder: clean the parsing structs on error

---
 builder/index-parse.y | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builder/index-parse.y b/builder/index-parse.y
index 5554e53..9355bd4 100644
--- a/builder/index-parse.y
+++ b/builder/index-parse.y
@@ -90,6 +90,11 @@ typedef void *yyscan_t;
 %parse-param { yyscan_t scanner }
 %parse-param { struct parse_context *context }
 
+%destructor { section_free ($$); } <sections>
+%destructor { section_free ($$); } <section>
+%destructor { field_free ($$); } <fields>
+%destructor { field_free ($$); } <field>
+
 %%
 
 index:
-- 
1.8.3.1

Richard W.M. Jones

2014-Mar-20 14:15 UTC

head link

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

Richard W.M. Jones

2014-Mar-20 14:16 UTC

head link

Re: [Libguestfs] [PATCH 3/3] builder: clean the parsing structs on error

On Thu, Mar 20, 2014 at 02:48:13PM +0100, Pino Toscano
wrote:> ---
>  builder/index-parse.y | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/builder/index-parse.y b/builder/index-parse.y
> index 5554e53..9355bd4 100644
> --- a/builder/index-parse.y
> +++ b/builder/index-parse.y
> @@ -90,6 +90,11 @@ typedef void *yyscan_t;
>  %parse-param { yyscan_t scanner }
>  %parse-param { struct parse_context *context }
>  
> +%destructor { section_free ($$); } <sections>
> +%destructor { section_free ($$); } <section>
> +%destructor { field_free ($$); } <fields>
> +%destructor { field_free ($$); } <field>
ACK to 2/3 & 3/3.

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

Pino Toscano

2014-Mar-20 15:24 UTC

head link

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

Reasonably Related Threads

Search for more reasonably related threads

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

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

[Libguestfs] [PATCH 2/3] builder: rename and make public the section/field free functions

[Libguestfs] [PATCH 3/3] builder: clean the parsing structs on error

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

Re: [Libguestfs] [PATCH 3/3] builder: clean the parsing structs on error

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

Reasonably Related Threads