| Message ID | 20210205134221.2953163-3-gprocida@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | ELF writing changes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote: > > Many operations in the libelf API return a pointer to a user-provided > struct (on success) or NULL (on failure). > > There are a couple of places in btf_elf__write where both structs and > pointers to the same structs are used. Holding on to the pointers > raises ownership and lifetime issues unncessarily and the code is typo: unnecessarily > cleaner with only a single access path for these data. > > The code now treats the returned pointers as booleans. > > Signed-off-by: Giuliano Procida <gprocida@google.com> > --- styling nits, but otherwise LGTM Acked-by: Andrii Nakryiko <andrii@kernel.org> > libbtf.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/libbtf.c b/libbtf.c > index 7bc49ba..ace8896 100644 > --- a/libbtf.c > +++ b/libbtf.c > @@ -698,8 +698,7 @@ int32_t btf_elf__add_datasec_type(struct btf_elf *btfe, const char *section_name > > static int btf_elf__write(const char *filename, struct btf *btf) > { > - GElf_Shdr shdr_mem, *shdr; > - GElf_Ehdr ehdr_mem, *ehdr; > + GElf_Ehdr ehdr; > Elf_Data *btf_data = NULL; > Elf_Scn *scn = NULL; > Elf *elf = NULL; > @@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf) > > elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY); > > - ehdr = gelf_getehdr(elf, &ehdr_mem); > - if (ehdr == NULL) { > + if (!gelf_getehdr(elf, &ehdr)) { > elf_error("elf_getehdr failed"); > goto out; > } > > - switch (ehdr_mem.e_ident[EI_DATA]) { > + switch (ehdr.e_ident[EI_DATA]) { > case ELFDATA2LSB: > btf__set_endianness(btf, BTF_LITTLE_ENDIAN); > break; > @@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf) > > elf_getshdrstrndx(elf, &strndx); > while ((scn = elf_nextscn(elf, scn)) != NULL) { > - shdr = gelf_getshdr(scn, &shdr_mem); > - if (shdr == NULL) > + GElf_Shdr shdr; it's a good style to have an empty line between variable declaration block and subsequent instructions > + if (!gelf_getshdr(scn, &shdr)) > continue; > - char *secname = elf_strptr(elf, strndx, shdr->sh_name); > + char *secname = elf_strptr(elf, strndx, shdr.sh_name); > if (strcmp(secname, ".BTF") == 0) { > btf_data = elf_getdata(scn, btf_data); > break; > -- > 2.30.0.478.g8a0d178c01-goog >
Hi. On Mon, 8 Feb 2021 at 22:23, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida <gprocida@google.com> wrote: > > > > Many operations in the libelf API return a pointer to a user-provided > > struct (on success) or NULL (on failure). > > > > There are a couple of places in btf_elf__write where both structs and > > pointers to the same structs are used. Holding on to the pointers > > raises ownership and lifetime issues unncessarily and the code is > > typo: unnecessarily > Thanks. Fixed. > > cleaner with only a single access path for these data. > > > > The code now treats the returned pointers as booleans. > > > > Signed-off-by: Giuliano Procida <gprocida@google.com> > > --- > > styling nits, but otherwise LGTM > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > libbtf.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/libbtf.c b/libbtf.c > > index 7bc49ba..ace8896 100644 > > --- a/libbtf.c > > +++ b/libbtf.c > > @@ -698,8 +698,7 @@ int32_t btf_elf__add_datasec_type(struct btf_elf *btfe, const char *section_name > > > > static int btf_elf__write(const char *filename, struct btf *btf) > > { > > - GElf_Shdr shdr_mem, *shdr; > > - GElf_Ehdr ehdr_mem, *ehdr; > > + GElf_Ehdr ehdr; > > Elf_Data *btf_data = NULL; > > Elf_Scn *scn = NULL; > > Elf *elf = NULL; > > @@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf) > > > > elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY); > > > > - ehdr = gelf_getehdr(elf, &ehdr_mem); > > - if (ehdr == NULL) { > > + if (!gelf_getehdr(elf, &ehdr)) { > > elf_error("elf_getehdr failed"); > > goto out; > > } > > > > - switch (ehdr_mem.e_ident[EI_DATA]) { > > + switch (ehdr.e_ident[EI_DATA]) { > > case ELFDATA2LSB: > > btf__set_endianness(btf, BTF_LITTLE_ENDIAN); > > break; > > @@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf) > > > > elf_getshdrstrndx(elf, &strndx); > > while ((scn = elf_nextscn(elf, scn)) != NULL) { > > - shdr = gelf_getshdr(scn, &shdr_mem); > > - if (shdr == NULL) > > + GElf_Shdr shdr; > > it's a good style to have an empty line between variable declaration > block and subsequent instructions > The variable in this question is effectively initialised by the statement on the next line, breaking them apart looks odd. Also, this is not a variable that needs end-of-scope clean-up. Its position at the top of the scope is coincidental. Later commits in the series also place declaration and initialisation as close together as possible. The only variables I would intentionally place at the top of a given scope *and* far from their natural points of initialisation are those corresponding to resources that need to be released at the end of the scope, with the labelled exit idiom. I feel this gives a better balance between readability (keeping things local) and keeping track of resources (memory, fds, other handles) in a scope. However, if that's contrary to the house style, it's easy enough to pull all the declarations out and move them to the top and separate them; the compiler should be clever enough to share stack slots in any case. Let me know. Regards, Giuliano. > > > + if (!gelf_getshdr(scn, &shdr)) > > continue; > > - char *secname = elf_strptr(elf, strndx, shdr->sh_name); > > + char *secname = elf_strptr(elf, strndx, shdr.sh_name); > > if (strcmp(secname, ".BTF") == 0) { > > btf_data = elf_getdata(scn, btf_data); > > break; > > -- > > 2.30.0.478.g8a0d178c01-goog > >
diff --git a/libbtf.c b/libbtf.c index 7bc49ba..ace8896 100644 --- a/libbtf.c +++ b/libbtf.c @@ -698,8 +698,7 @@ int32_t btf_elf__add_datasec_type(struct btf_elf *btfe, const char *section_name static int btf_elf__write(const char *filename, struct btf *btf) { - GElf_Shdr shdr_mem, *shdr; - GElf_Ehdr ehdr_mem, *ehdr; + GElf_Ehdr ehdr; Elf_Data *btf_data = NULL; Elf_Scn *scn = NULL; Elf *elf = NULL; @@ -727,13 +726,12 @@ static int btf_elf__write(const char *filename, struct btf *btf) elf_flagelf(elf, ELF_C_SET, ELF_F_DIRTY); - ehdr = gelf_getehdr(elf, &ehdr_mem); - if (ehdr == NULL) { + if (!gelf_getehdr(elf, &ehdr)) { elf_error("elf_getehdr failed"); goto out; } - switch (ehdr_mem.e_ident[EI_DATA]) { + switch (ehdr.e_ident[EI_DATA]) { case ELFDATA2LSB: btf__set_endianness(btf, BTF_LITTLE_ENDIAN); break; @@ -751,10 +749,10 @@ static int btf_elf__write(const char *filename, struct btf *btf) elf_getshdrstrndx(elf, &strndx); while ((scn = elf_nextscn(elf, scn)) != NULL) { - shdr = gelf_getshdr(scn, &shdr_mem); - if (shdr == NULL) + GElf_Shdr shdr; + if (!gelf_getshdr(scn, &shdr)) continue; - char *secname = elf_strptr(elf, strndx, shdr->sh_name); + char *secname = elf_strptr(elf, strndx, shdr.sh_name); if (strcmp(secname, ".BTF") == 0) { btf_data = elf_getdata(scn, btf_data); break;
Many operations in the libelf API return a pointer to a user-provided struct (on success) or NULL (on failure). There are a couple of places in btf_elf__write where both structs and pointers to the same structs are used. Holding on to the pointers raises ownership and lifetime issues unncessarily and the code is cleaner with only a single access path for these data. The code now treats the returned pointers as booleans. Signed-off-by: Giuliano Procida <gprocida@google.com> --- libbtf.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)