On Mon, 2011-03-21 at 18:02 +0100, Nikola Pajkovsky wrote: 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? 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. 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). I suggest log("Bad value for '%s': %s", missing_value, err_msg); And we continue? Shouldn't we loop back and ask questions again? validate_event_option is only called from validate_event -> -> can be made static inside event_config.c 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")); You need to also check that errno == 0 - this prevents overflows (think "99999999999999999999999999999999999999999"). -- vda

Signed-off-by: Nikola Pajkovsky <npajkovs(a)redhat.com&gt; --- 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; +} -- 1.7.1

Signed-off-by: Nikola Pajkovsky <npajkovs(a)redhat.com&gt; --- 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); -- 1.7.1

Denys Vlasenko <dvlasenk(a)redhat.com&gt; writes: I will set to NULL -- Nikola

Denys Vlasenko <dvlasenk(a)redhat.com&gt; writes: exactly like this -- Nikola

On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote: Looks good to me. -- vda

On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote: 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. -- vda

Denys Vlasenko <dvlasenk(a)redhat.com&gt; writes: :) ok :) -- Nikola

I sent wrong path before :-/ Signed-off-by: Nikola Pajkovsky <npajkovs(a)redhat.com&gt; --- 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: -- 1.7.1