Hi.

On Mon, 8 Feb 2021 at 22:23, Andrii Nakryiko [off-list ref] wrote:

On Fri, Feb 5, 2021 at 5:42 AM Giuliano Procida [off-list ref] wrote:

quoted

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.

quoted

cleaner with only a single access path for these data.

The code now treats the returned pointers as booleans.

Signed-off-by: Giuliano Procida <redacted>
---

styling nits, but otherwise LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

quoted

 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.

quoted

+               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