On Wed, Dec 06, 2023 at 08:39:07PM -0800, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <[email protected]>
> >
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
