[dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Stephen Hemminger]

When secondary process is run was noticing that the log always
contained complaints about unable to parse devargs.

It turns out that an empty devargs turns into "" and this
value is not parsable. Change the failsafe secondary to just
skip doing devargs if it empty.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/failsafe/failsafe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
index e91c274d8059..04ca0cab0d78 100644
--- a/drivers/net/failsafe/failsafe.c
+++ b/drivers/net/failsafe/failsafe.c
@@ -364,6 +364,10 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
 		 * A sub-device can be plugged later.
 		 */
 		FOREACH_SUBDEV(sdev, i, eth_dev) {
+			/* skip empty devargs */
+			if (sdev->devargs.name[0] == '\0')
+				continue;
+
 			/* rebuild devargs to be able to get the bus name. */
 			ret = rte_devargs_parse(&devargs,
 						sdev->devargs.name);
-- 
2.20.1

Re: [dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Gaëtan Rivet]

Hello Stephen,

On Fri, Jun 21, 2019 at 03:08:24PM -0700, Stephen Hemminger wrote:
> When secondary process is run was noticing that the log always
> contained complaints about unable to parse devargs.
> 
> It turns out that an empty devargs turns into "" and this
> value is not parsable. Change the failsafe secondary to just
> skip doing devargs if it empty.
> 

Commit log needs a little rework, a few typos.

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/failsafe/failsafe.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index e91c274d8059..04ca0cab0d78 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -364,6 +364,10 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
>  		 * A sub-device can be plugged later.
>  		 */
>  		FOREACH_SUBDEV(sdev, i, eth_dev) {
> +			/* skip empty devargs */
> +			if (sdev->devargs.name[0] == '\0')
> +				continue;
> +

An empty devargs being named "" is part of the internals of rte_devargs.
The clean solution would be to add a `bool rte_devargs_empty()` function
and test the devargs with it.

The simple solution is your proposition.

Clean seems a little heavy-handed, but it would be more stable. If you
agree, you can add the helper. I'm ok with keeping it simple otherwise.

>  			/* rebuild devargs to be able to get the bus name. */
>  			ret = rte_devargs_parse(&devargs,
>  						sdev->devargs.name);
> -- 
> 2.20.1
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Stephen Hemminger]

On Mon, 24 Jun 2019 10:15:58 +0200
Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:

> Hello Stephen,
> 
> On Fri, Jun 21, 2019 at 03:08:24PM -0700, Stephen Hemminger wrote:
> > When secondary process is run was noticing that the log always
> > contained complaints about unable to parse devargs.
> > 
> > It turns out that an empty devargs turns into "" and this
> > value is not parsable. Change the failsafe secondary to just
> > skip doing devargs if it empty.
> >   
> 
> Commit log needs a little rework, a few typos.
> 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  drivers/net/failsafe/failsafe.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> > index e91c274d8059..04ca0cab0d78 100644
> > --- a/drivers/net/failsafe/failsafe.c
> > +++ b/drivers/net/failsafe/failsafe.c
> > @@ -364,6 +364,10 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
> >  		 * A sub-device can be plugged later.
> >  		 */
> >  		FOREACH_SUBDEV(sdev, i, eth_dev) {
> > +			/* skip empty devargs */
> > +			if (sdev->devargs.name[0] == '\0')
> > +				continue;
> > +  
> 
> An empty devargs being named "" is part of the internals of rte_devargs.
> The clean solution would be to add a `bool rte_devargs_empty()` function
> and test the devargs with it.
> 
> The simple solution is your proposition.
> 
> Clean seems a little heavy-handed, but it would be more stable. If you
> agree, you can add the helper. I'm ok with keeping it simple otherwise.
> 
> >  			/* rebuild devargs to be able to get the bus name. */
> >  			ret = rte_devargs_parse(&devargs,
> >  						sdev->devargs.name);
> > -- 
> > 2.20.1
> >   
> 

Simpler is better.
Sorry, after working with failsafe my impression is that it is not
built with that in mind.

Re: [dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Gaëtan Rivet]

On Mon, Jun 24, 2019 at 08:23:38AM -0700, Stephen Hemminger wrote:
> On Mon, 24 Jun 2019 10:15:58 +0200
> Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> 
> > Hello Stephen,
> > 
> > On Fri, Jun 21, 2019 at 03:08:24PM -0700, Stephen Hemminger wrote:
> > > When secondary process is run was noticing that the log always
> > > contained complaints about unable to parse devargs.
> > > 
> > > It turns out that an empty devargs turns into "" and this
> > > value is not parsable. Change the failsafe secondary to just
> > > skip doing devargs if it empty.
> > >   
> > 
> > Commit log needs a little rework, a few typos.
> > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > >  drivers/net/failsafe/failsafe.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> > > index e91c274d8059..04ca0cab0d78 100644
> > > --- a/drivers/net/failsafe/failsafe.c
> > > +++ b/drivers/net/failsafe/failsafe.c
> > > @@ -364,6 +364,10 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
> > >  		 * A sub-device can be plugged later.
> > >  		 */
> > >  		FOREACH_SUBDEV(sdev, i, eth_dev) {
> > > +			/* skip empty devargs */
> > > +			if (sdev->devargs.name[0] == '\0')
> > > +				continue;
> > > +  
> > 
> > An empty devargs being named "" is part of the internals of rte_devargs.
> > The clean solution would be to add a `bool rte_devargs_empty()` function
> > and test the devargs with it.
> > 
> > The simple solution is your proposition.
> > 
> > Clean seems a little heavy-handed, but it would be more stable. If you
> > agree, you can add the helper. I'm ok with keeping it simple otherwise.
> > 
> > >  			/* rebuild devargs to be able to get the bus name. */
> > >  			ret = rte_devargs_parse(&devargs,
> > >  						sdev->devargs.name);
> > > -- 
> > > 2.20.1
> > >   
> > 
> 
> Simpler is better.

Ok

> Sorry, after working with failsafe my impression is that it is not
> built with that in mind.

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Ferruh Yigit]

On 6/24/2019 5:27 PM, Gaëtan Rivet wrote:
> On Mon, Jun 24, 2019 at 08:23:38AM -0700, Stephen Hemminger wrote:
>> On Mon, 24 Jun 2019 10:15:58 +0200
>> Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>>
>>> Hello Stephen,
>>>
>>> On Fri, Jun 21, 2019 at 03:08:24PM -0700, Stephen Hemminger wrote:
>>>> When secondary process is run was noticing that the log always
>>>> contained complaints about unable to parse devargs.
>>>>
>>>> It turns out that an empty devargs turns into "" and this
>>>> value is not parsable. Change the failsafe secondary to just
>>>> skip doing devargs if it empty.
>>>>   
>>>
>>> Commit log needs a little rework, a few typos.
>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> ---
>>>>  drivers/net/failsafe/failsafe.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
>>>> index e91c274d8059..04ca0cab0d78 100644
>>>> --- a/drivers/net/failsafe/failsafe.c
>>>> +++ b/drivers/net/failsafe/failsafe.c
>>>> @@ -364,6 +364,10 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
>>>>  		 * A sub-device can be plugged later.
>>>>  		 */
>>>>  		FOREACH_SUBDEV(sdev, i, eth_dev) {
>>>> +			/* skip empty devargs */
>>>> +			if (sdev->devargs.name[0] == '\0')
>>>> +				continue;
>>>> +  
>>>
>>> An empty devargs being named "" is part of the internals of rte_devargs.
>>> The clean solution would be to add a `bool rte_devargs_empty()` function
>>> and test the devargs with it.
>>>
>>> The simple solution is your proposition.
>>>
>>> Clean seems a little heavy-handed, but it would be more stable. If you
>>> agree, you can add the helper. I'm ok with keeping it simple otherwise.
>>>
>>>>  			/* rebuild devargs to be able to get the bus name. */
>>>>  			ret = rte_devargs_parse(&devargs,
>>>>  						sdev->devargs.name);
>>>> -- 
>>>> 2.20.1
>>>>   
>>>
>>
>> Simpler is better.
> 
> Ok

is this an ack :)

> 
>> Sorry, after working with failsafe my impression is that it is not
>> built with that in mind.
> 

Re: [dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Gaëtan Rivet]

On Wed, Jul 17, 2019 at 07:21:51PM +0100, Ferruh Yigit wrote:
> On 6/24/2019 5:27 PM, Gaëtan Rivet wrote:
> > On Mon, Jun 24, 2019 at 08:23:38AM -0700, Stephen Hemminger wrote:
> >> On Mon, 24 Jun 2019 10:15:58 +0200
> >> Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> >>
> >>> Hello Stephen,
> >>>
> >>> On Fri, Jun 21, 2019 at 03:08:24PM -0700, Stephen Hemminger wrote:
> >>>> When secondary process is run was noticing that the log always
> >>>> contained complaints about unable to parse devargs.
> >>>>
> >>>> It turns out that an empty devargs turns into "" and this
> >>>> value is not parsable. Change the failsafe secondary to just
> >>>> skip doing devargs if it empty.
> >>>>   
> >>>
> >>> Commit log needs a little rework, a few typos.
> >>>
> >>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>> ---
> >>>>  drivers/net/failsafe/failsafe.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> >>>> index e91c274d8059..04ca0cab0d78 100644
> >>>> --- a/drivers/net/failsafe/failsafe.c
> >>>> +++ b/drivers/net/failsafe/failsafe.c
> >>>> @@ -364,6 +364,10 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
> >>>>  		 * A sub-device can be plugged later.
> >>>>  		 */
> >>>>  		FOREACH_SUBDEV(sdev, i, eth_dev) {
> >>>> +			/* skip empty devargs */
> >>>> +			if (sdev->devargs.name[0] == '\0')
> >>>> +				continue;
> >>>> +  
> >>>
> >>> An empty devargs being named "" is part of the internals of rte_devargs.
> >>> The clean solution would be to add a `bool rte_devargs_empty()` function
> >>> and test the devargs with it.
> >>>
> >>> The simple solution is your proposition.
> >>>
> >>> Clean seems a little heavy-handed, but it would be more stable. If you
> >>> agree, you can add the helper. I'm ok with keeping it simple otherwise.
> >>>
> >>>>  			/* rebuild devargs to be able to get the bus name. */
> >>>>  			ret = rte_devargs_parse(&devargs,
> >>>>  						sdev->devargs.name);
> >>>> -- 
> >>>> 2.20.1
> >>>>   
> >>>
> >>
> >> Simpler is better.
> > 
> > Ok
> 
> is this an ack :)
> 

Yes sorry,

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> > 
> >> Sorry, after working with failsafe my impression is that it is not
> >> built with that in mind.
> > 
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] failsafe: skip devargs if not present in secondary

[Ferruh Yigit]

On 7/18/2019 12:18 PM, Gaëtan Rivet wrote:
> On Wed, Jul 17, 2019 at 07:21:51PM +0100, Ferruh Yigit wrote:
>> On 6/24/2019 5:27 PM, Gaëtan Rivet wrote:
>>> On Mon, Jun 24, 2019 at 08:23:38AM -0700, Stephen Hemminger wrote:
>>>> On Mon, 24 Jun 2019 10:15:58 +0200
>>>> Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>>>>
>>>>> Hello Stephen,
>>>>>
>>>>> On Fri, Jun 21, 2019 at 03:08:24PM -0700, Stephen Hemminger wrote:
>>>>>> When secondary process is run was noticing that the log always
>>>>>> contained complaints about unable to parse devargs.
>>>>>>
>>>>>> It turns out that an empty devargs turns into "" and this
>>>>>> value is not parsable. Change the failsafe secondary to just
>>>>>> skip doing devargs if it empty.
>>>>>>   
>>>>>
>>>>> Commit log needs a little rework, a few typos.
>>>>>
>>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

<...>

> 
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.