diff mbox series

[v4,04/10] libtracefs: Change tracefs_kprobe_info API

Message ID 20211104111047.302660-5-tz.stoyanov@gmail.com (mailing list archive)
State Superseded
Headers show
Series libtracefs dynamic events support | expand

Commit Message

Tzvetomir Stoyanov (VMware) Nov. 4, 2021, 11:10 a.m. UTC
In order to make kprobe APIs consistent with the other libtracefs APIs,
the tracefs_kprobe_info() API is reimplemented and changed to work with
the library dynamic event structure.

Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 include/tracefs.h     |  11 +--
 src/tracefs-kprobes.c | 159 ++++++++++++------------------------------
 2 files changed, 48 insertions(+), 122 deletions(-)

Comments

Steven Rostedt Nov. 4, 2021, 4:33 p.m. UTC | #1
On Thu,  4 Nov 2021 13:10:41 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> +enum tracefs_dynevent_type tracefs_kprobe_info(struct tracefs_dynevent *kprobe,
> +					       char **system, char **event,
> +					       char **prefix, char **addr, char **format)
> +{
> +	char **lv[] = { system, event, prefix, addr, format };
> +	char **rv[] = { &kprobe->system, &kprobe->event, &kprobe->prefix,
> +			&kprobe->address, &kprobe->format };
> +	int i;
> +
> +	if (!kprobe)
> +		return TRACEFS_DYNEVENT_MAX;
> +
> +	for (i = 0; i < ARRAY_SIZE(lv); i++)
> +		*lv[i] = NULL;

Do we really need to initialize them to NULL here?

Not to mention, if one of the parameters is NULL itself, this will SEGFAULT.

> +
> +	for (i = 0; i < ARRAY_SIZE(lv); i++) {

Nice use of ARRAY_SIZE() ;-)

> +		if (lv[i]) {
> +			if (*rv[i]) {
> +				*lv[i] = strdup(*rv[i]);
> +				if (!*lv[i])
> +					goto error;
> +			} else {
> +				*lv[i] = NULL;
>  			}

The above here sets it to NULL if the parameter is non NULL.

We don't need the initial NULL setting loop.

-- Steve


> -			break;
>  		}
> -		ret = parse_kprobe(NULL, &saveptr, &ktype, &system, &probe,
> -				   &kaddr, &kfmt);
>  	}
> -	free(content);
> -	return rtype;
> +
> +	return kprobe->type;
> +
> +error:
> +	for (i--; i >= 0; i--) {
> +		if (lv[i])
> +			free(*lv[i]);
> +	}
> +
> +	return TRACEFS_DYNEVENT_MAX;
>  }
Tzvetomir Stoyanov (VMware) Nov. 4, 2021, 5:28 p.m. UTC | #2
On Thu, Nov 4, 2021 at 6:33 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu,  4 Nov 2021 13:10:41 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > +enum tracefs_dynevent_type tracefs_kprobe_info(struct tracefs_dynevent *kprobe,
> > +                                            char **system, char **event,
> > +                                            char **prefix, char **addr, char **format)
> > +{
> > +     char **lv[] = { system, event, prefix, addr, format };
> > +     char **rv[] = { &kprobe->system, &kprobe->event, &kprobe->prefix,
> > +                     &kprobe->address, &kprobe->format };
> > +     int i;
> > +
> > +     if (!kprobe)
> > +             return TRACEFS_DYNEVENT_MAX;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(lv); i++)
> > +             *lv[i] = NULL;
>
> Do we really need to initialize them to NULL here?
>
> Not to mention, if one of the parameters is NULL itself, this will SEGFAULT.
>

I'll add a check for that. They should be set to NULL to ensure that
free() in the error path will not segfault, if the caller passes not
initialised pointers.

> > +
> > +     for (i = 0; i < ARRAY_SIZE(lv); i++) {
>
> Nice use of ARRAY_SIZE() ;-)
>
> > +             if (lv[i]) {
> > +                     if (*rv[i]) {
> > +                             *lv[i] = strdup(*rv[i]);
> > +                             if (!*lv[i])
> > +                                     goto error;
> > +                     } else {
> > +                             *lv[i] = NULL;
> >                       }
>
> The above here sets it to NULL if the parameter is non NULL.
>
> We don't need the initial NULL setting loop.
>
> -- Steve
>
>
> > -                     break;
> >               }
> > -             ret = parse_kprobe(NULL, &saveptr, &ktype, &system, &probe,
> > -                                &kaddr, &kfmt);
> >       }
> > -     free(content);
> > -     return rtype;
> > +
> > +     return kprobe->type;
> > +
> > +error:
> > +     for (i--; i >= 0; i--) {
> > +             if (lv[i])
> > +                     free(*lv[i]);
> > +     }
> > +
> > +     return TRACEFS_DYNEVENT_MAX;
> >  }
>
Steven Rostedt Nov. 4, 2021, 5:57 p.m. UTC | #3
On Thu, 4 Nov 2021 19:28:49 +0200
Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:

