Messages in this thread | | | From | Namhyung Kim <> | Subject | Re: [PATCH v2 1/5] perf/sdt: ELF support for SDT | Date | Tue, 07 Oct 2014 11:20:00 +0900 |
| |
Hi Hemant,
On Wed, 01 Oct 2014 08:17:24 +0530, Hemant Kumar wrote: > This patch serves as the basic support to identify and list SDT events in binaries. > When programs containing SDT markers are compiled, gcc with the help of assembler > directives identifies them and places them in the section ".note.stapsdt". To find > these markers from the binaries, one needs to traverse through this section and > parse the relevant details like the name, type and location of the marker. Also, > the original location could be skewed due to the effect of prelinking. If that is > the case, the locations need to be adjusted. > > The functions in this patch open a given ELF, find out the SDT section, parse the > relevant details, adjust the location (if necessary) and populate them in a list. > > Made the necessary changes as suggested by Namhyung Kim.
Below are just few more nitpicks, other than that, looks good to me.
[SNIP] > + /* Get the SDT notes */ > + for (offset = 0; (next = gelf_getnote(data, offset, &nhdr, &name_off, > + &desc_off)) > 0; offset = next) { > + if (nhdr.n_namesz == sizeof(SDT_NOTE_NAME) && > + !memcmp(data->d_buf + name_off, SDT_NOTE_NAME, > + sizeof(SDT_NOTE_NAME))) { > + ret = populate_sdt_note(&elf, ((data->d_buf) + desc_off), > + nhdr.n_descsz, nhdr.n_type, > + &tmp); > + if (ret < 0) > + goto out_ret; > + list_add_tail(&tmp->note_list, sdt_notes);
I think we can just pass the sdt_notes list into populate_sdt_note() instead of passing a tmp pointer.
> + } > + } > + if (list_empty(sdt_notes)) > + ret = -ENOENT; > + > +out_ret: > + return ret; > +} > + > +/* > + * get_sdt_note_list() : Takes two arguments "head" and "target", where head > + * is the head of the SDT events' list and "target" is the file name as to > + * where the SDT events should be looked for. This opens the file, initializes > + * the ELF and then calls construct_sdt_notes_list. > + */ > +int get_sdt_note_list(struct list_head *head, const char *target) > +{ > + Elf *elf; > + int fd, ret; > + > + fd = open(target, O_RDONLY); > + if (fd < 0) > + return -EBADF; > + > + elf = elf_begin(fd, ELF_C_READ, NULL);
You may want to use PERF_ELF_C_READ_MMAP. :)
Thanks, Namhyung
> + if (!elf) { > + ret = -EBADF; > + goto out_close; > + } > + ret = construct_sdt_notes_list(elf, head); > + elf_end(elf); > +out_close: > + close(fd); > + return ret; > +}
| |