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 ?
> + 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 ?
> +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?
> +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.
> + 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.
> + 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?
