Hi, > -----Original Message----- > From: David Marchand <[email protected]> > Sent: Thursday, November 17, 2022 4:32 PM > To: Suanming Mou <[email protected]> > Cc: Ori Kam <[email protected]>; Aman Singh <[email protected]>; > Yuying Zhang <[email protected]>; NBU-Contact-Thomas Monjalon > (EXTERNAL) <[email protected]>; Ferruh Yigit <[email protected]>; > Andrew Rybchenko <[email protected]>; Ray Kinsella > <[email protected]>; [email protected] > Subject: Re: [PATCH v1] ethdev: add indirect action async query > > On Thu, Nov 17, 2022 at 9:18 AM Suanming Mou <[email protected]> > wrote: > > > > Hi, > > > > > -----Original Message----- > > > From: David Marchand <[email protected]> > > > Sent: Thursday, November 17, 2022 4:07 PM > > > To: Suanming Mou <[email protected]> > > > Cc: Ori Kam <[email protected]>; Aman Singh > > > <[email protected]>; Yuying Zhang <[email protected]>; > > > NBU-Contact-Thomas Monjalon > > > (EXTERNAL) <[email protected]>; Ferruh Yigit > > > <[email protected]>; Andrew Rybchenko > > > <[email protected]>; Ray Kinsella <[email protected]>; > > > [email protected] > > > Subject: Re: [PATCH v1] ethdev: add indirect action async query > > > > > > Hello, > > > > > > On Tue, Sep 20, 2022 at 9:12 AM Suanming Mou <[email protected]> > > > wrote: > > > > @@ -2873,17 +2907,23 @@ port_queue_action_handle_destroy(portid_t > > > port_id, > > > > * of error. > > > > */ > > > > memset(&error, 0x99, sizeof(error)); > > > > + job = calloc(1, sizeof(*job)); > > > > + if (!job) { > > > > + printf("Queue action destroy job > > > > allocate failed\n"); > > > > + return -ENOMEM; > > > > + } > > > > + job->type = QUEUE_JOB_TYPE_ACTION_DESTROY; > > > > + job->pia = pia; > > > > > > > > if (pia->handle && > > > > > > > > rte_flow_async_action_handle_destroy(port_id, > > > > - queue_id, &attr, pia->handle, NULL, > > > > &error)) { > > > > + queue_id, &attr, pia->handle, job, > > > > + &error)) { > > > > ret = port_flow_complain(&error); > > > > continue; > > > > } > > > > *tmp = pia->next; > > > > printf("Indirect action #%u destruction > > > > queued\n", > > > > pia->id); > > > > - free(pia); > > > > break; > > > > } > > > > if (i == n) > > > > > > Our covscan tool reports a potential leak of "job" in this block. > > > I am unclear whether it is a normal occurence, but it seems that if > > > pia->handle == NULL, then job is leaked. > > > > OK, this function can only be called from destroying a created action > > handle. > For the created action handle, the pia->handle should never be NULL here. > > And we also have " if (actions[i] != pia->id) " several lines above to > > ensure it is > a valid pia. > > I agree from tools' point of view it looks like a leak here. But it should > > never > happen. > > Do you think we need a "fix" in that case? > > - If you are sure of it, unnecessary checks must be removed.
Sure, I will create a patch to remove that redundant check. > > - In pia->handle != NULL branch, won't "job" be leaked too if > rte_flow_async_action_handle_destroy() fails? Yes, you are right. Thanks, I will create a patch with the two changes. > > > -- > David Marchand
