| Message ID | 1633550663-25571-9-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | improving config parsing warnings | expand |
On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > Add error reporting to the remaining config handlers. If the value is > invalid, do not change the existing config option's value. Like for the previous patch, I'm unsure if this is wise. You rely on a reasonable default being set before the function is called. I suppose that's the case, but I like seeing the "invalid" value substituted right there where the validity is checked. That saves us from searching the code for the default value. Maybe I overlooked an important rationale for not touching the values in the case of invalid input, please explain. Cheers, Martin > Also print > an error whenever 0 is returned for an invalid value. When the > handler > returns 1, config processing already fails with an error message. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/dict.c | 73 +++++++++++++++++++++++++++++++------------ > -- > 1 file changed, 51 insertions(+), 22 deletions(-) > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > index e79fcdd7..8c3b5f72 100644 > --- a/libmultipath/dict.c > +++ b/libmultipath/dict.c > @@ -199,8 +199,11 @@ set_yes_no(vector strvec, void *ptr, const char > *file, int line_nr) > > if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) > *int_ptr = YN_YES; > - else > + else if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0) > *int_ptr = YN_NO; > + else > + condlog(1, "%s line %d, invalid value for %s: > \"%s\"", > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > buff); > > FREE(buff); > return 0; > @@ -221,7 +224,8 @@ set_yes_no_undef(vector strvec, void *ptr, const > char *file, int line_nr) > else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) > *int_ptr = YNU_YES; > else > - *int_ptr = YNU_UNDEF; > + condlog(1, "%s line %d, invalid value for %s: > \"%s\"", > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > buff); > > FREE(buff); > return 0; > @@ -471,9 +475,6 @@ def_find_multipaths_handler(struct config *conf, > vector strvec, > char *buff; > int i; > > - if (set_yes_no_undef(strvec, &conf->find_multipaths, file, > line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF) > - return 0; > - > buff = set_value(strvec); > if (!buff) > return 1; > @@ -486,9 +487,14 @@ def_find_multipaths_handler(struct config *conf, > vector strvec, > } > } > > - if (conf->find_multipaths == YNU_UNDEF) { > - condlog(0, "illegal value for find_multipaths: %s", > buff); > - conf->find_multipaths = DEFAULT_FIND_MULTIPATHS; > + if (i >= __FIND_MULTIPATHS_LAST) { > + if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == > 0) > + conf->find_multipaths = FIND_MULTIPATHS_OFF; > + else if (strcmp(buff, "yes") == 0 || strcmp(buff, > "1") == 0) > + conf->find_multipaths = FIND_MULTIPATHS_ON; > + else > + condlog(1, "%s line %d, invalid value for > find_multipaths: \"%s\"", > + file, line_nr, buff); > } > > FREE(buff); > @@ -537,8 +543,10 @@ static int uid_attrs_handler(struct config > *conf, vector strvec, > if (!val) > return 1; > if (parse_uid_attrs(val, conf)) > - condlog(1, "error parsing uid_attrs: \"%s\"", val); > - condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf- > >uid_attrs)); > + condlog(1, "%s line %d,error parsing uid_attrs: > \"%s\"", file, > + line_nr, val); > + else > + condlog(4, "parsed %d uid_attrs", VECTOR_SIZE(&conf- > >uid_attrs)); > FREE(val); > return 0; > } > @@ -766,8 +774,11 @@ def_config_dir_handler(struct config *conf, > vector strvec, const char *file, > int line_nr) > { > /* this is only valid in the main config file */ > - if (conf->processed_main_config) > + if (conf->processed_main_config) { > + condlog(1, "%s line %d, config_dir option only valid > in /etc/multipath.conf", > + file, line_nr); > return 0; > + } > return set_path(strvec, &conf->config_dir, file, line_nr); > } > declare_def_snprint(config_dir, print_str) > @@ -825,7 +836,9 @@ set_mode(vector strvec, void *ptr, int *flags, > const char *file, int line_nr) > if (sscanf(buff, "%o", &mode) == 1 && mode <= 0777) { > *flags |= (1 << ATTR_MODE); > *mode_ptr = mode; > - } > + } else > + condlog(1, "%s line %d, invalid value for mode: > \"%s\"", > + file, line_nr, buff); > > FREE(buff); > return 0; > @@ -850,7 +863,9 @@ set_uid(vector strvec, void *ptr, int *flags, > const char *file, int line_nr) > else if (sscanf(buff, "%u", &uid) == 1){ > *flags |= (1 << ATTR_UID); > *uid_ptr = uid; > - } > + } else > + condlog(1, "%s line %d, invalid value for uid: > \"%s\"", > + file, line_nr, buff); > > FREE(buff); > return 0; > @@ -876,7 +891,9 @@ set_gid(vector strvec, void *ptr, int *flags, > const char *file, int line_nr) > else if (sscanf(buff, "%u", &gid) == 1){ > *flags |= (1 << ATTR_GID); > *gid_ptr = gid; > - } > + } else > + condlog(1, "%s line %d, invalid value for gid: > \"%s\"", > + file, line_nr, buff); > FREE(buff); > return 0; > } > @@ -978,7 +995,8 @@ set_dev_loss(vector strvec, void *ptr, const char > *file, int line_nr) > if (!strcmp(buff, "infinity")) > *uint_ptr = MAX_DEV_LOSS_TMO; > else if (sscanf(buff, "%u", uint_ptr) != 1) > - *uint_ptr = DEV_LOSS_TMO_UNSET; > + condlog(1, "%s line %d, invalid value for > dev_loss_tmo: \"%s\"", > + file, line_nr, buff); > > FREE(buff); > return 0; > @@ -1012,13 +1030,19 @@ static int > set_pgpolicy(vector strvec, void *ptr, const char *file, int > line_nr) > { > char * buff; > + int policy; > int *int_ptr = (int *)ptr; > > buff = set_value(strvec); > if (!buff) > return 1; > > - *int_ptr = get_pgpolicy_id(buff); > + policy = get_pgpolicy_id(buff); > + if (policy != IOPOLICY_UNDEF) > + *int_ptr = policy; > + else > + condlog(1, "%s line %d, invalid value for > path_grouping_policy: \"%s\"", > + file, line_nr, buff); > FREE(buff); > > return 0; > @@ -1131,10 +1155,11 @@ set_rr_weight(vector strvec, void *ptr, const > char *file, int line_nr) > > if (!strcmp(buff, "priorities")) > *int_ptr = RR_WEIGHT_PRIO; > - > - if (!strcmp(buff, "uniform")) > + else if (!strcmp(buff, "uniform")) > *int_ptr = RR_WEIGHT_NONE; > - > + else > + condlog(1, "%s line %d, invalid value for rr_weight: > \"%s\"", > + file, line_nr, buff); > FREE(buff); > > return 0; > @@ -1270,10 +1295,13 @@ def_log_checker_err_handler(struct config > *conf, vector strvec, > if (!buff) > return 1; > > - if (strlen(buff) == 4 && !strcmp(buff, "once")) > + if (!strcmp(buff, "once")) > conf->log_checker_err = LOG_CHKR_ERR_ONCE; > - else if (strlen(buff) == 6 && !strcmp(buff, "always")) > + else if (!strcmp(buff, "always")) > conf->log_checker_err = LOG_CHKR_ERR_ALWAYS; > + else > + condlog(1, "%s line %d, invalid value for > log_checker_err: \"%s\"", > + file, line_nr, buff); > We lack a proper DEFAULT for log_checker_err. > free(buff); > return 0; > @@ -1534,7 +1562,8 @@ hw_vpd_vendor_handler(struct config *conf, > vector strvec, const char *file, > goto out; > } > } > - hwe->vpd_vendor_id = 0; > + condlog(1, "%s line %d, invalid value for vpd_vendor: > \"%s\"", > + file, line_nr, buff); > out: > FREE(buff); > return 0; -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Nov 04, 2021 at 09:11:34PM +0000, Martin Wilck wrote: > On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > > Add error reporting to the remaining config handlers. If the value is > > invalid, do not change the existing config option's value. > > Like for the previous patch, I'm unsure if this is wise. You rely on a > reasonable default being set before the function is called. I suppose > that's the case, but I like seeing the "invalid" value substituted > right there where the validity is checked. That saves us from searching > the code for the default value. > > Maybe I overlooked an important rationale for not touching the values > in the case of invalid input, please explain. Since these handlers are only called if people put the corresponding option in the config files, we had better have sensible defaults if they're not called (or if they don't set anything). I admit that I should take a look for cases were we cap an out-of-range value, to see if it would make more sense to treat it as an invalid value instead. Also, instead of accepting strings that are simply a number, we should convert the string, and the check the actual number. But I don't see any harm in simply ignoring the invalid values. It's no different than if the user didn't put the invalid line into multipath.conf Not setting the values on garbage input makes the handlers more general. If you have two options that work the same except that they have different defaults, then by not explicitly setting the value to the default when you have invalid input, one handler can be used for both options. set_yes_no() is a good example. Without my patch, it always set the value to something, even if the input was garbage. But the default value it set was "no". That had nothing to do with the default value of the options that were using it. You could do extra work to make sure that it would correctly use the option's default value, but you get the same outcome, with simpler code, just by not changing the default if you have a garbage value. Also, many of the handlers never set the value on invalid input. I'm just making that consistent across all of the handlers. -Ben > Cheers, > Martin > > > Also print > > an error whenever 0 is returned for an invalid value. When the > > handler > > returns 1, config processing already fails with an error message. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/dict.c | 73 +++++++++++++++++++++++++++++++------------ > > -- > > 1 file changed, 51 insertions(+), 22 deletions(-) > > > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > > index e79fcdd7..8c3b5f72 100644 > > --- a/libmultipath/dict.c > > +++ b/libmultipath/dict.c > > @@ -199,8 +199,11 @@ set_yes_no(vector strvec, void *ptr, const char > > *file, int line_nr) > > > > if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) > > *int_ptr = YN_YES; > > - else > > + else if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0) > > *int_ptr = YN_NO; > > + else > > + condlog(1, "%s line %d, invalid value for %s: > > \"%s\"", > > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > > buff); > > > > FREE(buff); > > return 0; > > @@ -221,7 +224,8 @@ set_yes_no_undef(vector strvec, void *ptr, const > > char *file, int line_nr) > > else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) > > *int_ptr = YNU_YES; > > else > > - *int_ptr = YNU_UNDEF; > > + condlog(1, "%s line %d, invalid value for %s: > > \"%s\"", > > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0), > > buff); > > > > FREE(buff); > > return 0; > > @@ -471,9 +475,6 @@ def_find_multipaths_handler(struct config *conf, > > vector strvec, > > char *buff; > > int i; > > > > - if (set_yes_no_undef(strvec, &conf->find_multipaths, file, > > line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF) > > - return 0; > > - > > buff = set_value(strvec); > > if (!buff) > > return 1; > > @@ -486,9 +487,14 @@ def_find_multipaths_handler(struct config *conf, > > vector strvec, > > } > > } > > > > - if (conf->find_multipaths == YNU_UNDEF) { > > - condlog(0, "illegal value for find_multipaths: %s", > > buff); > > - conf->find_multipaths = DEFAULT_FIND_MULTIPATHS; > > + if (i >= __FIND_MULTIPATHS_LAST) { > > + if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == > > 0) > > + conf->find_multipaths = FIND_MULTIPATHS_OFF; > > + else if (strcmp(buff, "yes") == 0 || strcmp(buff, > > "1") == 0) > > + conf->find_multipaths = FIND_MULTIPATHS_ON; > > + else > > + condlog(1, "%s line %d, invalid value for > > find_multipaths: \"%s\"", > > + file, line_nr, buff); > > } > > > > FREE(buff); > > @@ -537,8 +543,10 @@ static int uid_attrs_handler(struct config > > *conf, vector strvec, > > if (!val) > > return 1; > > if (parse_uid_attrs(val, conf)) > > - condlog(1, "error parsing uid_attrs: \"%s\"", val); > > - condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf- > > >uid_attrs)); > > + condlog(1, "%s line %d,error parsing uid_attrs: > > \"%s\"", file, > > + line_nr, val); > > + else > > + condlog(4, "parsed %d uid_attrs", VECTOR_SIZE(&conf- > > >uid_attrs)); > > FREE(val); > > return 0; > > } > > @@ -766,8 +774,11 @@ def_config_dir_handler(struct config *conf, > > vector strvec, const char *file, > > int line_nr) > > { > > /* this is only valid in the main config file */ > > - if (conf->processed_main_config) > > + if (conf->processed_main_config) { > > + condlog(1, "%s line %d, config_dir option only valid > > in /etc/multipath.conf", > > + file, line_nr); > > return 0; > > + } > > return set_path(strvec, &conf->config_dir, file, line_nr); > > } > > declare_def_snprint(config_dir, print_str) > > @@ -825,7 +836,9 @@ set_mode(vector strvec, void *ptr, int *flags, > > const char *file, int line_nr) > > if (sscanf(buff, "%o", &mode) == 1 && mode <= 0777) { > > *flags |= (1 << ATTR_MODE); > > *mode_ptr = mode; > > - } > > + } else > > + condlog(1, "%s line %d, invalid value for mode: > > \"%s\"", > > + file, line_nr, buff); > > > > FREE(buff); > > return 0; > > @@ -850,7 +863,9 @@ set_uid(vector strvec, void *ptr, int *flags, > > const char *file, int line_nr) > > else if (sscanf(buff, "%u", &uid) == 1){ > > *flags |= (1 << ATTR_UID); > > *uid_ptr = uid; > > - } > > + } else > > + condlog(1, "%s line %d, invalid value for uid: > > \"%s\"", > > + file, line_nr, buff); > > > > FREE(buff); > > return 0; > > @@ -876,7 +891,9 @@ set_gid(vector strvec, void *ptr, int *flags, > > const char *file, int line_nr) > > else if (sscanf(buff, "%u", &gid) == 1){ > > *flags |= (1 << ATTR_GID); > > *gid_ptr = gid; > > - } > > + } else > > + condlog(1, "%s line %d, invalid value for gid: > > \"%s\"", > > + file, line_nr, buff); > > FREE(buff); > > return 0; > > } > > @@ -978,7 +995,8 @@ set_dev_loss(vector strvec, void *ptr, const char > > *file, int line_nr) > > if (!strcmp(buff, "infinity")) > > *uint_ptr = MAX_DEV_LOSS_TMO; > > else if (sscanf(buff, "%u", uint_ptr) != 1) > > - *uint_ptr = DEV_LOSS_TMO_UNSET; > > + condlog(1, "%s line %d, invalid value for > > dev_loss_tmo: \"%s\"", > > + file, line_nr, buff); > > > > FREE(buff); > > return 0; > > @@ -1012,13 +1030,19 @@ static int > > set_pgpolicy(vector strvec, void *ptr, const char *file, int > > line_nr) > > { > > char * buff; > > + int policy; > > int *int_ptr = (int *)ptr; > > > > buff = set_value(strvec); > > if (!buff) > > return 1; > > > > - *int_ptr = get_pgpolicy_id(buff); > > + policy = get_pgpolicy_id(buff); > > + if (policy != IOPOLICY_UNDEF) > > + *int_ptr = policy; > > + else > > + condlog(1, "%s line %d, invalid value for > > path_grouping_policy: \"%s\"", > > + file, line_nr, buff); > > FREE(buff); > > > > return 0; > > @@ -1131,10 +1155,11 @@ set_rr_weight(vector strvec, void *ptr, const > > char *file, int line_nr) > > > > if (!strcmp(buff, "priorities")) > > *int_ptr = RR_WEIGHT_PRIO; > > - > > - if (!strcmp(buff, "uniform")) > > + else if (!strcmp(buff, "uniform")) > > *int_ptr = RR_WEIGHT_NONE; > > - > > + else > > + condlog(1, "%s line %d, invalid value for rr_weight: > > \"%s\"", > > + file, line_nr, buff); > > FREE(buff); > > > > return 0; > > @@ -1270,10 +1295,13 @@ def_log_checker_err_handler(struct config > > *conf, vector strvec, > > if (!buff) > > return 1; > > > > - if (strlen(buff) == 4 && !strcmp(buff, "once")) > > + if (!strcmp(buff, "once")) > > conf->log_checker_err = LOG_CHKR_ERR_ONCE; > > - else if (strlen(buff) == 6 && !strcmp(buff, "always")) > > + else if (!strcmp(buff, "always")) > > conf->log_checker_err = LOG_CHKR_ERR_ALWAYS; > > + else > > + condlog(1, "%s line %d, invalid value for > > log_checker_err: \"%s\"", > > + file, line_nr, buff); > > > > We lack a proper DEFAULT for log_checker_err. > True. Is it really the only one? I thought that there were a number of options for which we just relied on the fact that conf is zeroed out when it's created, so the 0 value (in this case LOG_CHKR_ERR_ALWAYS) is the implicit default. > > free(buff); > > return 0; > > @@ -1534,7 +1562,8 @@ hw_vpd_vendor_handler(struct config *conf, > > vector strvec, const char *file, > > goto out; > > } > > } > > - hwe->vpd_vendor_id = 0; > > + condlog(1, "%s line %d, invalid value for vpd_vendor: > > \"%s\"", > > + file, line_nr, buff); > > out: > > FREE(buff); > > return 0; -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, 2021-11-05 at 15:10 -0500, Benjamin Marzinski wrote: > On Thu, Nov 04, 2021 at 09:11:34PM +0000, Martin Wilck wrote: > > On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote: > > > Add error reporting to the remaining config handlers. If the > > > value is > > > invalid, do not change the existing config option's value. > > > > Like for the previous patch, I'm unsure if this is wise. You rely > > on a > > reasonable default being set before the function is called. I > > suppose > > that's the case, but I like seeing the "invalid" value substituted > > right there where the validity is checked. That saves us from > > searching > > the code for the default value. > > > > Maybe I overlooked an important rationale for not touching the > > values > > in the case of invalid input, please explain. > > Since these handlers are only called if people put the corresponding > option in the config files, we had better have sensible defaults if > they're not called (or if they don't set anything). > > I admit that I should take a look for cases were we cap an out-of- > range > value, to see if it would make more sense to treat it as an invalid > value instead. Also, instead of accepting strings that are simply a > number, we should convert the string, and the check the actual > number. > But I don't see any harm in simply ignoring the invalid values. It's > no > different than if the user didn't put the invalid line into > multipath.conf > > Not setting the values on garbage input makes the handlers more > general. > If you have two options that work the same except that they have > different defaults, then by not explicitly setting the value to the > default when you have invalid input, one handler can be used for both > options. set_yes_no() is a good example. Without my patch, it always > set the value to something, even if the input was garbage. But the > default value it set was "no". That had nothing to do with the > default > value of the options that were using it. You could do extra work to > make > sure that it would correctly use the option's default value, but you > get > the same outcome, with simpler code, just by not changing the default > if > you have a garbage value. > > Also, many of the handlers never set the value on invalid input. I'm > just > making that consistent across all of the handlers. OK, you've convinced me. Thanks Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/dict.c b/libmultipath/dict.c index e79fcdd7..8c3b5f72 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -199,8 +199,11 @@ set_yes_no(vector strvec, void *ptr, const char *file, int line_nr) if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) *int_ptr = YN_YES; - else + else if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0) *int_ptr = YN_NO; + else + condlog(1, "%s line %d, invalid value for %s: \"%s\"", + file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff); FREE(buff); return 0; @@ -221,7 +224,8 @@ set_yes_no_undef(vector strvec, void *ptr, const char *file, int line_nr) else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) *int_ptr = YNU_YES; else - *int_ptr = YNU_UNDEF; + condlog(1, "%s line %d, invalid value for %s: \"%s\"", + file, line_nr, (char*)VECTOR_SLOT(strvec, 0), buff); FREE(buff); return 0; @@ -471,9 +475,6 @@ def_find_multipaths_handler(struct config *conf, vector strvec, char *buff; int i; - if (set_yes_no_undef(strvec, &conf->find_multipaths, file, line_nr) == 0 && conf->find_multipaths != FIND_MULTIPATHS_UNDEF) - return 0; - buff = set_value(strvec); if (!buff) return 1; @@ -486,9 +487,14 @@ def_find_multipaths_handler(struct config *conf, vector strvec, } } - if (conf->find_multipaths == YNU_UNDEF) { - condlog(0, "illegal value for find_multipaths: %s", buff); - conf->find_multipaths = DEFAULT_FIND_MULTIPATHS; + if (i >= __FIND_MULTIPATHS_LAST) { + if (strcmp(buff, "no") == 0 || strcmp(buff, "0") == 0) + conf->find_multipaths = FIND_MULTIPATHS_OFF; + else if (strcmp(buff, "yes") == 0 || strcmp(buff, "1") == 0) + conf->find_multipaths = FIND_MULTIPATHS_ON; + else + condlog(1, "%s line %d, invalid value for find_multipaths: \"%s\"", + file, line_nr, buff); } FREE(buff); @@ -537,8 +543,10 @@ static int uid_attrs_handler(struct config *conf, vector strvec, if (!val) return 1; if (parse_uid_attrs(val, conf)) - condlog(1, "error parsing uid_attrs: \"%s\"", val); - condlog(3, "parsed %d uid_attrs", VECTOR_SIZE(&conf->uid_attrs)); + condlog(1, "%s line %d,error parsing uid_attrs: \"%s\"", file, + line_nr, val); + else + condlog(4, "parsed %d uid_attrs", VECTOR_SIZE(&conf->uid_attrs)); FREE(val); return 0; } @@ -766,8 +774,11 @@ def_config_dir_handler(struct config *conf, vector strvec, const char *file, int line_nr) { /* this is only valid in the main config file */ - if (conf->processed_main_config) + if (conf->processed_main_config) { + condlog(1, "%s line %d, config_dir option only valid in /etc/multipath.conf", + file, line_nr); return 0; + } return set_path(strvec, &conf->config_dir, file, line_nr); } declare_def_snprint(config_dir, print_str) @@ -825,7 +836,9 @@ set_mode(vector strvec, void *ptr, int *flags, const char *file, int line_nr) if (sscanf(buff, "%o", &mode) == 1 && mode <= 0777) { *flags |= (1 << ATTR_MODE); *mode_ptr = mode; - } + } else + condlog(1, "%s line %d, invalid value for mode: \"%s\"", + file, line_nr, buff); FREE(buff); return 0; @@ -850,7 +863,9 @@ set_uid(vector strvec, void *ptr, int *flags, const char *file, int line_nr) else if (sscanf(buff, "%u", &uid) == 1){ *flags |= (1 << ATTR_UID); *uid_ptr = uid; - } + } else + condlog(1, "%s line %d, invalid value for uid: \"%s\"", + file, line_nr, buff); FREE(buff); return 0; @@ -876,7 +891,9 @@ set_gid(vector strvec, void *ptr, int *flags, const char *file, int line_nr) else if (sscanf(buff, "%u", &gid) == 1){ *flags |= (1 << ATTR_GID); *gid_ptr = gid; - } + } else + condlog(1, "%s line %d, invalid value for gid: \"%s\"", + file, line_nr, buff); FREE(buff); return 0; } @@ -978,7 +995,8 @@ set_dev_loss(vector strvec, void *ptr, const char *file, int line_nr) if (!strcmp(buff, "infinity")) *uint_ptr = MAX_DEV_LOSS_TMO; else if (sscanf(buff, "%u", uint_ptr) != 1) - *uint_ptr = DEV_LOSS_TMO_UNSET; + condlog(1, "%s line %d, invalid value for dev_loss_tmo: \"%s\"", + file, line_nr, buff); FREE(buff); return 0; @@ -1012,13 +1030,19 @@ static int set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr) { char * buff; + int policy; int *int_ptr = (int *)ptr; buff = set_value(strvec); if (!buff) return 1; - *int_ptr = get_pgpolicy_id(buff); + policy = get_pgpolicy_id(buff); + if (policy != IOPOLICY_UNDEF) + *int_ptr = policy; + else + condlog(1, "%s line %d, invalid value for path_grouping_policy: \"%s\"", + file, line_nr, buff); FREE(buff); return 0; @@ -1131,10 +1155,11 @@ set_rr_weight(vector strvec, void *ptr, const char *file, int line_nr) if (!strcmp(buff, "priorities")) *int_ptr = RR_WEIGHT_PRIO; - - if (!strcmp(buff, "uniform")) + else if (!strcmp(buff, "uniform")) *int_ptr = RR_WEIGHT_NONE; - + else + condlog(1, "%s line %d, invalid value for rr_weight: \"%s\"", + file, line_nr, buff); FREE(buff); return 0; @@ -1270,10 +1295,13 @@ def_log_checker_err_handler(struct config *conf, vector strvec, if (!buff) return 1; - if (strlen(buff) == 4 && !strcmp(buff, "once")) + if (!strcmp(buff, "once")) conf->log_checker_err = LOG_CHKR_ERR_ONCE; - else if (strlen(buff) == 6 && !strcmp(buff, "always")) + else if (!strcmp(buff, "always")) conf->log_checker_err = LOG_CHKR_ERR_ALWAYS; + else + condlog(1, "%s line %d, invalid value for log_checker_err: \"%s\"", + file, line_nr, buff); free(buff); return 0; @@ -1534,7 +1562,8 @@ hw_vpd_vendor_handler(struct config *conf, vector strvec, const char *file, goto out; } } - hwe->vpd_vendor_id = 0; + condlog(1, "%s line %d, invalid value for vpd_vendor: \"%s\"", + file, line_nr, buff); out: FREE(buff); return 0;
Add error reporting to the remaining config handlers. If the value is invalid, do not change the existing config option's value. Also print an error whenever 0 is returned for an invalid value. When the handler returns 1, config processing already fails with an error message. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/dict.c | 73 +++++++++++++++++++++++++++++++-------------- 1 file changed, 51 insertions(+), 22 deletions(-)