diff mbox series

failsafe: skip devargs if not present in secondary

Message ID 20190621220824.18073-1-stephen@networkplumber.org (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series failsafe: skip devargs if not present in secondary |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail Compilation issues

Commit Message

Stephen Hemminger June 21, 2019, 10:08 p.m. UTC 
  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(+)

Comments

Gaëtan Rivet June 24, 2019, 8:15 a.m. UTC | #1 
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
>
Stephen Hemminger June 24, 2019, 3:23 p.m. UTC | #2 
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.
Gaëtan Rivet June 24, 2019, 4:27 p.m. UTC | #3 
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.
Ferruh Yigit July 17, 2019, 6:21 p.m. UTC | #4 
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.
>
Gaëtan Rivet July 18, 2019, 11:18 a.m. UTC | #5 
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.
> > 
>
Ferruh Yigit July 19, 2019, 12:36 p.m. UTC | #6 
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.
diff mbox series

Patch

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);