On Fri, 29 Oct 2021 05:46:42 +0300
Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
> 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.
>
I would make a separate API for just the count, and not have this
function server two functionalities.
> >
> > > +
> > > #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.
I think we should make a separate API for that as well.
dyn_generic_parse_check()?
>
> > > +{
> > > + 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.
Ah. But still, this is overly complex to try to have this server two
functionalities. It becomes confusing.
>
> 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.
I really think you should have that as two separate functions,
otherwise it becomes overly complex.
-- Steve
|