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;
+}