On Thu, Nov 04, 2021 at 08:54:11PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-06 at 15:04 -0500, Benjamin Marzinski wrote:
> > Split the code that does the actual value parsing out of set_int(),
> > into
> > a helper function, do_set_int(), so that it can be used by other
> > handlers. These functions no longer set the config value at all, when
> > they have invalid input.
>
> Not sure about that, do_set_int() sets the value to the cap (see below)
Sorry for the confustion. That's not what I meant. I meant that if
do_set_int() returns failure, we won't override the existing value in
the config.
>
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > ---
> > libmultipath/dict.c | 82 +++++++++++++++++++++++++------------------
> > --
> > 1 file changed, 46 insertions(+), 36 deletions(-)
> >
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index 91333068..e79fcdd7 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -31,17 +31,12 @@
> > #include "strbuf.h"
> >
> > static int
> > -set_int(vector strvec, void *ptr, int min, int max, const char
> > *file,
> > - int line_nr)
> > +do_set_int(vector strvec, void *ptr, int min, int max, const char
> > *file,
> > + int line_nr, char *buff)
> > {
> > int *int_ptr = (int *)ptr;
> > - char *buff, *eptr;
> > + char *eptr;
> > long res;
> > - int rc;
> > -
> > - buff = set_value(strvec);
> > - if (!buff)
> > - return 1;
> >
> > res = strtol(buff, &eptr, 10);
> > if (eptr > buff)
> > @@ -50,17 +45,30 @@ set_int(vector strvec, void *ptr, int min, int
> > max, const char *file,
> > if (*buff == '\0' || *eptr != '\0') {
> > condlog(1, "%s line %d, invalid value for %s:
> > \"%s\"",
> > file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > buff);
> > - rc = 1;
> > - } else {
> > - if (res > max || res < min) {
> > - res = (res > max) ? max : min;
> > - condlog(1, "%s line %d, value for %s too %s,
> > capping at %ld",
> > + return 1;
> > + }
> > + if (res > max || res < min) {
> > + res = (res > max) ? max : min;
> > + condlog(1, "%s line %d, value for %s too %s, capping
> > at %ld",
> > file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > - (res == max)? "large" : "small", res);
> > - }
> > - rc = 0;
> > - *int_ptr = res;
> > + (res == max)? "large" : "small", res);
> > }
> > + *int_ptr = res;
> > + return 0;
> > +}
> > +
> > +static int
> > +set_int(vector strvec, void *ptr, int min, int max, const char
> > *file,
> > + int line_nr)
> > +{
> > + char *buff;
> > + int rc;
> > +
> > + buff = set_value(strvec);
> > + if (!buff)
> > + return 1;
> > +
> > + rc = do_set_int(strvec, ptr, min, max, file, line_nr, buff);
> >
> > FREE(buff);
> > return rc;
> > @@ -918,6 +926,7 @@ declare_mp_attr_snprint(gid, print_gid)
> > static int
> > set_undef_off_zero(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc = 0;
> > char * buff;
> > int *int_ptr = (int *)ptr;
> >
> > @@ -927,10 +936,10 @@ set_undef_off_zero(vector strvec, void *ptr,
> > const char *file, int line_nr)
> >
> > if (strcmp(buff, "off") == 0)
> > *int_ptr = UOZ_OFF;
> > - else if (sscanf(buff, "%d", int_ptr) != 1 ||
> > - *int_ptr < UOZ_ZERO)
> > - *int_ptr = UOZ_UNDEF;
> > - else if (*int_ptr == 0)
> > + else
> > + rc = do_set_int(strvec, int_ptr, 0, INT_MAX, file,
> > line_nr,
> > + buff);
> > + if (rc == 0 && *int_ptr == 0)
> > *int_ptr = UOZ_ZERO;
> >
> > FREE(buff);
> > @@ -1082,14 +1091,12 @@ max_fds_handler(struct config *conf, vector
> > strvec, const char *file,
> > /* Assume safe limit */
> > max_fds = 4096;
> > }
> > - if (strlen(buff) == 3 &&
> > - !strcmp(buff, "max"))
> > - conf->max_fds = max_fds;
> > - else
> > - conf->max_fds = atoi(buff);
> > -
> > - if (conf->max_fds > max_fds)
> > + if (!strcmp(buff, "max")) {
> > conf->max_fds = max_fds;
> > + r = 0;
> > + } else
> > + r = do_set_int(strvec, &conf->max_fds, 0, max_fds,
> > file,
> > + line_nr, buff);
> >
> > FREE(buff);
> >
> > @@ -1158,6 +1165,7 @@ declare_mp_snprint(rr_weight, print_rr_weight)
> > static int
> > set_pgfailback(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc = 0;
> > int *int_ptr = (int *)ptr;
> > char * buff;
> >
> > @@ -1172,11 +1180,11 @@ set_pgfailback(vector strvec, void *ptr,
> > const char *file, int line_nr)
> > else if (strlen(buff) == 10 && !strcmp(buff, "followover"))
> > *int_ptr = -FAILBACK_FOLLOWOVER;
> > else
> > - *int_ptr = atoi(buff);
> > + rc = do_set_int(strvec, ptr, 0, INT_MAX, file,
> > line_nr, buff);
> >
> > FREE(buff);
> >
> > - return 0;
> > + return rc;
> > }
> >
> > int
> > @@ -1208,6 +1216,7 @@ declare_mp_snprint(pgfailback,
> > print_pgfailback)
> > static int
> > no_path_retry_helper(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc = 0;
> > int *int_ptr = (int *)ptr;
> > char * buff;
> >
> > @@ -1219,11 +1228,11 @@ no_path_retry_helper(vector strvec, void
> > *ptr, const char *file, int line_nr)
> > *int_ptr = NO_PATH_RETRY_FAIL;
> > else if (!strcmp(buff, "queue"))
> > *int_ptr = NO_PATH_RETRY_QUEUE;
> > - else if ((*int_ptr = atoi(buff)) < 1)
> > - *int_ptr = NO_PATH_RETRY_UNDEF;
> > + else
> > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file,
> > line_nr, buff);
>
> This will set no_path_retry to 1 if the input was something like "0 "
> or a negative value. The previous code would have set
> NO_PATH_RETRY_UNDEF (== 0). That's a semantic change, as the code
> checks for NO_PATH_RETRY_UNDEF in various places. Was this intentional?
>
Not completely. I do think that is makes sense not to change the
existing value if the input is invalid. I admit that I didn't think
about the fact that "0 " wouldn't be the same as "0". It certainly
makes sense to change this so that do_set_int() accepts 0, and then we
can handle 0 afterwards.
It might also make sense in some cases to simply treat values outside
the range as invalid, instead of capping them. Thoughts?
> >
> > FREE(buff);
> > - return 0;
> > + return rc;
> > }
> >
> > int
> > @@ -1365,6 +1374,7 @@ snprint_mp_reservation_key (struct config
> > *conf, struct strbuf *buff,
> > static int
> > set_off_int_undef(vector strvec, void *ptr, const char *file, int
> > line_nr)
> > {
> > + int rc =0;
> > int *int_ptr = (int *)ptr;
> > char * buff;
> >
> > @@ -1374,11 +1384,11 @@ set_off_int_undef(vector strvec, void *ptr,
> > const char *file, int line_nr)
> >
> > if (!strcmp(buff, "no") || !strcmp(buff, "0"))
> > *int_ptr = NU_NO;
> > - else if ((*int_ptr = atoi(buff)) < 1)
> > - *int_ptr = NU_UNDEF;
> > + else
> > + rc = do_set_int(strvec, ptr, 1, INT_MAX, file,
> > line_nr, buff);
>
> Likewise, you'd set 1 here for negative input or "0 ", while
> previously the result would be NU_UNDEF == 0.
>
> Negative values are of course garbage and I'm unsure if trailing spaces
> can occur at this point in the code, but do_set_int() handles them.
Same here.
-Ben
> Regards,
> Martin
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
