Re: [PATCH bpf-next v5 1/2] selftests/bpf: implement setting global variables in veristat

[Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>]

On Tue, Feb 25, 2025 at 1:56 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Feb 25, 2025 at 11:04 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > On Tue, 2025-02-25 at 16:31 +0000, Mykyta Yatsenko wrote:
> >
> > New warning for trying to set non-enums from enumerators works fine.
> > This still can be abused if numeric value outside of the supported
> > range is specified, but that's fine, I think.
> >
> > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> >
> > [...]
> >
> > > @@ -1292,6 +1320,268 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
> > >       return 0;
> > >  };
> > >
> > > +static int append_var_preset(struct var_preset **presets, int *cnt, const char *expr)
> > > +{
> > > +     void *tmp;
> > > +     struct var_preset *cur;
> > > +     char var[256], val[256];
> > > +     long long value;
> > > +     int r, n, val_len;
> > > +
> > > +     tmp = realloc(*presets, (*cnt + 1) * sizeof(**presets));
> > > +     if (!tmp)
> > > +             return -ENOMEM;
> > > +     *presets = tmp;
> > > +     cur = &(*presets)[*cnt];
> > > +     cur->applied = false;
> > > +
> > > +     if (sscanf(expr, "%s = %s\n", var, val) != 2) {
> > > +             fprintf(stderr, "Could not parse expression %s\n", expr);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     val_len = strlen(val);
> > > +     errno = 0;
> > > +     r = sscanf(val, "%lli %n", &value, &n);
> > > +     if (r == 1 && n == val_len) {
> > > +             if (errno == ERANGE) {
> > > +                     /* Try parsing as unsigned */
> > > +                     errno = 0;
> > > +                     r = sscanf(val, "%llu %n", &value, &n);
> > > +                     /* Try hex if not all chars consumed */
> > > +                     if (n != val_len) {
> > > +                             errno = 0;
> > > +                             r = sscanf(val, "%llx %n", &value, &n);
> >
> > The discrepancy between %lli accepting 0x but %llu not accepting 0x is
> > annoying unfortunate. Does not look simpler then before, but let's
> > merge this already.
>
> yeah... with sscanf() we at least get whitespace trimming at end, so
> don't need to check that. But I agree, doesn't look better.

While applying I realized that space trimming is not necessary,
because earlier sscanf() does that for us. So I rewrote this back to
something that looks like the earlier version. I think it's cleaner. I
ended up with this implementation of append_var_preset:

static int append_var_preset(struct var_preset **presets, int *cnt,
const char *expr)
{
        void *tmp;
        struct var_preset *cur;
        char var[256], val[256], *val_end;
        long long value;
        int n;

        tmp = realloc(*presets, (*cnt + 1) * sizeof(**presets));
        if (!tmp)
                return -ENOMEM;
        *presets = tmp;
        cur = &(*presets)[*cnt];
        memset(cur, 0, sizeof(*cur));
        (*cnt)++;

        if (sscanf(expr, "%s = %s %n", var, val, &n) != 2 || n !=
strlen(expr)) {
                fprintf(stderr, "Failed to parse expression '%s'\n", expr);
                return -EINVAL;
        }

        if (val[0] == '-' || isdigit(val[0])) {
                /* must be a number */
                errno = 0;
                value = strtoll(val, &val_end, 0);
                if (errno == ERANGE) {
                        errno = 0;
                        value = strtoull(val, &val_end, 0);
                }
                if (errno || *val_end != '\0') {
                        fprintf(stderr, "Failed to parse value '%s'\n", val);
                        return -EINVAL;
                }
                cur->ivalue = value;
                cur->type = INTEGRAL;
        } else {
                /* if not a number, consider it enum value */
                cur->svalue = strdup(val);
                if (!cur->svalue)
                        return -ENOMEM;
                cur->type = ENUMERATOR;
        }

        cur->name = strdup(var);
        if (!cur->name)
                return -ENOMEM;

        return 0;
}


I added memset() and early cnt++ to make error handling more correct
(we could have leaked cur->svalue if strdup(var) fails). And I added
%n to initial sscanf() to validate we parsed the entire string.

Oh, I also added back '-' or is_digit() detection, as otherwise users
can be confused due to a small typo in their integer (they'd get
something about enum, very confusing).

So that's what I applied before pushing to bpf-next. Yell if I messed
something up.

>
> >
> > > +                     }
> > > +             }
> > > +             if (errno || r != 1 || n != val_len) {
> > > +                     fprintf(stderr, "Could not parse value %s\n", val);
> > > +                     return -EINVAL;
> > > +             }
> > > +             cur->ivalue = value;
> > > +             cur->type = INTEGRAL;
> > > +     } else {
> > > +             /* If not a number, consider it enum value */
> > > +             cur->svalue = strdup(val);
> > > +             if (!cur->svalue)
> > > +                     return -ENOMEM;
> > > +
> > > +             cur->type = ENUMERATOR;
> > > +     }
> > > +
> > > +     cur->name = strdup(var);
> > > +     if (!cur->name)
> > > +             return -ENOMEM;
> > > +
> > > +     (*cnt)++;
> > > +     return 0;
> > > +}
> >
> > [...]
> >