8:15 a.m.
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;
+}
--
vda