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