Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Monday, November 5, 2018 8:31 AM
> To: Stojaczyk, Dariusz <dariusz.stojac...@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 2/3] devargs: delay freeing previous devargs when
> overriding them
>
> Hi,
>
> 05/11/2018 08:04, Darek Stojaczyk:
> > -int __rte_experimental
> > -rte_devargs_insert(struct rte_devargs *da)
> > +void __rte_experimental
> > +rte_devargs_insert(struct rte_devargs *da, struct rte_devargs
> **prev_da)
>
> You should update the API section of the release notes.
Even for experimental API? OK, I didn't know it's needed.
>
> > {
> > - int ret;
> > + struct rte_devargs *d;
> > + void *tmp;
> > +
> > + *prev_da = NULL;
> > + TAILQ_FOREACH_SAFE(d, &devargs_list, next, tmp) {
> > + if (strcmp(d->bus->name, da->bus->name) == 0 &&
> > + strcmp(d->name, da->name) == 0) {
> > + TAILQ_REMOVE(&devargs_list, d, next);
> > + *prev_da = d;
> > + break;
> > + }
> > + }
> >
> > - ret = rte_devargs_remove(da);
> > - if (ret < 0)
> > - return ret;
>
> Why not updating rte_devargs_remove instead of duplicating its code?
>
We still want to preserve the functionality of rte_devargs_remove.
rte_devargs_remove does TAILQ_REMOVE + free; rte_devargs_insert does just
TAILQ_REMOVE. (I think I also forgot to update rte_devargs_insert
documentation, I'll do that in V2)
Since you've mentioned it:
Eventually I'd see rte_devargs_remove to accept the exact same devargs
parameter that was passed to rte_devargs_insert. Then rte_devargs_remove
wouldn't do any sort of lookup. Maybe additional rte_devargs_find(const char
*name) could be added for existing cases where the original devargs struct is
not available. However, I'm not familiar enough with this code to perform the
refactor and am just trying to fix the stuff. Still, how does it sound?
D.
