Re: [PATCH v2 bpf-next 1/3] libbpf: support libbpf-provided extern variables

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Dec 12, 2019 at 9:11 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Dec 10, 2019 at 10:50:00PM -0800, Andrii Nakryiko wrote:
> > +static int set_ext_value_tri(struct extern_desc *ext, void *ext_val,
> > +                          char value)
> > +{
> > +     switch (ext->type) {
> > +     case EXT_BOOL:
> > +             if (value == 'm') {
> > +                     pr_warn("extern %s=%c should be tristate or char\n",
> > +                             ext->name, value);
> > +                     return -EINVAL;
> > +             }
> > +             *(bool *)ext_val = value == 'y' ? true : false;
>
> may be check for strict y/n ?

Can't be anything else due to `case 'y': case 'n': case 'm':` check in
the caller.


>
> > +             break;
> > +     case EXT_TRISTATE:
> > +             if (value == 'y')
> > +                     *(enum libbpf_tristate *)ext_val = TRI_YES;
> > +             else if (value == 'm')
> > +                     *(enum libbpf_tristate *)ext_val = TRI_MODULE;
> > +             else /* value == 'n' */
> > +                     *(enum libbpf_tristate *)ext_val = TRI_NO;
>
> same here ?
>
> > +             break;
> > +     case EXT_CHAR:
> > +             *(char *)ext_val = value;
> > +             break;
> > +     case EXT_UNKNOWN:
> > +     case EXT_INT:
> > +     case EXT_CHAR_ARR:
>
> why enumerate them when there is a default ?

Because compiler is too smart and strict. Otherwise getting:

libbpf.c: In function ‘set_ext_value_tri’:
libbpf.c:982:2: error: enumeration value ‘EXT_UNKNOWN’ not handled in
switch [-Werror=switch-enum]
  switch (ext->type) {
  ^~~~~~
libbpf.c:982:2: error: enumeration value ‘EXT_INT’ not handled in
switch [-Werror=switch-enum]
libbpf.c:982:2: error: enumeration value ‘EXT_CHAR_ARR’ not handled in
switch [-Werror=switch-enum]

>
> > +static int set_ext_value_str(struct extern_desc *ext, void *ext_val,
> > +                          const char *value)
> > +{
> > +     size_t len;
> > +
> > +     if (ext->type != EXT_CHAR_ARR) {
> > +             pr_warn("extern %s=%s should char array\n", ext->name, value);
> > +             return -EINVAL;
> > +     }
> > +
> > +     len = strlen(value);
> > +     if (value[len - 1] != '"') {
> > +             pr_warn("extern '%s': invalid string config '%s'\n",
> > +                     ext->name, value);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* strip quotes */
> > +     len -= 2;
> > +     if (len + 1 > ext->sz) {
> > +             pr_warn("extern '%s': too long string config %s (%zu bytes), up to %d expected\n",
> > +                     ext->name, value, len + 1, ext->sz);
> > +             return -EINVAL;
>
> may be print warning and truncate instead of hard error?

Could do. I erred on the side of being strict. I don't mind relaxing
this, though.

>
> > +static bool is_ext_value_in_range(const struct extern_desc *ext, __u64 v)
> > +{
> > +     int bit_sz = ext->sz * 8;
> > +
> > +     if (ext->sz == 8)
> > +             return true;
> > +
> > +     if (ext->is_signed)
> > +             return v + (1ULL << (bit_sz - 1)) < (1ULL << bit_sz);
> > +     else
> > +             return (v >> bit_sz) == 0;
>
> a comment would be helpful.

will add

>
> > +             ext_val = data + ext->data_off;
> > +             value = sep + 1;
> > +
> > +             switch (*value) {
> > +             case 'y': case 'n': case 'm':
>
> I don't think config.gz has 'n', but it's good to have it here.
>
> > -                     } else if (strcmp(name, ".data") == 0) {
> > +                     } else if (strcmp(name, DATA_SEC) == 0) {
> >                               obj->efile.data = data;
> >                               obj->efile.data_shndx = idx;
> > -                     } else if (strcmp(name, ".rodata") == 0) {
> > +                     } else if (strcmp(name, RODATA_SEC) == 0) {
>
> such cleanup changes should be in separate patch.

Ok, will split out. Needed that for .extern, decided to complete for
others in the same go.

>
> > +             if (strcmp(ext->name, "LINUX_KERNEL_VERSION") == 0) {
> > +                     void *ext_val = data + ext->data_off;
> > +                     __u32 kver = get_kernel_version();
> > +
> > +                     if (!kver) {
> > +                             pr_warn("failed to get kernel version\n");
> > +                             return -EINVAL;
> > +                     }
> > +                     err = set_ext_value_num(ext, ext_val, kver);
>
> I think it should work when kern_ver is not 'int'.
> Could you add a test for u64 ?
> Or it will fail on big endian?
>

Yeah, it is handled inside set_ext_value_num just the same as any
CONFIG_xxx integers. I will make sure that test_core_extern and
test_skeleton use both u32 and u64.