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".