On Mon, Jan 09, 2023 at 08:26:37PM +0800, lihuisong (C) wrote:
>
> 在 2023/1/9 17:14, Bruce Richardson 写道:
> > On Mon, Jan 09, 2023 at 02:55:46PM +0800, Huisong Li wrote:
> > > The telemetry client script uses argparse module to get input parameter.
> > >
> > > Signed-off-by: Huisong Li <lihuis...@huawei.com>
> > > ---
> > > usertools/dpdk-telemetry-client.py | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > This is an old script using the older telemetry V1 interface, so I'd
> > generally recommend users switch to using scripts for the v2 interface.
> > That said, no reason not to improve the script while we have it.
> Yes. After all, the telemetry v1 interface and this script are still exist.
> >
> > > diff --git a/usertools/dpdk-telemetry-client.py
> > > b/usertools/dpdk-telemetry-client.py
> > > index df41d04fbe..fd69955b32 100755
> > > --- a/usertools/dpdk-telemetry-client.py
> > > +++ b/usertools/dpdk-telemetry-client.py
> > > @@ -6,6 +6,7 @@
> > > import os
> > > import sys
> > > import time
> > > +import argparse
> > > BUFFER_SIZE = 200000
> > > @@ -115,13 +116,12 @@ def interactiveMenu(self, sleep_time): # Creates
> > > Interactive menu within the scr
> > > if __name__ == "__main__":
> > > sleep_time = 1
> > > - file_path = ""
> > > - if len(sys.argv) == 2:
> > > - file_path = sys.argv[1]
> > > - else:
> > > - print("Warning - No filepath passed, using default (" +
> > > DEFAULT_FP + ").")
> > > - file_path = DEFAULT_FP
> > > + parser = argparse.ArgumentParser()
> > > + parser.add_argument('-s', '--sock_path', default=DEFAULT_FP,
> > > + help='Provide socket file path connected by
> > > legacy client')
> > > + args = parser.parse_args()
> > > +
> > While I like using argparse rather than handling args directly, this breaks
> > compatibility. For anyone already using this script via automation, this
> > would break things, as the path needs to be provided via a "-s" parameter,
> > rather than just tacked on as argv[1].
> If there isn't the modification patch 2/2 mentioned, this script cannot be
> directly used in most scenarios. From the first commit of this script, it's
> just used as a demo client example. See
> commit d1b94da4a4e0 ("usertools: add client script for telemetry")
> From this point of view, can this compatibility issue be ignored?
Agree with you that patch 2/2 is necessary to make script useful for most
cases, and also agree that argparse is the better way to do argument
handling. However, I also think that we can keep compatibility in this
matter - you can add optional positional arguments in argparse to support
backward compatibility.
parser.add_argument('sock_path', nargs='?', default=...)
should work, I believe, for this case.
/Bruce
