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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