> On Thu, Nov 4, 2021 at 6:33 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Thu,  4 Nov 2021 13:10:41 +0200
> > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> >  
> > > +enum tracefs_dynevent_type tracefs_kprobe_info(struct tracefs_dynevent *kprobe,
> > > +                                            char **system, char **event,
> > > +                                            char **prefix, char **addr, char **format)
> > > +{
> > > +     char **lv[] = { system, event, prefix, addr, format };
> > > +     char **rv[] = { &kprobe->system, &kprobe->event, &kprobe->prefix,
> > > +                     &kprobe->address, &kprobe->format };
> > > +     int i;
> > > +
> > > +     if (!kprobe)
> > > +             return TRACEFS_DYNEVENT_MAX;
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(lv); i++)
> > > +             *lv[i] = NULL;  
> >
> > Do we really need to initialize them to NULL here?
> >
> > Not to mention, if one of the parameters is NULL itself, this will SEGFAULT.
> >  
> 
> I'll add a check for that. They should be set to NULL to ensure that
> free() in the error path will not segfault, if the caller passes not
> initialised pointers.

It wont, due to the fact that it starts at i and works backward down to
zero. The only items it will look at are the ones that were already
initialized. Which is why I did it that way.

> 
> > > +
> > > +     for (i = 0; i < ARRAY_SIZE(lv); i++) {  
> >
> > Nice use of ARRAY_SIZE() ;-)
> >  
> > > +             if (lv[i]) {
> > > +                     if (*rv[i]) {
> > > +                             *lv[i] = strdup(*rv[i]);
> > > +                             if (!*lv[i])
> > > +                                     goto error;
> > > +                     } else {
> > > +                             *lv[i] = NULL;
> > >                       }  

> > > +
> > > +     return kprobe->type;
> > > +
> > > +error:
> > > +     for (i--; i >= 0; i--) {

The above means that we do not need to initialize anything that wasn't
touched.

-- Steve


> > > +             if (lv[i])
> > > +                     free(*lv[i]);
> > > +     }
> > > +
> > > +     return TRACEFS_DYNEVENT_MAX;
> > >  }  
> >  
> 
>
diff mbox series

Patch

diff --git a/include/tracefs.h b/include/tracefs.h
index 7d9a9af..8377ade 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -257,12 +257,6 @@  void tracefs_dynevent_list_free(struct tracefs_dynevent **events);
 struct tracefs_dynevent **
 tracefs_dynevent_get_all(enum tracefs_dynevent_type types, const char *system);
 
-enum tracefs_kprobe_type {
-	TRACEFS_ALL_KPROBES,
-	TRACEFS_KPROBE,
-	TRACEFS_KRETPROBE,
-};
-
 struct tracefs_dynevent *
 tracefs_kprobe_alloc(const char *system, const char *event, const char *addr, const char *format);
 struct tracefs_dynevent *
@@ -272,8 +266,9 @@  int tracefs_kprobe_raw(const char *system, const char *event,
 		       const char *addr, const char *format);
 int tracefs_kretprobe_raw(const char *system, const char *event,
 			  const char *addr, const char *format);
