[v2] Revert "eal: fix parsing option --telemetry"

This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.

Reverting this patch as it currently breaks the initialization of
telemetry, more investigation is ongoing to fix the issue for the
printed error message for unrecognized argument.

Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
---
v2:
  Adding sign off
---
 lib/librte_eal/common/eal_common_options.c | 3 ---
 lib/librte_eal/common/eal_options.h        | 4 ----
 2 files changed, 7 deletions(-)
  

24/07/2019 17:20, Sean Morrissey:
> This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.
> 
> Reverting this patch as it currently breaks the initialization of
> telemetry, more investigation is ongoing to fix the issue for the
> printed error message for unrecognized argument.
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>

It's very disapointing.
Did you or the reviewer tested the previous patch?
  

Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry"

24/07/2019 17:20, Sean Morrissey:
> This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.
> 
> Reverting this patch as it currently breaks the initialization of 
> telemetry, more investigation is ongoing to fix the issue for the 
> printed error message for unrecognized argument.
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>

It's very disapointing.
Did you or the reviewer tested the previous patch?

The reporter of the bug tested this and verified functionality and closed the internal bug.

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
  

24/07/2019 18:28, Morrissey, Sean:
> 
> Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry"
> 
> 24/07/2019 17:20, Sean Morrissey:
> > This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.
> > 
> > Reverting this patch as it currently breaks the initialization of 
> > telemetry, more investigation is ongoing to fix the issue for the 
> > printed error message for unrecognized argument.
> > 
> > Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> 
> It's very disapointing.
> Did you or the reviewer tested the previous patch?
> 
> The reporter of the bug tested this and verified functionality and closed the internal bug.

So the reverted commit is supposed to work?
I won't apply this revert until I fully understand what happens.
  

Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry"

24/07/2019 18:28, Morrissey, Sean:
> 
> Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry"
> 
> 24/07/2019 17:20, Sean Morrissey:
> > This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.
> > 
> > Reverting this patch as it currently breaks the initialization of 
> > telemetry, more investigation is ongoing to fix the issue for the 
> > printed error message for unrecognized argument.
> > 
> > Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> 
> It's very disapointing.
> Did you or the reviewer tested the previous patch?
> 
> The reporter of the bug tested this and verified functionality and closed the internal bug.

So the reverted commit is supposed to work?
I won't apply this revert until I fully understand what happens.

The patch that I sent up was to remove an "unrecognized option telemetry" warning  message, which the patch removed and was tested to ensure the message was removed. Further testing, after the patch was sent up, revealed that the unix domain socket, which is required by telemetry consumers, was no longer being created, rendering the telemetry functionality non-functional. 
On further investigation, the full fix involves a change in the EAL command line parameter handling, which is probably too risky for RC3. 
This revert will allow telemetry to function again, but with the erroneous message still in place.
We will aim to fix in the next release.

--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
  

On Thu, Jul 25, 2019 at 10:38 AM Morrissey, Sean
<sean.morrissey@intel.com> wrote:
>
> Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry"
>
> 24/07/2019 18:28, Morrissey, Sean:
> >
> > Subject: Re: [PATCH v2] Revert "eal: fix parsing option --telemetry"
> >
> > 24/07/2019 17:20, Sean Morrissey:
> > > This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.
> > >
> > > Reverting this patch as it currently breaks the initialization of
> > > telemetry, more investigation is ongoing to fix the issue for the
> > > printed error message for unrecognized argument.
> > >
> > > Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> >
> > It's very disapointing.
> > Did you or the reviewer tested the previous patch?
> >
> > The reporter of the bug tested this and verified functionality and closed the internal bug.
>
> So the reverted commit is supposed to work?
> I won't apply this revert until I fully understand what happens.
>
> The patch that I sent up was to remove an "unrecognized option telemetry" warning  message, which the patch removed and was tested to ensure the message was removed. Further testing, after the patch was sent up, revealed that the unix domain socket, which is required by telemetry consumers, was no longer being created, rendering the telemetry functionality non-functional.
> On further investigation, the full fix involves a change in the EAL command line parameter handling, which is probably too risky for RC3.
> This revert will allow telemetry to function again, but with the erroneous message still in place.
> We will aim to fix in the next release.

Might be good to look and revisit the rte_option api.


> --------------------------------------------------------------
> Intel Research and Development Ireland Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
>
>
> This e-mail and any attachments may contain confidential material for the sole
> use of the intended recipient(s). Any review or distribution by others is
> strictly prohibited. If you are not the intended recipient, please contact the
> sender and delete all copies.
>

Can you contact your IT and get this footer removed ?
Thanks.
  

