Re: [ndctl PATCH v5 3/5] cxl/list: collect and parse the poison list records

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

On Wed, Dec 06, 2023 at 08:39:07PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 

snip

> > +	struct json_object *jerrors, *jpoison, *jobj = NULL;
> > +	struct jlist_node *jnode, *next;
> > +	struct event_ctx ectx = {
> > +		.event_name = "cxl_poison",
> > +		.event_pid = getpid(),
> > +		.system = "cxl",
> > +	};
> > +	int rc, count = 0;
> > +
> > +	list_head_init(&ectx.jlist_head);
> > +	rc = cxl_parse_events(inst, &ectx);
> 
> This pattern really feels like it wants a cxl_for_each_event() -style
> helper rather than require the end caller to open code list usage.
> Basically cxl_parse_events() is a helper that should stay local to
> cxl/monitor.c. This new cxl_for_each_event() would become used
> internally by cxl_parse_events() and let
> util_cxl_poison_events_to_json() do its own per-event iteration.
> 

snip & concat'ing your next comment:

> So we're building a json_object internal to cxl_parse_events() only to
> turn around and extract details out of that object that tell us this
> event was not of interest, or to create yet another json object?
> 
> I think this implementation has a chance to be significantly less
> complicated if the event list can be iterated directly without this
> temporary json_object parsing.

DaveJ actually already implemented a method to include a 'private'
parsing function in the event_ctx structure. I didn't use it, but
but rather used the generic cxl_event_to_json helper, and then
parsed that all over again to refine for poison list output.

I think reorganizing w a private event_ctx->parse_event will
streamline as you suggest.

Alison