-enum tracefs_kprobe_type tracefs_kprobe_info(const char *group, const char *event,
-					     char **type, char **addr, char **format);
+enum tracefs_dynevent_type tracefs_kprobe_info(struct tracefs_dynevent *kprobe,
+					       char **system, char **event,
+					       char **prefix, char **addr, char **format);
 
 enum tracefs_hist_key_type {
 	TRACEFS_HIST_KEY_NORMAL = 0,
diff --git a/src/tracefs-kprobes.c b/src/tracefs-kprobes.c
index f67a93a..a391637 100644
--- a/src/tracefs-kprobes.c
+++ b/src/tracefs-kprobes.c
@@ -214,125 +214,56 @@  int tracefs_kretprobe_raw(const char *system, const char *event,
 	return insert_kprobe("r", system, event, addr, format);
 }
 
-/*
- * Helper function to parse kprobes.
- * @content: The content of kprobe_events on the first iteration.
- *           NULL on next iterations.
- * @saveptr: Same as saveptr for strtok_r
- * @type: Where to store the type (before ':')
- * @system: Store the system of the kprobe (NULL to have event contain
- *          both system and event, as in "kprobes/myprobe").
- * @event: Where to store the event.
- * @addr: Where to store the addr (may be NULL to ignore)
- * @format: Where to store the format (may be NULL to ignore)
- */
-static int parse_kprobe(char *content, char **saveptr,
-			char **type, char **system, char **event,
-			char **addr, char **format)
-{
-	char *p;
-
-	p = strtok_r(content, ":", saveptr);
-	if (!p)
-		return 1; /* eof */
-	*type = p;
-
-	if (system) {
-		p = strtok_r(NULL, "/", saveptr);
-		if (!p)
-			return -1;
-		*system = p;
-	}
-
-	p = strtok_r(NULL, " ", saveptr);
-	if (!p)
-		return -1;
-	*event = p;
-
-	if (addr || format) {
-		p = strtok_r(NULL, " ", saveptr);
-		if (!p)
-			return -1;
-		if (addr)
-			*addr = p;
-	}
-
-	p = strtok_r(NULL, "\n", saveptr);
-	if (!p)
-		return -1;
-	if (format)
-		*format = p;
-
-	return 0;
-}
-
 /**
- * tracefs_kprobe_info - return the type of kprobe specified.
- * @group: The group the kprobe is in (NULL for the default "kprobes")
- * @event: The name of the kprobe to find.
- * @type: String to return kprobe type (before ':') NULL to ignore.
- * @addr: String to return address kprobe is attached to. NULL to ignore.
- * @format: String to return kprobe format. NULL to ignore.
- *
- * If @type, @addr, or @format is non NULL, then the returned string
- * must be freed with free(). They will also be set to NULL, and
- * even on error, they may contain strings to be freed. If they are
- * not NULL, then they still need to be freed.
+ * tracefs_kprobe_info - return details of a kprobe
+ * @kprobe: A kprobe context, describing given kprobe.
+ * @group: return, group in which the kprobe is configured
+ * @event: return, name of the kprobe event
+ * @prefix: return, prefix string of the kprobe
+ *	    for kretprobes, the maxactive count is encoded in the prefix
+ * @addr: return, the function and offset (or address) of the kprobe
+ * @format: return, the format string of the kprobe
  *
- * Returns TRACEFS_ALL_KPROBES if an error occurs or the kprobe is not found,
- *            or the probe is of an unknown type.
- * TRACEFS_KPROBE if the type of kprobe found is a normal kprobe.
- * TRACEFS_KRETPROBE if the type of kprobe found is a kretprobe.
+ * Returns the type of the kprobe, or TRACEFS_DYNEVENT_MAX in case of an error.
+ * Any of the @group, @event, @prefix, @addr and @format parameters are optional.
+ * If a valid pointer is passed, in case of success - a string is allocated and returned.
+ * These strings must be freed with free().
  */
-enum tracefs_kprobe_type tracefs_kprobe_info(const char *group, const char *event,
-					     char **type, char **addr, char **format)
-{
-	enum tracefs_kprobe_type rtype = TRACEFS_ALL_KPROBES;
-	char *saveptr;
-	char *content;
-	char *system;
-	char *probe;
-	char *ktype;
-	char *kaddr;
-	char *kfmt;
-	int ret;
-
-	if (!group)
-		group = KPROBE_DEFAULT_GROUP;
-
-	if (type)
-		*type = NULL;
-	if (addr)
-		*addr = NULL;
-	if (format)
-		*format = NULL;
-
-	content = tracefs_instance_file_read(NULL, KPROBE_EVENTS, NULL);
-	if (!content)
-		return rtype;
-
-	ret = parse_kprobe(content, &saveptr, &ktype, &system, &probe,
-			   &kaddr, &kfmt);
-
-	while (!ret) {
-
-		if (!strcmp(system, group) && !strcmp(probe, event)) {
-			if (type)
-				*type = strdup(ktype);
-			if (addr)
-				*addr = strdup(kaddr);
-			if (format)
-				*format = strdup(kfmt);
 
-			switch (*ktype) {
-			case 'p': rtype = TRACEFS_KPROBE; break;
-			case 'r': rtype = TRACEFS_KRETPROBE; break;
+enum tracefs_dynevent_type tracefs_kprobe_info(struct tracefs_dynevent *kprobe,
+					       char **system, char **event,
+					       char **prefix, char **addr, char **format)
+{
+	char **lv[] = { system, event, prefix, addr, format };
+	char **rv[] = { &kprobe->system, &kprobe->event, &kprobe->prefix,
+			&kprobe->address, &kprobe->format };
+	int i;
+
+	if (!kprobe)
+		return TRACEFS_DYNEVENT_MAX;
+
+	for (i = 0; i < ARRAY_SIZE(lv); i++)
+		*lv[i] = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(lv); i++) {
+		if (lv[i]) {
+			if (*rv[i]) {
+				*lv[i] = strdup(*rv[i]);
+				if (!*lv[i])
+					goto error;
+			} else {
+				*lv[i] = NULL;
 			}
-			break;
 		}
-		ret = parse_kprobe(NULL, &saveptr, &ktype, &system, &probe,
-				   &kaddr, &kfmt);
 	}
-	free(content);
-	return rtype;
+
+	return kprobe->type;
+
+error:
+	for (i--; i >= 0; i--) {
+		if (lv[i])
+			free(*lv[i]);
+	}
+
+	return TRACEFS_DYNEVENT_MAX;
 }