On Thu, Feb 10, 2022, at 16:00, Ferruh Yigit wrote: > On 2/10/2022 7:10 AM, madhuker.myt...@oracle.com wrote: >> From: Madhuker Mythri <madhuker.myt...@oracle.com> >> >> Failsafe pmd started crashing with global devargs syntax as devargs is >> not memset to zero. Access it to in rte_devargs_parse resulted in a >> crash when called from secondary process. >> >> Bugzilla Id: 933 >> >> Signed-off-by: Madhuker Mythri <madhuker.myt...@oracle.com> >> --- >> drivers/net/failsafe/failsafe.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/failsafe/failsafe.c >> b/drivers/net/failsafe/failsafe.c >> index 3c754a5f66..aa93cc6000 100644 >> --- a/drivers/net/failsafe/failsafe.c >> +++ b/drivers/net/failsafe/failsafe.c >> @@ -360,6 +360,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev) >> if (sdev->devargs.name[0] == '\0') >> continue; >> >> + memset(&devargs, 0, sizeof(devargs)); >> /* rebuild devargs to be able to get the bus name. */ >> ret = rte_devargs_parse(&devargs, >> sdev->devargs.name); > > if 'rte_devargs_parse()' requires 'devargs' parameter to be memset, > what do you think memset it in the API? > This prevents forgotten cases like this.
Hi, I was looking at it this morning. Before the last release, rte_devargs_parse() was only supporting legacy syntax. It never read from the devargs structure, only wrote to it. So it was safe to use with a non-memset devargs. The rte_devargs_layer_parse() however is more complex. To allow rte_dev_iterator_init() to call it without doing memory allocation, it reads parts of the devargs to make decisions. Doing a first call to rte_devargs_layer_parse() as part of rte_devargs_parse() thus modified the contract it had with the users, that it would never read from devargs. It is not possible to completely avoid reading from devargs in rte_devargs_layer_parse(). It is necessary for RTE_DEV_FOREACH() to be safe to interrupt without having to do iterator cleanup. This is my current understanding. In that context, yes I think it is preferrable to do memset() within rte_devargs_parse(). It will restore the previous part of the API saying that calling it with non-memset devargs was safe to do. Thanks, -- Gaetan Rivet
