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

[Alison Schofield <alison.schofield@intel.com>]

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!