Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/cli/report.cpp | 93 +++++++++++++++++++++++------------- src/include/report/event_config.h | 3 + src/lib/event_config.c | 64 +++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 34 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 05f0b11..fe7a791 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -396,7 +396,6 @@ static int run_report_editor(crash_data_t *crash_data) * @param result_size * Maximum byte count to be written. */ -#if 0 /* error: ‘void read_from_stdin(const char*, char*, int)’ defined but not used (-Werror) */ static void read_from_stdin(const char *question, char *result, int result_size) { assert(result_size > 1); @@ -407,7 +406,6 @@ static void read_from_stdin(const char *question, char *result, int result_size) // Remove the newline from the login. strchrnul(result, '\n')[0] = '\0'; } -#endif
/** * Asks a [y/n] question on stdin/stdout. @@ -429,7 +427,6 @@ static bool ask_yesno(const char *question) return 0 == strncmp(answer, yes, strlen(yes)); }
-#if 0 /* error: ‘bool set_echo(bool)’ defined but not used (-Werror) */ /* Returns true if echo has been changed from another state. */ static bool set_echo(bool enable) { @@ -447,48 +444,76 @@ static bool set_echo(bool enable)
return true; } -#endif
/** * Asks user for missing information */ -#if 0 /* TODO: npajkovs: FIX ME!!! */ static void ask_for_missing_settings(const char *event_name) { - event_config_t *config = get_event_config(event_name); - event_option_t *login, *passwd; - login = get_event_option_from_list("Bugzilla_Login", config->options); - passwd = get_event_option_from_list("Bugzilla_Password", config->options); - - int login_missing = (login && login->value && login->value[0] == '\0'); - int passwd_missing = (passwd && passwd->value && passwd->value[0] == '\0'); - if (!login_missing && !passwd_missing) + GHashTable *error_table = validate_event(event_name); + if (!error_table) return;
- // Read the missing information and push it to plugin settings. - printf(_("Wrong settings were detected for plugin %s\n"), event_name); - char result[64]; - if (login_missing) - { - free(login->value); - read_from_stdin(_("Enter your login: "), result, 64); - login->value = xstrdup(result); - } + event_config_t *event_config = get_event_config(event_name);
- if (passwd_missing) + GHashTableIter iter; + char *missing_value, *err_msg; + g_hash_table_iter_init(&iter, error_table); + while (g_hash_table_iter_next(&iter, (void**)&missing_value, (void**)&err_msg)) { - bool changed = set_echo(false); - free(passwd->value); - read_from_stdin(_("Enter your password: "), result, 64); - if (changed) - set_echo(true); - - passwd->value = xstrdup(result); - // Newline was not added by pressing Enter because ECHO was disabled, so add it now. - puts(""); + event_option_t *opt = get_event_option_from_list(missing_value, + event_config->options); + + char result[512]; + + char *question = xasprintf(_("Enter your %s: "), opt->label); + switch (opt->type) { + case OPTION_TYPE_TEXT: + case OPTION_TYPE_NUMBER: + read_from_stdin(question, result, 512); + opt->value = xstrdup(result); + break; + case OPTION_TYPE_PASSWORD: + { + bool changed = set_echo(false); + read_from_stdin(question, result, 512); + if (changed) + set_echo(true); + + opt->value = xstrdup(result); + /* Newline was not added by pressing Enter because ECHO was + disabled, so add it now. */ + puts(""); + break; + } + case OPTION_TYPE_BOOL: + free(question); + question = xasprintf("Using %s", opt->label); + if (ask_yesno(question)) + opt->value = xstrdup("yes"); + else + opt->value = xstrdup("no"); + + VERB3 log("bool = %s", opt->value); + break; + case OPTION_TYPE_INVALID: + break; + }; } + + g_hash_table_destroy(error_table); + + error_table = validate_event(event_name); + if (!error_table) + return; + + log(_("Your input is not valid, because of:")); + g_hash_table_iter_init(&iter, error_table); + while (g_hash_table_iter_next(&iter, (void**)&missing_value, (void**)&err_msg)) + log("%s: %s", missing_value, err_msg); + + g_hash_table_destroy(error_table); } -#endif
struct logging_state { char *last_line; @@ -698,7 +723,7 @@ int report(const char *dump_dir_name, int flags) errors++; continue; } - // ask_for_missing_settings(it->c_str()); + ask_for_missing_settings(it->c_str()); } }
diff --git a/src/include/report/event_config.h b/src/include/report/event_config.h index 12beefe..72b1d8c 100644 --- a/src/include/report/event_config.h +++ b/src/include/report/event_config.h @@ -89,6 +89,9 @@ extern GHashTable *g_event_config_list; // for iterating through entire list o GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
+GHashTable *validate_event(const char *event_name); +char *validate_event_option(event_option_t *opt); + #ifdef __cplusplus } #endif diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 3f76021..4502a83 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -280,3 +280,67 @@ void unexport_event_config(GList *env_list) free(var_val); } } + +GHashTable *validate_event(const char *event_name) +{ + event_config_t *config = get_event_config(event_name); + if (!config) + return NULL; + + + GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free); + + for (GList *li = config->options; li; li = li->next) + { + event_option_t *opt = (event_option_t *)li->data; + char *err = validate_event_option(opt); + if (err) + g_hash_table_insert(errors, xstrdup(opt->name), err); + } + + return errors; +} + +/* return NULL if successful otherwise appropriate error message */ +char *validate_event_option(event_option_t *opt) +{ + if (!opt->allow_empty && (!opt->value || !opt->value[0])) + return xasprintf(_("Missing mandatory %s value"), opt->name); + + /* if value is NULL and allow-empty yes than it doesn't make sence to check it */ + if (!opt->value) + return NULL; + + const gchar *s = NULL; + if (!g_utf8_validate(opt->value, -1, &s)) + return xasprintf(_("Invalid utf8 character '%c'"), *s); + + switch (opt->type) { + case OPTION_TYPE_TEXT: + case OPTION_TYPE_PASSWORD: + break; + case OPTION_TYPE_NUMBER: + { + long r = strtol(opt->value, (char **)&s, 10); + (void) r; + if (*s) + return xasprintf(_("Invalid number '%s'"), opt->value); + break; + } + case OPTION_TYPE_BOOL: + if (strcmp(opt->value, "yes") != 0 + && strcmp(opt->value, "no") != 0 + && strcmp(opt->value, "on") != 0 + && strcmp(opt->value, "off") != 0 + && strcmp(opt->value, "1") != 0 + && strcmp(opt->value, "0") != 0) + { + return xasprintf(_("Invalid boolean value '%s'"), opt->value); + } + break; + default: + return xstrdup(_("Unsupported option type")); + }; + + return NULL; +}
On Mon, 2011-03-21 at 18:02 +0100, Nikola Pajkovsky wrote:
- GHashTableIter iter;
- char *missing_value, *err_msg;
It's not missing_value, it's not a value at all. It's "the name of the option whose value is bad/missing". Maybe rename it to opt_name?
- g_hash_table_iter_init(&iter, error_table);
- while (g_hash_table_iter_next(&iter, (void**)&missing_value, (void**)&err_msg))
- {
event_option_t *opt = get_event_option_from_list(missing_value,
event_config->options);
char result[512];
char *question = xasprintf(_("Enter your %s: "), opt->label);
switch (opt->type) {
case OPTION_TYPE_TEXT:
case OPTION_TYPE_NUMBER:
read_from_stdin(question, result, 512);
opt->value = xstrdup(result);
break;
case OPTION_TYPE_PASSWORD:
{
bool changed = set_echo(false);
read_from_stdin(question, result, 512);
if (changed)
set_echo(true);
opt->value = xstrdup(result);
/* Newline was not added by pressing Enter because ECHO was
disabled, so add it now. */
puts("");
break;
}
case OPTION_TYPE_BOOL:
free(question);
question = xasprintf("Using %s", opt->label);
if (ask_yesno(question))
opt->value = xstrdup("yes");
else
opt->value = xstrdup("no");
VERB3 log("bool = %s", opt->value);
break;
case OPTION_TYPE_INVALID:
break;
}};
Handling of "question" variable above is confusing. For OPTION_TYPE_BOOL, you allocate, then free, then allocate it again. In all cases, you leak it.
I suspect that "Enter your %s: " and "Using %s" prompts will look confusing for some labels. Maybe use just simple "%s: " one? This would exactly match its usage in GUI.
In some cases, opt->label might be NULL. In this case, use opt->name.
- g_hash_table_destroy(error_table);
- error_table = validate_event(event_name);
- if (!error_table)
return;
- log(_("Your input is not valid, because of:"));
This is untrue. validate_event() may return non-NULL GHashTable which nevertheless is empty, and then the code above will say "Your input is not valid, because of:" but happily proceed without printing anything more.
I suggest changing validate_event() so that it never returns empty GHashTable (returns either non-empty one, or NULL).
- g_hash_table_iter_init(&iter, error_table);
- while (g_hash_table_iter_next(&iter, (void**)&missing_value, (void**)&err_msg))
log("%s: %s", missing_value, err_msg);
I suggest log("Bad value for '%s': %s", missing_value, err_msg);
- g_hash_table_destroy(error_table);
}
And we continue? Shouldn't we loop back and ask questions again?
--- a/src/include/report/event_config.h +++ b/src/include/report/event_config.h @@ -89,6 +89,9 @@ extern GHashTable *g_event_config_list; // for iterating through entire list o GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
+GHashTable *validate_event(const char *event_name); +char *validate_event_option(event_option_t *opt);
validate_event_option is only called from validate_event -> -> can be made static inside event_config.c
--- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -280,3 +280,67 @@ void unexport_event_config(GList *env_list) free(var_val); } }
+GHashTable *validate_event(const char *event_name) +{
- event_config_t *config = get_event_config(event_name);
- if (!config)
return NULL;
- GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free);
- for (GList *li = config->options; li; li = li->next)
- {
event_option_t *opt = (event_option_t *)li->data;
char *err = validate_event_option(opt);
if (err)
g_hash_table_insert(errors, xstrdup(opt->name), err);
- }
- return errors;
+}
+/* return NULL if successful otherwise appropriate error message */ +char *validate_event_option(event_option_t *opt) +{
- if (!opt->allow_empty && (!opt->value || !opt->value[0]))
return xasprintf(_("Missing mandatory %s value"), opt->name);
In this code, you should not add option name in the message, it may result in confusing error message later. For example: combined with log("%s: %s", missing_value, err_msg) above, this will result in:
<progname>: Append: Missing mandatory Append value
Looks somewhat unclear. I would do something like this:
<progname>: Bad value for 'Append': Missing mandatory value
Therefore, here I'd use return xstrdup(_("Missing mandatory value"));
- /* if value is NULL and allow-empty yes than it doesn't make sence to check it */
- if (!opt->value)
return NULL;
- const gchar *s = NULL;
- if (!g_utf8_validate(opt->value, -1, &s))
return xasprintf(_("Invalid utf8 character '%c'"), *s);
- switch (opt->type) {
- case OPTION_TYPE_TEXT:
- case OPTION_TYPE_PASSWORD:
break;
- case OPTION_TYPE_NUMBER:
- {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s)
return xasprintf(_("Invalid number '%s'"), opt->value);
You need to also check that errno == 0 - this prevents overflows (think "99999999999999999999999999999999999999999").
break;
- }
- case OPTION_TYPE_BOOL:
if (strcmp(opt->value, "yes") != 0
&& strcmp(opt->value, "no") != 0
&& strcmp(opt->value, "on") != 0
&& strcmp(opt->value, "off") != 0
&& strcmp(opt->value, "1") != 0
&& strcmp(opt->value, "0") != 0)
{
return xasprintf(_("Invalid boolean value '%s'"), opt->value);
}
break;
- default:
return xstrdup(_("Unsupported option type"));
- };
- return NULL;
+}
Denys> On Mon, 2011-03-21 at 18:02 +0100, Nikola Pajkovsky wrote: Denys> > + GHashTableIter iter; Denys> > + char *missing_value, *err_msg; Denys> Denys> It's not missing_value, it's not a value at all. Denys> It's "the name of the option whose value is bad/missing". Denys> Maybe rename it to opt_name?
Yeah, definitely better name
Denys> > + g_hash_table_iter_init(&iter, error_table); Denys> > + while (g_hash_table_iter_next(&iter, (void**)&missing_value, Denys> > (void**)&err_msg)) Denys> > + { Denys> > + event_option_t *opt = Denys> > get_event_option_from_list(missing_value, Denys> > + Denys> > event_config->options); + Denys> > + char result[512]; Denys> > + Denys> > + char *question = xasprintf(_("Enter your %s: "), opt->label); Denys> > + switch (opt->type) { Denys> > + case OPTION_TYPE_TEXT: Denys> > + case OPTION_TYPE_NUMBER: Denys> > + read_from_stdin(question, result, 512); Denys> > + opt->value = xstrdup(result); Denys> > + break; Denys> > + case OPTION_TYPE_PASSWORD: Denys> > + { Denys> > + bool changed = set_echo(false); Denys> > + read_from_stdin(question, result, 512); Denys> > + if (changed) Denys> > + set_echo(true); Denys> > + Denys> > + opt->value = xstrdup(result); Denys> > + /* Newline was not added by pressing Enter because ECHO Denys> > was Denys> > + disabled, so add it now. */ Denys> > + puts(""); Denys> > + break; Denys> > + } Denys> > + case OPTION_TYPE_BOOL: Denys> > + free(question); Denys> > + question = xasprintf("Using %s", opt->label); Denys> > + if (ask_yesno(question)) Denys> > + opt->value = xstrdup("yes"); Denys> > + else Denys> > + opt->value = xstrdup("no"); Denys> > + Denys> > + VERB3 log("bool = %s", opt->value); Denys> > + break; Denys> > + case OPTION_TYPE_INVALID: Denys> > + break; Denys> > + }; Denys> > } Denys> Denys> Handling of "question" variable above is confusing. Denys> For OPTION_TYPE_BOOL, you allocate, then free, then allocate it again. Denys> In all cases, you leak it. Denys> Denys> I suspect that "Enter your %s: " and "Using %s" prompts will look Denys> confusing for some labels. Maybe use just simple "%s: " one? Denys> This would exactly match its usage in GUI.
Yes, much much better
Denys> In some cases, opt->label might be NULL. In this case, use opt->name.
Fixed
Denys> > + g_hash_table_destroy(error_table); Denys> > + Denys> > + error_table = validate_event(event_name); Denys> > + if (!error_table) Denys> > + return; Denys> > + Denys> > + log(_("Your input is not valid, because of:")); Denys> Denys> This is untrue. validate_event() may return non-NULL Denys> GHashTable which nevertheless is empty, and then the code above will say Denys> "Your input is not valid, because of:" but happily proceed without Denys> printing anything more. Denys> Denys> I suggest changing validate_event() so that it never returns Denys> empty GHashTable (returns either non-empty one, or NULL).
Damn, I forgot write it :)
Denys> > + g_hash_table_iter_init(&iter, error_table); Denys> > + while (g_hash_table_iter_next(&iter, (void**)&missing_value, Denys> > (void**)&err_msg)) Denys> > + log("%s: %s", missing_value, err_msg); Denys> Denys> I suggest log("Bad value for '%s': %s", missing_value, err_msg);
Fixed
Denys> > + Denys> > + g_hash_table_destroy(error_table); Denys> > } Denys> Denys> And we continue? Shouldn't we loop back and ask questions again?
Yes we should, let's say we will looping for 3 times?
Denys> > --- a/src/include/report/event_config.h Denys> > +++ b/src/include/report/event_config.h Denys> > @@ -89,6 +89,9 @@ extern GHashTable *g_event_config_list; // for Denys> > iterating through entire list o GList *export_event_config(const char Denys> > *event_name); void unexport_event_config(GList *env_list); Denys> > Denys> > +GHashTable *validate_event(const char *event_name); Denys> > +char *validate_event_option(event_option_t *opt); Denys> Denys> validate_event_option is only called from validate_event -> Denys> -> can be made static inside event_config.c Denys> Denys> Denys> > --- a/src/lib/event_config.c Denys> > +++ b/src/lib/event_config.c Denys> > @@ -280,3 +280,67 @@ void unexport_event_config(GList *env_list) Denys> > free(var_val); Denys> > } Denys> > } Denys> > + Denys> > +GHashTable *validate_event(const char *event_name) Denys> > +{ Denys> > + event_config_t *config = get_event_config(event_name); Denys> > + if (!config) Denys> > + return NULL; Denys> > + Denys> > + Denys> > + GHashTable *errors = g_hash_table_new_full(g_str_hash, Denys> > g_str_equal, free, free); + Denys> > + for (GList *li = config->options; li; li = li->next) Denys> > + { Denys> > + event_option_t *opt = (event_option_t *)li->data; Denys> > + char *err = validate_event_option(opt); Denys> > + if (err) Denys> > + g_hash_table_insert(errors, xstrdup(opt->name), err); Denys> > + } Denys> > + Denys> > + return errors; Denys> > +} Denys> > + Denys> > +/* return NULL if successful otherwise appropriate error message */ Denys> > +char *validate_event_option(event_option_t *opt) Denys> > +{ Denys> > + if (!opt->allow_empty && (!opt->value || !opt->value[0])) Denys> > + return xasprintf(_("Missing mandatory %s value"), opt->name); Denys> Denys> In this code, you should not add option name in the message, it may Denys> result in confusing error message later. For example: Denys> combined with log("%s: %s", missing_value, err_msg) above, this will Denys> result in: Denys> Denys> <progname>: Append: Missing mandatory Append value Denys> Denys> Looks somewhat unclear. I would do something like this: Denys> Denys> <progname>: Bad value for 'Append': Missing mandatory value Denys> Denys> Therefore, here I'd use return xstrdup(_("Missing mandatory value"));
Fixed
Denys> > + /* if value is NULL and allow-empty yes than it doesn't make Denys> > sence to check it */ Denys> > + if (!opt->value) Denys> > + return NULL; Denys> > + Denys> > + const gchar *s = NULL; Denys> > + if (!g_utf8_validate(opt->value, -1, &s)) Denys> > + return xasprintf(_("Invalid utf8 character '%c'"), *s); Denys> > + Denys> > + switch (opt->type) { Denys> > + case OPTION_TYPE_TEXT: Denys> > + case OPTION_TYPE_PASSWORD: Denys> > + break; Denys> > + case OPTION_TYPE_NUMBER: Denys> > + { Denys> > + long r = strtol(opt->value, (char **)&s, 10); Denys> > + (void) r; Denys> > + if (*s) Denys> > + return xasprintf(_("Invalid number '%s'"), opt->value); Denys> Denys> You need to also check that errno == 0 - this prevents overflows Denys> (think "99999999999999999999999999999999999999999").
Fixed
Denys> > + break; Denys> > + } Denys> > + case OPTION_TYPE_BOOL: Denys> > + if (strcmp(opt->value, "yes") != 0 Denys> > + && strcmp(opt->value, "no") != 0 Denys> > + && strcmp(opt->value, "on") != 0 Denys> > + && strcmp(opt->value, "off") != 0 Denys> > + && strcmp(opt->value, "1") != 0 Denys> > + && strcmp(opt->value, "0") != 0) Denys> > + { Denys> > + return xasprintf(_("Invalid boolean value '%s'"), Denys> > opt->value); Denys> > + } Denys> > + break; Denys> > + default: Denys> > + return xstrdup(_("Unsupported option type")); Denys> > + }; Denys> > + Denys> > + return NULL; Denys> > +} Denys>
Thanks
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/cli/report.cpp | 102 ++++++++++++++++++++++++------------- src/include/report/event_config.h | 3 + src/lib/event_config.c | 69 +++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 36 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 05f0b11..a2eb15a 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -396,7 +396,6 @@ static int run_report_editor(crash_data_t *crash_data) * @param result_size * Maximum byte count to be written. */ -#if 0 /* error: ‘void read_from_stdin(const char*, char*, int)’ defined but not used (-Werror) */ static void read_from_stdin(const char *question, char *result, int result_size) { assert(result_size > 1); @@ -407,7 +406,6 @@ static void read_from_stdin(const char *question, char *result, int result_size) // Remove the newline from the login. strchrnul(result, '\n')[0] = '\0'; } -#endif
/** * Asks a [y/n] question on stdin/stdout. @@ -429,7 +427,6 @@ static bool ask_yesno(const char *question) return 0 == strncmp(answer, yes, strlen(yes)); }
-#if 0 /* error: ‘bool set_echo(bool)’ defined but not used (-Werror) */ /* Returns true if echo has been changed from another state. */ static bool set_echo(bool enable) { @@ -447,48 +444,81 @@ static bool set_echo(bool enable)
return true; } -#endif
/** * Asks user for missing information */ -#if 0 /* TODO: npajkovs: FIX ME!!! */ static void ask_for_missing_settings(const char *event_name) { - event_config_t *config = get_event_config(event_name); - event_option_t *login, *passwd; - login = get_event_option_from_list("Bugzilla_Login", config->options); - passwd = get_event_option_from_list("Bugzilla_Password", config->options); - - int login_missing = (login && login->value && login->value[0] == '\0'); - int passwd_missing = (passwd && passwd->value && passwd->value[0] == '\0'); - if (!login_missing && !passwd_missing) - return; - - // Read the missing information and push it to plugin settings. - printf(_("Wrong settings were detected for plugin %s\n"), event_name); - char result[64]; - if (login_missing) + for (int i = 0; i < 3; ++i) { - free(login->value); - read_from_stdin(_("Enter your login: "), result, 64); - login->value = xstrdup(result); - } + GHashTable *error_table = validate_event(event_name); + if (!error_table) + return;
- if (passwd_missing) - { - bool changed = set_echo(false); - free(passwd->value); - read_from_stdin(_("Enter your password: "), result, 64); - if (changed) - set_echo(true); - - passwd->value = xstrdup(result); - // Newline was not added by pressing Enter because ECHO was disabled, so add it now. - puts(""); + event_config_t *event_config = get_event_config(event_name); + + GHashTableIter iter; + char *opt_value, *err_msg; + g_hash_table_iter_init(&iter, error_table); + while (g_hash_table_iter_next(&iter, (void**)&opt_value, (void**)&err_msg)) + { + event_option_t *opt = get_event_option_from_list(opt_value, + event_config->options); + + char result[512]; + + char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name); + switch (opt->type) { + case OPTION_TYPE_TEXT: + case OPTION_TYPE_NUMBER: + read_from_stdin(question, result, 512); + opt->value = xstrdup(result); + break; + case OPTION_TYPE_PASSWORD: + { + bool changed = set_echo(false); + read_from_stdin(question, result, 512); + if (changed) + set_echo(true); + + opt->value = xstrdup(result); + /* Newline was not added by pressing Enter because ECHO was + disabled, so add it now. */ + puts(""); + break; + } + case OPTION_TYPE_BOOL: + if (ask_yesno(question)) + opt->value = xstrdup("yes"); + else + opt->value = xstrdup("no"); + + break; + case OPTION_TYPE_INVALID: + break; + }; + + free(question); + } + + g_hash_table_destroy(error_table); + + error_table = validate_event(event_name); + if (!error_table) + return; + + log(_("Your input is not valid, because of:")); + g_hash_table_iter_init(&iter, error_table); + while (g_hash_table_iter_next(&iter, (void**)&opt_value, (void**)&err_msg)) + log(_("Bad value for '%s': %s"), opt_value, err_msg); + + g_hash_table_destroy(error_table); } + + /* we ask for 3 times and still don't have valid infromation */ + error_msg_and_die("Invalid input, program exiting..."); } -#endif
struct logging_state { char *last_line; @@ -698,7 +728,7 @@ int report(const char *dump_dir_name, int flags) errors++; continue; } - // ask_for_missing_settings(it->c_str()); + ask_for_missing_settings(it->c_str()); } }
diff --git a/src/include/report/event_config.h b/src/include/report/event_config.h index 12beefe..72b1d8c 100644 --- a/src/include/report/event_config.h +++ b/src/include/report/event_config.h @@ -89,6 +89,9 @@ extern GHashTable *g_event_config_list; // for iterating through entire list o GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
+GHashTable *validate_event(const char *event_name); +char *validate_event_option(event_option_t *opt); + #ifdef __cplusplus } #endif diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 3f76021..4e14c79 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -280,3 +280,72 @@ void unexport_event_config(GList *env_list) free(var_val); } } + +GHashTable *validate_event(const char *event_name) +{ + event_config_t *config = get_event_config(event_name); + if (!config) + return NULL; + + + GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free); + + for (GList *li = config->options; li; li = li->next) + { + event_option_t *opt = (event_option_t *)li->data; + char *err = validate_event_option(opt); + if (err) + g_hash_table_insert(errors, xstrdup(opt->name), err); + } + + if (g_hash_table_size(errors)) + return errors; + + g_hash_table_destroy(errors); + + return NULL; +} + +/* return NULL if successful otherwise appropriate error message */ +char *validate_event_option(event_option_t *opt) +{ + if (!opt->allow_empty && (!opt->value || !opt->value[0])) + return xstrdup(_("Missing mandatory value")); + + /* if value is NULL and allow-empty yes than it doesn't make sence to check it */ + if (!opt->value) + return NULL; + + const gchar *s = NULL; + if (!g_utf8_validate(opt->value, -1, &s)) + return xasprintf(_("Invalid utf8 character '%c'"), *s); + + switch (opt->type) { + case OPTION_TYPE_TEXT: + case OPTION_TYPE_PASSWORD: + break; + case OPTION_TYPE_NUMBER: + { + long r = strtol(opt->value, (char **)&s, 10); + (void) r; + if (*s || errno) + return xasprintf(_("Invalid number '%s'"), opt->value); + break; + } + case OPTION_TYPE_BOOL: + if (strcmp(opt->value, "yes") != 0 + && strcmp(opt->value, "no") != 0 + && strcmp(opt->value, "on") != 0 + && strcmp(opt->value, "off") != 0 + && strcmp(opt->value, "1") != 0 + && strcmp(opt->value, "0") != 0) + { + return xasprintf(_("Invalid boolean value '%s'"), opt->value); + } + break; + default: + return xstrdup(_("Unsupported option type")); + }; + + return NULL; +}
Hi Nikola,
On Tue, 2011-03-22 at 14:15 +0100, Denys Vlasenko wrote:
On Mon, 2011-03-21 at 18:02 +0100, Nikola Pajkovsky wrote:
- GHashTableIter iter;
- char *missing_value, *err_msg;
It's not missing_value, it's not a value at all. It's "the name of the option whose value is bad/missing". Maybe rename it to opt_name?
- g_hash_table_iter_init(&iter, error_table);
- while (g_hash_table_iter_next(&iter, (void**)&missing_value, (void**)&err_msg))
- {
event_option_t *opt = get_event_option_from_list(missing_value,
event_config->options);
char result[512];
char *question = xasprintf(_("Enter your %s: "), opt->label);
switch (opt->type) {
case OPTION_TYPE_TEXT:
case OPTION_TYPE_NUMBER:
read_from_stdin(question, result, 512);
opt->value = xstrdup(result);
break;
case OPTION_TYPE_PASSWORD:
{
bool changed = set_echo(false);
read_from_stdin(question, result, 512);
if (changed)
set_echo(true);
opt->value = xstrdup(result);
/* Newline was not added by pressing Enter because ECHO was
disabled, so add it now. */
puts("");
break;
}
case OPTION_TYPE_BOOL:
free(question);
question = xasprintf("Using %s", opt->label);
if (ask_yesno(question))
opt->value = xstrdup("yes");
else
opt->value = xstrdup("no");
VERB3 log("bool = %s", opt->value);
break;
case OPTION_TYPE_INVALID:
break;
}};
Handling of "question" variable above is confusing. For OPTION_TYPE_BOOL, you allocate, then free, then allocate it again. In all cases, you leak it.
I suspect that "Enter your %s: " and "Using %s" prompts will look confusing for some labels. Maybe use just simple "%s: " one? This would exactly match its usage in GUI.
In some cases, opt->label might be NULL. In this case, use opt->name.
- g_hash_table_destroy(error_table);
- error_table = validate_event(event_name);
- if (!error_table)
return;
- log(_("Your input is not valid, because of:"));
This is untrue. validate_event() may return non-NULL GHashTable which nevertheless is empty, and then the code above will say "Your input is not valid, because of:" but happily proceed without printing anything more.
I suggest changing validate_event() so that it never returns empty GHashTable (returns either non-empty one, or NULL).
- g_hash_table_iter_init(&iter, error_table);
- while (g_hash_table_iter_next(&iter, (void**)&missing_value, (void**)&err_msg))
log("%s: %s", missing_value, err_msg);
I suggest log("Bad value for '%s': %s", missing_value, err_msg);
- g_hash_table_destroy(error_table);
}
And we continue? Shouldn't we loop back and ask questions again?
--- a/src/include/report/event_config.h +++ b/src/include/report/event_config.h @@ -89,6 +89,9 @@ extern GHashTable *g_event_config_list; // for iterating through entire list o GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
+GHashTable *validate_event(const char *event_name); +char *validate_event_option(event_option_t *opt);
validate_event_option is only called from validate_event -> -> can be made static inside event_config.c
--- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -280,3 +280,67 @@ void unexport_event_config(GList *env_list) free(var_val); } }
+GHashTable *validate_event(const char *event_name) +{
- event_config_t *config = get_event_config(event_name);
- if (!config)
return NULL;
- GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free);
- for (GList *li = config->options; li; li = li->next)
- {
event_option_t *opt = (event_option_t *)li->data;
char *err = validate_event_option(opt);
if (err)
g_hash_table_insert(errors, xstrdup(opt->name), err);
- }
- return errors;
+}
+/* return NULL if successful otherwise appropriate error message */ +char *validate_event_option(event_option_t *opt) +{
- if (!opt->allow_empty && (!opt->value || !opt->value[0]))
return xasprintf(_("Missing mandatory %s value"), opt->name);
In this code, you should not add option name in the message, it may result in confusing error message later. For example: combined with log("%s: %s", missing_value, err_msg) above, this will result in:
<progname>: Append: Missing mandatory Append value
Looks somewhat unclear. I would do something like this:
<progname>: Bad value for 'Append': Missing mandatory value
Therefore, here I'd use return xstrdup(_("Missing mandatory value"));
- /* if value is NULL and allow-empty yes than it doesn't make sence to check it */
- if (!opt->value)
return NULL;
- const gchar *s = NULL;
- if (!g_utf8_validate(opt->value, -1, &s))
return xasprintf(_("Invalid utf8 character '%c'"), *s);
- switch (opt->type) {
- case OPTION_TYPE_TEXT:
- case OPTION_TYPE_PASSWORD:
break;
- case OPTION_TYPE_NUMBER:
- {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s)
return xasprintf(_("Invalid number '%s'"), opt->value);
You need to also check that errno == 0 - this prevents overflows (think "99999999999999999999999999999999999999999").
break;
- }
- case OPTION_TYPE_BOOL:
if (strcmp(opt->value, "yes") != 0
&& strcmp(opt->value, "no") != 0
&& strcmp(opt->value, "on") != 0
&& strcmp(opt->value, "off") != 0
&& strcmp(opt->value, "1") != 0
&& strcmp(opt->value, "0") != 0)
{
return xasprintf(_("Invalid boolean value '%s'"), opt->value);
}
break;
- default:
return xstrdup(_("Unsupported option type"));
- };
- return NULL;
+}
Can you send your updated patch for this?
Hi Nikola,
I would like to further discuss this change.
After my comments quited below, you committed the change efc9f956c65da5b151f20371c00d76aad326dc7b without further discussion.
Your change addresses some of my comments. However, in my opinion it still has a few problems, which need to be addressed.
Please let me know what you think above the following:
On Mon, 2011-03-28 at 17:08 +0200, Denys Vlasenko wrote:
Hi Nikola,
On Tue, 2011-03-22 at 14:15 +0100, Denys Vlasenko wrote:
On Mon, 2011-03-21 at 18:02 +0100, Nikola Pajkovsky wrote:
- GHashTableIter iter;
- char *missing_value, *err_msg;
It's not missing_value, it's not a value at all. It's "the name of the option whose value is bad/missing". Maybe rename it to opt_name?
End result in current git:
+ GHashTable *error_table = validate_event(event_name); + if (!error_table) + return;
+ event_config_t *event_config = get_event_config(event_name); + + GHashTableIter iter; + char *opt_value, *err_msg; + g_hash_table_iter_init(&iter, error_table); + while (g_hash_table_iter_next(&iter, (void**)&opt_value, (void**)&err_msg)) + {
validate_event() returns an array of (option name, error msg) key/value pairs. In the fragment above, opt_value variable holds option name. Why is it named opt_*value* then?
+ case OPTION_TYPE_NUMBER: + read_from_stdin(question, result, 512); + opt->value = xstrdup(result); + break; + case OPTION_TYPE_PASSWORD: + { + bool changed = set_echo(false); + read_from_stdin(question, result, 512); + if (changed) + set_echo(true); + + opt->value = xstrdup(result); + /* Newline was not added by pressing Enter because ECHO was + disabled, so add it now. */ + puts(""); + break; + } + case OPTION_TYPE_BOOL: + if (ask_yesno(question)) + opt->value = xstrdup("yes"); + else + opt->value = xstrdup("no");
In this fragment, you save newly entered data into opt->value. The bug is, you forget to free(opt->value) before storing new data, leaking previously-allocated string.
GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
+GHashTable *validate_event(const char *event_name); +char *validate_event_option(event_option_t *opt);
validate_event_option is only called from validate_event -> -> can be made static inside event_config.c
The above is still true. Do you object to this proposal?
- case OPTION_TYPE_NUMBER:
- {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s)
return xasprintf(_("Invalid number '%s'"), opt->value);
You need to also check that errno == 0 - this prevents overflows (think "99999999999999999999999999999999999999999").
The code in current git is a bit wrong:
+ long r = strtol(opt->value, (char **)&s, 10); + (void) r; + if (*s || errno) + return xasprintf(_("Invalid number '%s'"), opt->value); + break;
(1) For reliable error detection, you need to clear errno before strtol. (2) strtol("") doesn't set errno. I guess we don't want to accept "" as a valid number, thus a check "s == opt->value" needs to be added to the if() above. (3) Instead of casting gchar**: "(char **)&s", it's better to declare and use properly typed variable, say, "char *end;" ... "&end".
Denys Vlasenko dvlasenk@redhat.com writes:
Hi Nikola,
I would like to further discuss this change.
I'd prefer it too, but nobody answers for a few days, so I merged it.
After my comments quited below, you committed the change efc9f956c65da5b151f20371c00d76aad326dc7b without further discussion.
Your change addresses some of my comments. However, in my opinion it still has a few problems, which need to be addressed.
Please let me know what you think above the following:
On Mon, 2011-03-28 at 17:08 +0200, Denys Vlasenko wrote:
Hi Nikola,
On Tue, 2011-03-22 at 14:15 +0100, Denys Vlasenko wrote:
On Mon, 2011-03-21 at 18:02 +0100, Nikola Pajkovsky wrote:
- GHashTableIter iter;
- char *missing_value, *err_msg;
It's not missing_value, it's not a value at all. It's "the name of the option whose value is bad/missing". Maybe rename it to opt_name?
End result in current git:
GHashTable *error_table = validate_event(event_name);
if (!error_table)
return;
event_config_t *event_config = get_event_config(event_name);
GHashTableIter iter;
char *opt_value, *err_msg;
g_hash_table_iter_init(&iter, error_table);
while (g_hash_table_iter_next(&iter, (void**)&opt_value, (void**)&err_msg))
{
validate_event() returns an array of (option name, error msg) key/value pairs. In the fragment above, opt_value variable holds option name. Why is it named opt_*value* then?
I was driven by devil, yes it has to be opt_name.
case OPTION_TYPE_NUMBER:
read_from_stdin(question, result, 512);
opt->value = xstrdup(result);
break;
case OPTION_TYPE_PASSWORD:
{
bool changed = set_echo(false);
read_from_stdin(question, result, 512);
if (changed)
set_echo(true);
opt->value = xstrdup(result);
/* Newline was not added by pressing Enter because ECHO was
disabled, so add it now. */
puts("");
break;
}
case OPTION_TYPE_BOOL:
if (ask_yesno(question))
opt->value = xstrdup("yes");
else
opt->value = xstrdup("no");
In this fragment, you save newly entered data into opt->value. The bug is, you forget to free(opt->value) before storing new data, leaking previously-allocated string.
Damned, I did it before, I have to more careful.
GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
+GHashTable *validate_event(const char *event_name); +char *validate_event_option(event_option_t *opt);
validate_event_option is only called from validate_event -> -> can be made static inside event_config.c
The above is still true. Do you object to this proposal?
Yes, why not. The idea was that everyone can validate a single option. Nevertheless nobody is using this right now, so lets hide it.
- case OPTION_TYPE_NUMBER:
- {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s)
return xasprintf(_("Invalid number '%s'"), opt->value);
You need to also check that errno == 0 - this prevents overflows (think "99999999999999999999999999999999999999999").
The code in current git is a bit wrong:
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s || errno)
return xasprintf(_("Invalid number '%s'"), opt->value);
break;
(1) For reliable error detection, you need to clear errno before strtol. (2) strtol("") doesn't set errno. I guess we don't want to accept "" as a valid number, thus a check "s == opt->value" needs to be added to the if() above. (3) Instead of casting gchar**: "(char **)&s", it's better to declare and use properly typed variable, say, "char *end;" ... "&end".
duh! :) I made some improvements
patchset will follow in a minute for review
Thank you denys
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/cli/report.cpp | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 2b964fc..953f2b3 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -466,6 +466,8 @@ static void ask_for_missing_settings(const char *event_name) event_option_t *opt = get_event_option_from_list(opt_name, event_config->options);
+ free(opt->value); + char result[512];
char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name);
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/cli/report.cpp | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 2b964fc..953f2b3 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -466,6 +466,8 @@ static void ask_for_missing_settings(const char *event_name) event_option_t *opt = get_event_option_from_list(opt_name, event_config->options);
free(opt->value);
char result[512]; char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name);
...followed by:
switch (opt->type) { case OPTION_TYPE_TEXT: case OPTION_TYPE_NUMBER: read_from_stdin(question, result, 512); opt->value = xstrdup(result); break; case OPTION_TYPE_PASSWORD: { bool changed = set_echo(false); read_from_stdin(question, result, 512); if (changed) set_echo(true);
opt->value = xstrdup(result); /* Newline was not added by pressing Enter because ECHO was disabled, so add it now. */ puts(""); break; } case OPTION_TYPE_BOOL: if (ask_yesno(question)) opt->value = xstrdup("yes"); else opt->value = xstrdup("no");
break; case OPTION_TYPE_INVALID: break; };
What will happen if we fall through switch() without assigning anything to opt->value?
(This might be currently impossible, because opt->type maybe always contains one of "allowed" values only, but for the sake of writing robust code, let's assume we can't be 100% sure it does).
Answer: opt->value will still point to the free'd memory. Bug.
Denys Vlasenko dvlasenk@redhat.com writes:
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/cli/report.cpp | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 2b964fc..953f2b3 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -466,6 +466,8 @@ static void ask_for_missing_settings(const char *event_name) event_option_t *opt = get_event_option_from_list(opt_name, event_config->options);
free(opt->value);
char result[512]; char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name);
...followed by:
switch (opt->type) { case OPTION_TYPE_TEXT: case OPTION_TYPE_NUMBER: read_from_stdin(question, result, 512); opt->value = xstrdup(result); break; case OPTION_TYPE_PASSWORD: { bool changed = set_echo(false); read_from_stdin(question, result, 512); if (changed) set_echo(true); opt->value = xstrdup(result); /* Newline was not added by pressing Enter because ECHO was disabled, so add it now. */ puts(""); break; } case OPTION_TYPE_BOOL: if (ask_yesno(question)) opt->value = xstrdup("yes"); else opt->value = xstrdup("no"); break; case OPTION_TYPE_INVALID: break; };
What will happen if we fall through switch() without assigning anything to opt->value?
(This might be currently impossible, because opt->type maybe always contains one of "allowed" values only, but for the sake of writing robust code, let's assume we can't be 100% sure it does).
Answer: opt->value will still point to the free'd memory. Bug.
I will set to NULL
On Wed, 2011-03-30 at 14:39 +0200, Nikola Pajkovsky wrote:
Denys Vlasenko dvlasenk@redhat.com writes:
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/cli/report.cpp | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 2b964fc..953f2b3 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -466,6 +466,8 @@ static void ask_for_missing_settings(const char *event_name) event_option_t *opt = get_event_option_from_list(opt_name, event_config->options);
free(opt->value);
char result[512]; char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name);
...followed by:
switch (opt->type) { case OPTION_TYPE_TEXT: case OPTION_TYPE_NUMBER: read_from_stdin(question, result, 512); opt->value = xstrdup(result); break; case OPTION_TYPE_PASSWORD: { bool changed = set_echo(false); read_from_stdin(question, result, 512); if (changed) set_echo(true); opt->value = xstrdup(result); /* Newline was not added by pressing Enter because ECHO was disabled, so add it now. */ puts(""); break; } case OPTION_TYPE_BOOL: if (ask_yesno(question)) opt->value = xstrdup("yes"); else opt->value = xstrdup("no"); break; case OPTION_TYPE_INVALID: break; };
What will happen if we fall through switch() without assigning anything to opt->value?
(This might be currently impossible, because opt->type maybe always contains one of "allowed" values only, but for the sake of writing robust code, let's assume we can't be 100% sure it does).
Answer: opt->value will still point to the free'd memory. Bug.
I will set to NULL
Like this?
free(opt->value); opt->value = NULL;
This is ok with me.
Denys Vlasenko dvlasenk@redhat.com writes:
On Wed, 2011-03-30 at 14:39 +0200, Nikola Pajkovsky wrote:
Denys Vlasenko dvlasenk@redhat.com writes:
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/cli/report.cpp | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/cli/report.cpp b/src/cli/report.cpp index 2b964fc..953f2b3 100644 --- a/src/cli/report.cpp +++ b/src/cli/report.cpp @@ -466,6 +466,8 @@ static void ask_for_missing_settings(const char *event_name) event_option_t *opt = get_event_option_from_list(opt_name, event_config->options);
free(opt->value);
char result[512]; char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name);
...followed by:
switch (opt->type) { case OPTION_TYPE_TEXT: case OPTION_TYPE_NUMBER: read_from_stdin(question, result, 512); opt->value = xstrdup(result); break; case OPTION_TYPE_PASSWORD: { bool changed = set_echo(false); read_from_stdin(question, result, 512); if (changed) set_echo(true); opt->value = xstrdup(result); /* Newline was not added by pressing Enter because ECHO was disabled, so add it now. */ puts(""); break; } case OPTION_TYPE_BOOL: if (ask_yesno(question)) opt->value = xstrdup("yes"); else opt->value = xstrdup("no"); break; case OPTION_TYPE_INVALID: break; };
What will happen if we fall through switch() without assigning anything to opt->value?
(This might be currently impossible, because opt->type maybe always contains one of "allowed" values only, but for the sake of writing robust code, let's assume we can't be 100% sure it does).
Answer: opt->value will still point to the free'd memory. Bug.
I will set to NULL
Like this?
free(opt->value); opt->value = NULL;
This is ok with me.
exactly like this
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/include/report/event_config.h | 1 - src/lib/event_config.c | 54 ++++++++++++++++++------------------ 2 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/src/include/report/event_config.h b/src/include/report/event_config.h index 72b1d8c..8dcfb9f 100644 --- a/src/include/report/event_config.h +++ b/src/include/report/event_config.h @@ -90,7 +90,6 @@ GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
GHashTable *validate_event(const char *event_name); -char *validate_event_option(event_option_t *opt);
#ifdef __cplusplus } diff --git a/src/lib/event_config.c b/src/lib/event_config.c index a84aae0..7a8b5e3 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -284,34 +284,8 @@ void unexport_event_config(GList *env_list) } }
-GHashTable *validate_event(const char *event_name) -{ - event_config_t *config = get_event_config(event_name); - if (!config) - return NULL; - - - GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free); - GList *li; - - for (li = config->options; li; li = li->next) - { - event_option_t *opt = (event_option_t *)li->data; - char *err = validate_event_option(opt); - if (err) - g_hash_table_insert(errors, xstrdup(opt->name), err); - } - - if (g_hash_table_size(errors)) - return errors; - - g_hash_table_destroy(errors); - - return NULL; -} - /* return NULL if successful otherwise appropriate error message */ -char *validate_event_option(event_option_t *opt) +static char *validate_event_option(event_option_t *opt) { if (!opt->allow_empty && (!opt->value || !opt->value[0])) return xstrdup(_("Missing mandatory value")); @@ -353,3 +327,29 @@ char *validate_event_option(event_option_t *opt)
return NULL; } + +GHashTable *validate_event(const char *event_name) +{ + event_config_t *config = get_event_config(event_name); + if (!config) + return NULL; + + + GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free); + GList *li; + + for (li = config->options; li; li = li->next) + { + event_option_t *opt = (event_option_t *)li->data; + char *err = validate_event_option(opt); + if (err) + g_hash_table_insert(errors, xstrdup(opt->name), err); + } + + if (g_hash_table_size(errors)) + return errors; + + g_hash_table_destroy(errors); + + return NULL; +}
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/include/report/event_config.h | 1 - src/lib/event_config.c | 54 ++++++++++++++++++------------------ 2 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/src/include/report/event_config.h b/src/include/report/event_config.h index 72b1d8c..8dcfb9f 100644 --- a/src/include/report/event_config.h +++ b/src/include/report/event_config.h @@ -90,7 +90,6 @@ GList *export_event_config(const char *event_name); void unexport_event_config(GList *env_list);
GHashTable *validate_event(const char *event_name); -char *validate_event_option(event_option_t *opt);
#ifdef __cplusplus } diff --git a/src/lib/event_config.c b/src/lib/event_config.c index a84aae0..7a8b5e3 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -284,34 +284,8 @@ void unexport_event_config(GList *env_list) } }
-GHashTable *validate_event(const char *event_name) -{
- event_config_t *config = get_event_config(event_name);
- if (!config)
return NULL;
- GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free);
- GList *li;
- for (li = config->options; li; li = li->next)
- {
event_option_t *opt = (event_option_t *)li->data;
char *err = validate_event_option(opt);
if (err)
g_hash_table_insert(errors, xstrdup(opt->name), err);
- }
- if (g_hash_table_size(errors))
return errors;
- g_hash_table_destroy(errors);
- return NULL;
-}
/* return NULL if successful otherwise appropriate error message */ -char *validate_event_option(event_option_t *opt) +static char *validate_event_option(event_option_t *opt) { if (!opt->allow_empty && (!opt->value || !opt->value[0])) return xstrdup(_("Missing mandatory value")); @@ -353,3 +327,29 @@ char *validate_event_option(event_option_t *opt)
return NULL;
}
+GHashTable *validate_event(const char *event_name) +{
- event_config_t *config = get_event_config(event_name);
- if (!config)
return NULL;
- GHashTable *errors = g_hash_table_new_full(g_str_hash, g_str_equal, free, free);
- GList *li;
- for (li = config->options; li; li = li->next)
- {
event_option_t *opt = (event_option_t *)li->data;
char *err = validate_event_option(opt);
if (err)
g_hash_table_insert(errors, xstrdup(opt->name), err);
- }
- if (g_hash_table_size(errors))
return errors;
- g_hash_table_destroy(errors);
- return NULL;
+}
Looks good to me.
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/lib/event_config.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..b855394 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,22 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: { - long r = strtol(opt->value, (char **)&s, 10); - (void) r; - if (*s || errno) + char *endptr; + errno = 0; + long r = strtol(opt->value, &endptr, 10); + if ((errno == ERANGE && (r == LONG_MAX || r == LONG_MIN)) + || (errno != 0 && r == 0)) + { + perror_msg("strtol"); + return xasprintf(_("Invalid number '%s'"), opt->value); + } + + if (endptr == str) + return xasprintf(_("No digits were found '%s'"), opt->value); + + if (*endptr) return xasprintf(_("Invalid number '%s'"), opt->value); + break; } case OPTION_TYPE_BOOL:
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/lib/event_config.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..b855394 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,22 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s || errno)
char *endptr;
errno = 0;
long r = strtol(opt->value, &endptr, 10);
if ((errno == ERANGE && (r == LONG_MAX || r == LONG_MIN))
|| (errno != 0 && r == 0))
{
perror_msg("strtol");
return xasprintf(_("Invalid number '%s'"), opt->value);
}
if (endptr == str)
return xasprintf(_("No digits were found '%s'"), opt->value);
if (*endptr) return xasprintf(_("Invalid number '%s'"), opt->value);
Wow. Now it does too much IMO... I propose something like this:
char *endptr; errno = 0; long r = strtol(opt->value, &endptr, 10); (void) r; if (errno != 0 || endptr == str || *endptr != '\0') { return xasprintf(_("Invalid number '%s'"), opt->value); }
Rationale: any errno != 0 indicates error, need just check that, no need to check for ERANGE. endptr == str checks for "" case. *endptr != '\0' checks for "123garbage" case.
On Wed, 2011-03-30 at 14:20 +0200, Denys Vlasenko wrote:
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/lib/event_config.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..b855394 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,22 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s || errno)
char *endptr;
errno = 0;
long r = strtol(opt->value, &endptr, 10);
if ((errno == ERANGE && (r == LONG_MAX || r == LONG_MIN))
|| (errno != 0 && r == 0))
{
perror_msg("strtol");
return xasprintf(_("Invalid number '%s'"), opt->value);
}
if (endptr == str)
return xasprintf(_("No digits were found '%s'"), opt->value);
if (*endptr) return xasprintf(_("Invalid number '%s'"), opt->value);
Wow. Now it does too much IMO... I propose something like this:
char *endptr; errno = 0; long r = strtol(opt->value, &endptr, 10); (void) r; if (errno != 0 || endptr == str || *endptr != '\0')
s/str/opt->value in the line above:
if (errno != 0 || endptr == opt->value || *endptr != '\0')
{ return xasprintf(_("Invalid number '%s'"), opt->value); }
Rationale: any errno != 0 indicates error, need just check that, no need to check for ERANGE. endptr == str checks for "" case. *endptr != '\0' checks for "123garbage" case.
Denys Vlasenko dvlasenk@redhat.com writes:
On Wed, 2011-03-30 at 14:20 +0200, Denys Vlasenko wrote:
On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote:
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/lib/event_config.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..b855394 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,22 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: {
long r = strtol(opt->value, (char **)&s, 10);
(void) r;
if (*s || errno)
char *endptr;
errno = 0;
long r = strtol(opt->value, &endptr, 10);
if ((errno == ERANGE && (r == LONG_MAX || r == LONG_MIN))
|| (errno != 0 && r == 0))
{
perror_msg("strtol");
return xasprintf(_("Invalid number '%s'"), opt->value);
}
if (endptr == str)
return xasprintf(_("No digits were found '%s'"), opt->value);
if (*endptr) return xasprintf(_("Invalid number '%s'"), opt->value);
Wow. Now it does too much IMO... I propose something like this:
:)
char *endptr; errno = 0; long r = strtol(opt->value, &endptr, 10); (void) r; if (errno != 0 || endptr == str || *endptr != '\0')
s/str/opt->value in the line above: if (errno != 0 || endptr == opt->value || *endptr != '\0')
ok :)
{ return xasprintf(_("Invalid number '%s'"), opt->value); }
Rationale: any errno != 0 indicates error, need just check that, no need to check for ERANGE. endptr == str checks for "" case. *endptr != '\0' checks for "123garbage" case.
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/lib/event_config.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..b855394 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,22 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: { - long r = strtol(opt->value, (char **)&s, 10); - (void) r; - if (*s || errno) + char *endptr; + errno = 0; + long r = strtol(opt->value, &endptr, 10); + if ((errno == ERANGE && (r == LONG_MAX || r == LONG_MIN)) + || (errno != 0 && r == 0)) + { + perror_msg("strtol"); + return xasprintf(_("Invalid number '%s'"), opt->value); + } + + if (endptr == str) + return xasprintf(_("No digits were found '%s'"), opt->value); + + if (*endptr) return xasprintf(_("Invalid number '%s'"), opt->value); + break; } case OPTION_TYPE_BOOL:
I sent wrong path before :-/
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com --- src/lib/event_config.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..6b8a288 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,13 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: { - long r = strtol(opt->value, (char **)&s, 10); + char *endptr; + errno = 0; + long r = strtol(opt->value, &endptr, 10); (void) r; - if (*s || errno) + if (errno != 0 || endptr == opt->value || *endptr != '\0') return xasprintf(_("Invalid number '%s'"), opt->value); + break; } case OPTION_TYPE_BOOL:
On Wed, 2011-03-30 at 14:43 +0200, Nikola Pajkovsky wrote:
I sent wrong path before :-/
Signed-off-by: Nikola Pajkovsky npajkovs@redhat.com
src/lib/event_config.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/lib/event_config.c b/src/lib/event_config.c index 7a8b5e3..6b8a288 100644 --- a/src/lib/event_config.c +++ b/src/lib/event_config.c @@ -304,10 +304,13 @@ static char *validate_event_option(event_option_t *opt) break; case OPTION_TYPE_NUMBER: {
long r = strtol(opt->value, (char **)&s, 10);
char *endptr;
errno = 0;
long r = strtol(opt->value, &endptr, 10); (void) r;
if (*s || errno)
if (errno != 0 || endptr == opt->value || *endptr != '\0') return xasprintf(_("Invalid number '%s'"), opt->value);
} case OPTION_TYPE_BOOL:break;
Looks good to me, please apply.
crash-catcher@lists.fedorahosted.org