On Thu, Dec 12, 2019 at 9:11 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> 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.
