Re: [PATCH bpf-next 1/2] tools/resolve_btfids: emit warnings and patch zero id for missing symbols

[Hengqi Chen <hengqi.chen@gmail.com>]

On 2021/7/26 11:32 AM, Yonghong Song wrote:
> 
> 
> On 7/25/21 7:18 AM, Hengqi Chen wrote:
>> Kernel functions referenced by .BTF_ids may changed from global to static
>> and get inlined and thus disappears from BTF. This causes kernel build
> 
> the function could be renamed or removed too.
> 
>> failure when resolve_btfids do id patch for symbols in .BTF_ids in vmlinux.
>> Update resolve_btfids to emit warning messages and patch zero id for missing
>> symbols instead of aborting kernel build process.
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> 
> LGTM with one minor comment below.
> 
> Acked-by: Yonghong Song <yhs@fb.com>
> 
>> ---
>>   tools/bpf/resolve_btfids/main.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
>> index 3ad9301b0f00..3ea19e33250d 100644
>> --- a/tools/bpf/resolve_btfids/main.c
>> +++ b/tools/bpf/resolve_btfids/main.c
>> @@ -291,7 +291,7 @@ static int compressed_section_fix(Elf *elf, Elf_Scn *scn, GElf_Shdr *sh)
>>       sh->sh_addralign = expected;
>>
>>       if (gelf_update_shdr(scn, sh) == 0) {
>> -        printf("FAILED cannot update section header: %s\n",
>> +        pr_err("FAILED cannot update section header: %s\n",
>>               elf_errmsg(-1));
>>           return -1;
>>       }
>> @@ -317,6 +317,7 @@ static int elf_collect(struct object *obj)
>>
>>       elf = elf_begin(fd, ELF_C_RDWR_MMAP, NULL);
>>       if (!elf) {
>> +        close(fd);
>>           pr_err("FAILED cannot create ELF descriptor: %s\n",
>>               elf_errmsg(-1));
>>           return -1;
>> @@ -484,7 +485,7 @@ static int symbols_resolve(struct object *obj)
>>       err = libbpf_get_error(btf);
>>       if (err) {
>>           pr_err("FAILED: load BTF from %s: %s\n",
>> -            obj->path, strerror(-err));
>> +            obj->btf ?: obj->path, strerror(-err));
> 
> Why you change "obj->path" to "obj->btf ?: obj->path"?
> Note that obj->path cannot be NULL.

The diff didn't see the whole picture. Let me quote it here:
```
btf = btf__parse(obj->btf ?: obj->path, NULL);
err = libbpf_get_error(btf);
if (err) {
        pr_err("FAILED: load BTF from %s: %s\n",
                obj->path, strerror(-err));
        return -1;
}
```

Because btf__parse parses either obj->btf or obj->path,
I think the error message should reveal this.

> 
>>           return -1;
>>       }
>>
> [...]