diff mbox series

[ndctl,v10,4/7] cxl/event_trace: add helpers to retrieve tep fields by type

Message ID 3d264f1fe4c92a90eabf9cd3365a2dc69caacc4e.1709748564.git.alison.schofield@intel.com (mailing list archive)
State Superseded
Headers show
Series Support poison list retrieval | expand

Commit Message

Alison Schofield March 6, 2024, 6:42 p.m. UTC 
From: Alison Schofield <alison.schofield@intel.com>

Add helpers to extract the value of an event record field given the
field name. This is useful when the user knows the name and format
of the field and simply needs to get it.

Since this is in preparation for adding a cxl_poison private parser
for 'cxl list --media-errors' support, add those specific required
types: u8, u32, u64, char*

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 cxl/event_trace.h | 10 ++++++-
 2 files changed, 84 insertions(+), 1 deletion(-)

Comments

Dan Williams March 6, 2024, 11:53 p.m. UTC | #1 
alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Add helpers to extract the value of an event record field given the
> field name. This is useful when the user knows the name and format
> of the field and simply needs to get it.
> 
> Since this is in preparation for adding a cxl_poison private parser
> for 'cxl list --media-errors' support, add those specific required
> types: u8, u32, u64, char*
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/event_trace.h | 10 ++++++-
>  2 files changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index bdad0c19dbd4..6cc9444f3204 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -15,6 +15,81 @@
>  #define _GNU_SOURCE
>  #include <string.h>
>  
> +static struct tep_format_field *__find_field(struct tep_event *event,
> +					     const char *name)
> +{
> +	struct tep_format_field **fields;
> +
> +	fields = tep_event_fields(event);
> +	if (!fields)
> +		return NULL;
> +
> +	for (int i = 0; fields[i]; i++) {
> +		struct tep_format_field *f = fields[i];
> +
> +		if (strcmp(f->name, name) != 0)
> +			continue;
> +
> +		return f;
> +	}
> +	return NULL;
> +}

Is this open-coded tep_find_field()?

> +
> +u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
> +		      const char *name)
> +{
> +	struct tep_format_field *f;
> +	unsigned char *val;
> +	int len;
> +
> +	f = __find_field(event, name);
> +	if (!f)
> +		return ULLONG_MAX;
> +
> +	val = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +	if (!val)
> +		return ULLONG_MAX;
> +
> +	return *(u64 *)val;
> +}

Is this just open-coded tep_get_any_field_val()?

> +
> +char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
> +			   const char *name)

Return a 'const char *'?

> +{
> +	struct tep_format_field *f;
> +	int len;
> +
> +	f = __find_field(event, name);
> +	if (!f)
> +		return NULL;
> +
> +	return tep_get_field_raw(NULL, event, f->name, record, &len, 0);

Is this guaranteed to be a string? ...and guaranteed to be NULL
terminated?
Alison Schofield March 8, 2024, 4:06 a.m. UTC | #2 
On Wed, Mar 06, 2024 at 03:53:57PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Add helpers to extract the value of an event record field given the
> > field name. This is useful when the user knows the name and format
> > of the field and simply needs to get it.
> > 
> > Since this is in preparation for adding a cxl_poison private parser
> > for 'cxl list --media-errors' support, add those specific required
> > types: u8, u32, u64, char*
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  cxl/event_trace.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++
> >  cxl/event_trace.h | 10 ++++++-
> >  2 files changed, 84 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> > index bdad0c19dbd4..6cc9444f3204 100644
> > --- a/cxl/event_trace.c
> > +++ b/cxl/event_trace.c
> > @@ -15,6 +15,81 @@
> >  #define _GNU_SOURCE
> >  #include <string.h>
> >  
> > +static struct tep_format_field *__find_field(struct tep_event *event,
> > +					     const char *name)
> > +{
> > +	struct tep_format_field **fields;
> > +
> > +	fields = tep_event_fields(event);
> > +	if (!fields)
> > +		return NULL;
> > +
> > +	for (int i = 0; fields[i]; i++) {
> > +		struct tep_format_field *f = fields[i];
> > +
> > +		if (strcmp(f->name, name) != 0)
> > +			continue;
> > +
> > +		return f;
> > +	}
> > +	return NULL;
> > +}
> 
> Is this open-coded tep_find_field()?

Yes it is and now it is gone.

> 
> > +
> > +u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
> > +		      const char *name)
> > +{
> > +	struct tep_format_field *f;
> > +	unsigned char *val;
> > +	int len;
> > +
> > +	f = __find_field(event, name);
> > +	if (!f)
> > +		return ULLONG_MAX;
> > +
> > +	val = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> > +	if (!val)
> > +		return ULLONG_MAX;
> > +
> > +	return *(u64 *)val;
> > +}
> 
> Is this just open-coded tep_get_any_field_val()?

