This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 10/08/2013 02:27 PM, Namhyung Kim wrote:
Hi Hemant,
Hi and thanks for reviewing the patches...
On Mon, 07 Oct 2013 12:17:57 +0530, Hemant Kumar wrote:[...] +static void cleanup_sdt_note_list(struct list_head *sdt_notes) +{ + struct sdt_note *tmp; + struct list_head *pos, *s; + + list_for_each_safe(pos, s, sdt_notes) { + tmp = list_entry(pos, struct sdt_note, note_list);You might use list_for_each_entry_safe() instead.
Ok, will use that.
+ list_del(pos); + free(tmp->name); + free(tmp->provider); + free(tmp); + } +} + static int convert_to_probe_trace_events(struct perf_probe_event *pev, struct probe_trace_event **tevs, int max_tevs, const char *target) @@ -2372,3 +2386,28 @@ out: free(name); return ret; } + +static void display_sdt_note_info(struct list_head *start) +{ + struct list_head *pos; + struct sdt_note *tmp; + + list_for_each(pos, start) { + tmp = list_entry(pos, struct sdt_note, note_list);Ditto. list_for_each_entry().
Ok...
+ printf("%%%s:%s\n", tmp->provider, tmp->name); + } +} + +int show_sdt_notes(const char *target) +{ + struct list_head sdt_notes; + int ret = -1; + + INIT_LIST_HEAD(&sdt_notes);You can use LIST_HEAD(sdt_notes) here.
Yes. Will use LIST_HEAD() instead.
+ ret = get_sdt_note_list(&sdt_notes, target); + if (!ret) { + display_sdt_note_info(&sdt_notes); + cleanup_sdt_note_list(&sdt_notes); + } + return ret; +} diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h index f9f3de8..b8a9347 100644 --- a/tools/perf/util/probe-event.h +++ b/tools/perf/util/probe-event.h @@ -133,7 +133,7 @@ extern int show_available_vars(struct perf_probe_event *pevs, int npevs, struct strfilter *filter, bool externs); extern int show_available_funcs(const char *module, struct strfilter *filter, bool user); - +int show_sdt_notes(const char *target); /* Maximum index number of event-name postfix */ #define MAX_EVENT_INDEX 1024diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.cindex 4b12bf8..6b17260 100644 --- a/tools/perf/util/symbol-elf.c +++ b/tools/perf/util/symbol-elf.c @@ -846,6 +846,182 @@ out_elf_end: return err; }+/*+ * Populate the name, type, offset in the SDT note structure and + * ignore the argument fields (for now) + */ +static struct sdt_note *populate_sdt_note(Elf **elf, const char *data, + size_t len, int type) +{ + const char *provider, *name; + struct sdt_note *note; + + /* + * Three addresses need to be obtained : + * Marker location, address of base section and semaphore location + */ + union { + Elf64_Addr a64[3]; + Elf32_Addr a32[3]; + } buf; + + /* + * dst and src are required for translation from file to memory + * representation + */ + Elf_Data dst = { + .d_buf = &buf, .d_type = ELF_T_ADDR, .d_version = EV_CURRENT, + .d_size = gelf_fsize((*elf), ELF_T_ADDR, 3, EV_CURRENT), + .d_off = 0, .d_align = 0 + }; + + Elf_Data src = { + .d_buf = (void *) data, .d_type = ELF_T_ADDR, + .d_version = EV_CURRENT, .d_size = dst.d_size, .d_off = 0, + .d_align = 0 + }; + + /* Check the type of each of the notes */ + if (type != SDT_NOTE_TYPE) + goto out_ret; + + note = (struct sdt_note *)zalloc(sizeof(struct sdt_note)); + if (note == NULL) { + pr_err("Memory allocation error\n"); + goto out_ret; + } + INIT_LIST_HEAD(¬e->note_list); + + if (len < dst.d_size + 3) + goto out_free; + + /* Translation from file representation to memory representation */ + if (gelf_xlatetom(*elf, &dst, &src, + elf_getident(*elf, NULL)[EI_DATA]) == NULL) + pr_debug("gelf_xlatetom : %s", elf_errmsg(-1));I doubt this translation is really needed as we only deal with SDTs on a local system only, right?
In case of SDTs on a local system, we don't need gelf_xlatetom().
+ + /* Populate the fields of sdt_note */ + provider = data + dst.d_size; + + name = (const char *)memchr(provider, '\0', data + len - provider); + if (name++ == NULL) + goto out_free; + note->provider = strdup(provider); + note->name = strdup(name);You need to check the return value of strdup's and it should also be freed when an error occurres after this.
Missed that. Thanks for pointing that out.
[...] + /* 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))) { + tmp = populate_sdt_note(&elf, (const char *) + ((long)(data->d_buf) + + (long)desc_off),It seems that data->d_buf is type of void *, so these casts can go away, I guess.
Yeah correct, we don't need these casts.
+ nhdr.n_descsz, nhdr.n_type); + if (!note && tmp) + note = tmp; + else if (tmp) + list_add_tail(&tmp->note_list, + &(note->note_list)); + } + } + if (tmp) + return &tmp->note_list;This list handling code looks very strange to me. Why not just passing a list head and connect notes to it?
Yes it will be better to use list_head...
+out_err: + return NULL; +} + +int get_sdt_note_list(struct list_head *head, const char *target) +{ + Elf *elf; + int fd, ret = -1; + struct list_head *tmp = NULL; + + fd = open(target, O_RDONLY); + if (fd < 0) { + pr_err("Failed to open %s\n", target); + goto out_ret; + } + + symbol__elf_init(); + elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL); + if (!elf) { + pr_err("%s: cannot read %s ELF file.\n", __func__, target); + goto out_close; + } + tmp = construct_sdt_notes_list(elf); + if (tmp) { + list_add(head, tmp);Look like the params are exchanged? /** * list_add - add a new entry * @new: new entry to be added * @head: list head to add it after * * Insert a new entry after the specified head. * This is good for implementing stacks. */ static inline void list_add(struct list_head *new, struct list_head *head) { __list_add(new, head, head->next); }
I guess they won't be exchanged... tmp will be added to head, right? Anyway, this won't be needed if we go with list_head in struct probe_point instead of sdt_note. Will make the required changes.
+ ret = 0; + } + elf_end(elf); + +out_close: + close(fd); +out_ret: + return ret; +} + void symbol__elf_init(void) { elf_version(EV_CURRENT); diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h index 5f720dc..037185c 100644 --- a/tools/perf/util/symbol.h +++ b/tools/perf/util/symbol.h @@ -197,6 +197,17 @@ struct symsrc { #endif };+struct sdt_note {+ char *name; + char *provider; + bool bit32; /* 32 or 64 bit flag */ + union { + Elf64_Addr a64[3]; + Elf32_Addr a32[3]; + } addr; + struct list_head note_list; +}; + void symsrc__destroy(struct symsrc *ss); int symsrc__init(struct symsrc *ss, struct dso *dso, const char *name, enum dso_binary_type type); @@ -247,4 +258,11 @@ void symbols__fixup_duplicate(struct rb_root *symbols); void symbols__fixup_end(struct rb_root *symbols); void __map_groups__fixup_end(struct map_groups *mg, enum map_type type);+/* Specific to SDT notes */+int get_sdt_note_list(struct list_head *head, const char *target); + +#define SDT_NOTE_TYPE 3 +#define NOTE_SCN ".note.stapsdt"What about SDT_NOTE_SCN for consistency?
Seems better. Will change to that. -- Thanks Hemant
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |