On Fri, Jun 19, 2020 at 2:57 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Only two small places on this version, otherwise it looks good to me.
> I can offer my reviewed-by, if need. :)
>
> Thanks for the patch!
>
> Reviewed-by: Hao Luo <haoluo@xxxxxxxxxx>
>
> On Fri, Jun 19, 2020 at 1:34 PM Andrii Nakryiko <andriin@xxxxxx> wrote:
> >
> > Switch existing Kconfig externs to be just one of few possible kinds of more
> > generic externs. This refactoring is in preparation for ksymbol extern
> > support, added in the follow up patch. There are no functional changes
> > intended.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> > ---
>
> [...]
>
> > @@ -2756,23 +2796,29 @@ static int cmp_externs(const void *_a, const void *_b)
>
> [...]
>
> > +
> > + if (a->type == EXT_KCFG) {
> > + /* descending order by alignment requirements */
> > + if (a->kcfg.align != b->kcfg.align)
> > + return a->kcfg.align > b->kcfg.align ? -1 : 1;
> > + /* ascending order by size, within same alignment class */
> > + if (a->kcfg.sz != b->kcfg.sz)
> > + return a->kcfg.sz < b->kcfg.sz ? -1 : 1;
> > + /* resolve ties by name */
> > + }
> > +
> > return strcmp(a->name, b->name);
> > }
>
> I assume the comment /* resolve ties by name */ is intended to be
> close to strcmp?
yep
>
> > @@ -2818,22 +2864,39 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
> > ext->name = btf__name_by_offset(obj->btf, t->name_off);
> > ext->sym_idx = i;
> > ext->is_weak = GELF_ST_BIND(sym.st_info) == STB_WEAK;
> > - ext->sz = btf__resolve_size(obj->btf, t->type);
> > - if (ext->sz <= 0) {
> > - pr_warn("failed to resolve size of extern '%s': %d\n",
> > - ext_name, ext->sz);
> > - return ext->sz;
> > - }
> > - ext->align = btf__align_of(obj->btf, t->type);
> > - if (ext->align <= 0) {
> > - pr_warn("failed to determine alignment of extern '%s': %d\n",
> > - ext_name, ext->align);
> > - return -EINVAL;
> > - }
> > - ext->type = find_extern_type(obj->btf, t->type,
> > - &ext->is_signed);
> > - if (ext->type == EXT_UNKNOWN) {
> > - pr_warn("extern '%s' type is unsupported\n", ext_name);
> > +
> > + ext->sec_btf_id = find_extern_sec_btf_id(obj->btf, ext->btf_id);
> > + if (ext->btf_id <= 0) {
> > + pr_warn("failed to find BTF for extern '%s' [%d] section: %d\n",
> > + ext_name, ext->btf_id, ext->sec_btf_id);
> > + return ext->sec_btf_id;
> > + }
>
> Did you mean "ext->sec_btf_id <= 0" rather than "ext->btf_id <= 0"?
yep, argh...
