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
[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
[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
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.1ACK. 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
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
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
Possibly Parallel Threads
- [PATCH 1/3] builder/virt-index-validate: try to cleanup in any occasion
- [PATCH 1/3] builder: make the C index parser reentrant
- [PATCH 1/2] builder: add an optional suffix string for INI parsing errors
- [PATCH v2 1/2] GCC 7: Add __attribute__((noreturn)) to some usage functions which call exit.
- [PATCH] builder: better handle some index parsing errors