25/07/2019 10:42, David Marchand:
> > > > 24/07/2019 17:20, Sean Morrissey:
> > > > > This reverts commit debacba0297fbe214b4185a9791e6a9fdf6642ba.
> > > > > 
> > > > > Reverting this patch as it currently breaks the initialization of
> > > > > telemetry, more investigation is ongoing to fix the issue for the
> > > > > printed error message for unrecognized argument.
> > > > > 
> > > > > Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> > > > 
> > > > It's very disapointing.
> > > > Did you or the reviewer tested the previous patch?
> > > > 
> > > > The reporter of the bug tested this and verified functionality and
> > > > closed the internal bug.> > > 
> > > 
> > > So the reverted commit is supposed to work?
> > > I won't apply this revert until I fully understand what happens.
> >
> > The patch that I sent up was to remove an "unrecognized
> > option telemetry" warning  message, which the patch
> > removed and was tested to ensure the message was removed.
> > Further testing, after the patch was sent up,

So the patch was sent and reviewed without testing the functionality.

> > revealed that the unix domain socket,
> > which is required by telemetry consumers,
> > was no longer being created,
> > rendering the telemetry functionality non-functional.

Why this socket is not created?
How is it related to the option parsing?

> > On further investigation, the full fix involves
> > a change in the EAL command line parameter handling,
> > which is probably too risky for RC3.

No way you change the command line parsing,
except the rte_option which was created for telemetry.
The history of this "simple" feature is already full
of hesitations which made me hesitate to merge.
Please, don't force me to dig in the code, otherwise
I will be tempted to do a big "clean-up".

> > This revert will allow telemetry to function again,
> > but with the erroneous message still in place.
> > We will aim to fix in the next release.
> 
> Might be good to look and revisit the rte_option api.
  

On Sun, Jul 28, 2019 at 9:55 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 25/07/2019 10:42, David Marchand:
> > > > > 24/07/2019 17:20, Sean Morrissey:
> > > On further investigation, the full fix involves
> > > a change in the EAL command line parameter handling,
> > > which is probably too risky for RC3.
>
> No way you change the command line parsing,
> except the rte_option which was created for telemetry.
> The history of this "simple" feature is already full
> of hesitations which made me hesitate to merge.
> Please, don't force me to dig in the code, otherwise
> I will be tempted to do a big "clean-up".
>
> > > This revert will allow telemetry to function again,
> > > but with the erroneous message still in place.
> > > We will aim to fix in the next release.
> >
> > Might be good to look and revisit the rte_option api.
>

The patch on eal did not make any sense.
I am for reverting it too.

About the real fix, I'd like to first see a description of the actual problem.
Thanks.
  

On Mon, Jul 29, 2019 at 10:40:59AM +0200, David Marchand wrote:
> On Sun, Jul 28, 2019 at 9:55 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 25/07/2019 10:42, David Marchand:
> > > > > > 24/07/2019 17:20, Sean Morrissey:
> > > > On further investigation, the full fix involves
> > > > a change in the EAL command line parameter handling,
> > > > which is probably too risky for RC3.
> >
> > No way you change the command line parsing,
> > except the rte_option which was created for telemetry.
> > The history of this "simple" feature is already full
> > of hesitations which made me hesitate to merge.
> > Please, don't force me to dig in the code, otherwise
> > I will be tempted to do a big "clean-up".
> >
> > > > This revert will allow telemetry to function again,
> > > > but with the erroneous message still in place.
> > > > We will aim to fix in the next release.
> > >
> > > Might be good to look and revisit the rte_option api.
> >
> 
> The patch on eal did not make any sense.
> I am for reverting it too.
> 

Hi all,

quick look at it:

 719                 /*
 720                  * getopt didn't recognise the option, lets parse the
 721                  * registered options to see if the flag is valid
 722                  */
 723                 if (opt == '?') {
 724                         ret = rte_option_parse(argv[optind-1]);
 725                         if (ret == 0)
 726                                 continue;
 727
 728                         eal_usage(prgname);
 729                         ret = -1;
 730                         goto out;
 731                 }

If the --telemetry option is added to the EAL command line,
then this branch will not happen, which explains why the socket was not opened
on the lib side.

> About the real fix, I'd like to first see a description of the actual problem.
> Thanks.
> 
> 
> -- 
> David Marchand

Agreed, to add to this: was the build shared? It seems Stephen
encountered errors with RTE_INIT of DPDK libs not being executed in
shared builds, which could explain why everything would seem fine, but
the rte_option would not be registered, making the parsing error appear.
  

29/07/2019 10:40, David Marchand:
> On Sun, Jul 28, 2019 at 9:55 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 25/07/2019 10:42, David Marchand:
> > > > > > 24/07/2019 17:20, Sean Morrissey:
> > > > On further investigation, the full fix involves
> > > > a change in the EAL command line parameter handling,
> > > > which is probably too risky for RC3.
> >
> > No way you change the command line parsing,
> > except the rte_option which was created for telemetry.
> > The history of this "simple" feature is already full
> > of hesitations which made me hesitate to merge.
> > Please, don't force me to dig in the code, otherwise
> > I will be tempted to do a big "clean-up".
> >
> > > > This revert will allow telemetry to function again,
> > > > but with the erroneous message still in place.
> > > > We will aim to fix in the next release.
> > >
> > > Might be good to look and revisit the rte_option api.
> 
> The patch on eal did not make any sense.
> I am for reverting it too.

OK, reverted.

Next time I will see a patch for telemetry, I will ask all questions
and will check by myself.