It's a bit more. It returns ULLONG_MAX and casts to the u64 which
makes the call site cleaner. 

I did change it to use tep_get_field_val(). Please look at next rev.

> 
> > +
> > +char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
> > +			   const char *name)
> 
> Return a 'const char *'?
> 
> > +{
> > +	struct tep_format_field *f;
> > +	int len;
> > +
> > +	f = __find_field(event, name);
> > +	if (!f)
> > +		return NULL;
> > +
> > +	return tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> 
> Is this guaranteed to be a string? ...and guaranteed to be NULL
> terminated?
>
It's gone. Using tep_get_field_raw() directly for str.

Thanks for reviewing!
diff mbox series

Patch

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index bdad0c19dbd4..6cc9444f3204 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -15,6 +15,81 @@ 
 #define _GNU_SOURCE
 #include <string.h>
 
+static struct tep_format_field *__find_field(struct tep_event *event,
+					     const char *name)
+{
+	struct tep_format_field **fields;
+
+	fields = tep_event_fields(event);
+	if (!fields)
+		return NULL;
+
+	for (int i = 0; fields[i]; i++) {
+		struct tep_format_field *f = fields[i];
+
+		if (strcmp(f->name, name) != 0)
+			continue;
+
+		return f;
+	}
+	return NULL;
+}
+
+u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
+		      const char *name)
+{
+	struct tep_format_field *f;
+	unsigned char *val;
+	int len;
+
+	f = __find_field(event, name);
+	if (!f)
+		return ULLONG_MAX;
+
+	val = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+	if (!val)
+		return ULLONG_MAX;
+
+	return *(u64 *)val;
+}
+
+char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
+			   const char *name)
+{
+	struct tep_format_field *f;
+	int len;
+
+	f = __find_field(event, name);
+	if (!f)
+		return NULL;
+
+	return tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+}
+
+u32 cxl_get_field_u32(struct tep_event *event, struct tep_record *record,
+		      const char *name)
+{
+	char *val;
+
+	val = cxl_get_field_string(event, record, name);
+	if (!val)
+		return UINT_MAX;
+
+	return *(u32 *)val;
+}
+
+u8 cxl_get_field_u8(struct tep_event *event, struct tep_record *record,
+		    const char *name)
+{
+	char *val;
+
+	val = cxl_get_field_string(event, record, name);
+	if (!val)
+		return UCHAR_MAX;
+
+	return *(u8 *)val;
+}
+
 static struct json_object *num_to_json(void *num, int elem_size, unsigned long flags)
 {
 	bool sign = flags & TEP_FIELD_IS_SIGNED;
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index ec61962abbc6..bbdea3b896e0 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -5,6 +5,7 @@ 
 
 #include <json-c/json.h>
 #include <ccan/list/list.h>
+#include <ccan/short_types/short_types.h>
 
 struct jlist_node {
 	struct json_object *jobj;
@@ -25,5 +26,12 @@  int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
 int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system,
 		const char *event);
 int cxl_event_tracing_disable(struct tracefs_instance *inst);
-
+char *cxl_get_field_string(struct tep_event *event, struct tep_record *record,
+			   const char *name);
+u8 cxl_get_field_u8(struct tep_event *event, struct tep_record *record,
+		    const char *name);
+u32 cxl_get_field_u32(struct tep_event *event, struct tep_record *record,
+		      const char *name);
+u64 cxl_get_field_u64(struct tep_event *event, struct tep_record *record,
+		      const char *name);
 #endif