On Fri, Oct 29, 2021 at 12:41 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Thu, 28 Oct 2021 15:08:56 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
>
[...]
> > +
> > +struct tracefs_dynevent *
> > +dynevent_alloc(enum trace_dynevent_type type, const char *system,
> > + const char *event, const char *address, const char *format);
> > +void dynevent_free(struct tracefs_dynevent *devent);
> > +int dynevent_create(struct tracefs_dynevent *devent);
> > +int dynevent_destroy(struct tracefs_dynevent *devent);
> > +int dynevent_get_all(unsigned long type_mask, const char *system,
> > + struct tracefs_dynevent ***ret_events);
>
> I usually prefer to have a list returned, and NULL on error, instead of
> passing it in by reference. The "***" gets a bit out of hand.
>
The ret_events argument is optional, that's why it is passed as a
parameter. If ret_events is NULL, the API will return only the count
of the requested events. No array with events will be allocated in
that case.
>
> > +
> > #endif /* _TRACE_FS_LOCAL_H */
> > diff --git a/include/tracefs.h b/include/tracefs.h
> > index a2cda30..4020551 100644
[ ... ]
> > +
> > +static int dyn_generic_parse(struct dyn_events_desc *desc, const char *group,
> > + char *line, struct tracefs_dynevent **ret_dyn)
>
> Again, I prefer to have the pointer returned, and not assigned as a
> parameter. All this returns is 0 on success or -1 on failure. That doesn't
> give us anything more than having ret_dyn allocated or not. (return NULL or
> non-NULL).
>
The ret_dyn is optional. If it is NULL, the function only checks if
the line contains an event of the requested type, without allocating
any memory.
> > +{
> > + struct tracefs_dynevent *dyn;
> > + char *address;
> > + char *system;
> > + char *format;
> > + char *event;
> > +
> > + if (strncmp(line, desc->prefix, strlen(desc->prefix)))
> > + return -1;
> > +
> > + system = strchr(line, ':');
> > + if (!system)
> > + return -1;
> > + event = strchr(line, '/');
> > + if (!event)
> > + return -1;
> > + address = strchr(line, ' ');
> > + if (!address)
> > + return -1;
> > + format = strchr(address+1, ' ');
> > +
> > + *address = '\0';
> > + *event = '\0';
>
>
> The above should use strtok_r.
>
> char *probe;
> char *sav;
>
> probe = strtok_r(line, ":", &sav);
> system = strtok_r(NULL, "/", &sav);
> event = strtok_r(NULL, " ", &sav);
> address = strtok_r(NULL, " ", &sav);
> format = strtok_r(NULL, "\n", &sav);
>
I used strchr() instead of strtok_r() here, because my original idea
was that this function should not modify the original line string, if
the event is not of the requested type. When parsing the
"dynamic_events" file, it contains events from any types. My initial
design was to pass the same line string to all parse functions, until
there is a match. But the design was changed, now a new copy of the
file is used for each event type. I'll change that code to use
strtok_r().
>
>
>
> > +
> > + /* KPROBEs and UPROBEs share the same prefix, check the format */
> > + if (desc->type == TRACE_DYNEVENT_UPROBE || desc->type == TRACE_DYNEVENT_URETPROBE) {
> > + if (!strchr(format, '/'))
> > + return -1;
> > + }
[ ... ]
> > +
> > +static int get_all_type(enum trace_dynevent_type type, const char *system,
> > + struct tracefs_dynevent ***ret_all)
> > +{
> > + struct dyn_events_desc *desc;
> > + struct tracefs_dynevent *devent, **tmp, **all = NULL;
> > + char *content;
> > + int count = 0;
> > + char *line;
> > + char *next;
> > + int ret;
> > +
> > + desc = get_devent_desc(type);
> > + if (!desc)
> > + return -1;
> > +
> > + content = tracefs_instance_file_read(NULL, desc->file, NULL);
> > + if (!content)
> > + return -1;
>
> space.
>
> > + line = content;
> > + do {
> > + next = strchr(line, '\n');
> > + if (next)
> > + *next = '\0';
> > + ret = desc->dyn_events_parse(desc, system, line, ret_all ? &devent : NULL);
>
> So if ret_all is NULL, then we allocate a bunch of devent and lose them?
>
Hm, no devents are allocated if ret_all is NULL. That's the idea of
that API design - ret_all is an optional input parameter. Without it,
the functions can be used only to get the count of the requested
events, without any memory allocation. I would like to keep that
functionality, even though it is not used at the moment.
> > + if (!ret) {
> > + if (ret_all) {
> > + tmp = realloc(all, (count+1)*sizeof(struct tracefs_dynevent *));
> > + if (!tmp)
> > + goto error;
> > + all = tmp;
> > + all[count] = devent;
> > + }
> > + count++;
> > + }
> > + line = next + 1;
> > + } while (next);
> > +
> > + free(content);
> > + if (ret_all)
> > + *ret_all = all;
>
> We really should just return all, and if we want a count, we can pass a
> pointer to an integer, and put the count in there. Not the other way around.
>
> > + return count;
> > +
> > +error:
> > + free(content);
> > + free(all);
> > + return -1;
> > +}
> > +
> > +/**
> > + * tracefs_dynevent_list_free - Deletes an array of pointers to dynamic event contexts
> > + * @events: An array of pointers to dynamic event contexts. The last element of the array
> > + * must be a NULL pointer.
> > + */
> > +void tracefs_dynevent_list_free(struct tracefs_dynevent ***events)
>
> Again, just pass in ** not ***.
>
> > +{
> > + struct tracefs_dynevent **devent;
> > + int i = 0;
> > +
> > + if (!events || !(*events))
> > + return;
> > +
> > + devent = *events;
> > + while (devent[i])
> > + dynevent_free(devent[i++]);
> > +
> > + free(*events);
> > +}
> > +
> > +__hidden int dynevent_get_all(unsigned long type_mask, const char *system,
> > + struct tracefs_dynevent ***ret_events)
> > +{
> > + struct tracefs_dynevent **events, **tmp, **all_events = NULL;
> > + int count, all = 0;
> > + int i;
> > +
> > + for (i = 0; i < TRACE_DYNEVENT_MAX; i++) {
> > + if (!DYNEVENT_CHECK_BIT(type_mask, i))
> > + continue;
>
> space.
>
> > + count = get_all_type(i, system, ret_events ? &events : NULL);
> > + if (count > 0) {
> > + if (ret_events) {
> > + tmp = realloc(all_events,
> > + (all + count)*sizeof(struct tracefs_dynevent *));
> > + if (!tmp)
> > + goto error;
> > + all_events = tmp;
> > + memcpy(all_events + all, events,
> > + count*sizeof(struct tracefs_dynevent *));
> > + }
> > + all += count;
> > + }
> > +
> > + }
> > +
> > + if (ret_events) {
> > + /* Add a NULL pointer at the end */
> > + if (all > 0) {
> > + tmp = realloc(all_events,
> > + (all + 1)*sizeof(struct tracefs_dynevent *));
> > + if (!tmp)
> > + goto error;
> > + all_events = tmp;
> > + all_events[all] = NULL;
> > + }
> > + *ret_events = all_events;
> > + }
> > +
> > + return all;
> > +
> > +error:
> > + if (all_events) {
> > + for (i = 0; i < all; i++)
> > + free(all_events[i]);
> > + free(all_events);
> > + }
> > + return -1;
> > +}
>
>
> -- Steve
Thanks Steven! I'll address these comments in the next version, but I
really want to keep the dynevent_get_all() API more flexible - to
have a way to get only the count, without allocating an array with the
events.
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|