From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4768D43873; Tue, 9 Jan 2024 15:31:51 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E8DB14021F; Tue, 9 Jan 2024 15:31:50 +0100 (CET) Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) by mails.dpdk.org (Postfix) with ESMTP id 7252E4013F for ; Tue, 9 Jan 2024 15:31:49 +0100 (CET) Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-1d54b86538aso10653345ad.0 for ; Tue, 09 Jan 2024 06:31:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1704810708; x=1705415508; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=XVxgBij+3SDJBn2Dc9lCMLd7pvF+8aftEfXYlQLHWqU=; b=JWn+94PzaSY+PEWb5e2QCnJLfCz6bib94Srf9ItQtA/5/jkv7ECqRuFmqRNob5qjHl U8KrAUsWy7amVos9VEISEHJK1e3SISudLVYtfwpVO0HXzEyjipjIqPI9aCiIYquzDkpe bLW+nlf8seALR985T6Cfxp0CjTL/ehjMjoNis= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704810708; x=1705415508; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=XVxgBij+3SDJBn2Dc9lCMLd7pvF+8aftEfXYlQLHWqU=; b=AzThZRKsGKx+UrRDsdFXZ4oc2jze1rizeyGw9BFc7c64PbwBBOi0f9SCyJD8fkufG6 5huG55JOYl4jiAzOaqtDBTQvapCP7Pfz7CTK4XKBSBo+a0ptlO+4nIpvTSL8/kj9tO1o LBPo0qCbj0spXPc23d6z2wlyjwiRbd0PIVeamc3Snbz/uaJTeeaMjo2Nb0MNF/UIZVzD aDd/1fwUQcLb3CsvBc+p9x4DVxxfTo/tdxKY0M7Bt2wUZjTYyga64Pawn6JdLBW/Vj6P 1n9jvSNivSqUtjQauG8ZCyGFuH2PE2BANmfBmIn+nF54iWFOcAQFjJ0oR0xWijwY9dWo 59eA== X-Gm-Message-State: AOJu0YxU+CZr3t9e7z8NNI7ExcWRZQfDXdYE19FO/D5p46Ws2PfEr9Ee b+43BILUho6i890Rh/xu6+fWtNzBpR+VQJcxFu7ppkzaKcUtI2FxEUJ8w+CESD0= X-Google-Smtp-Source: AGHT+IG7Q/FpxHexePJ/9YkdDpPkSJdaXJmlHcc48XJnpQrNPwigxrUrGAOctFSmnv4vqRb/eviveYBQXR+BFm/Gg50= X-Received: by 2002:a17:90a:c41:b0:28d:4fcf:c224 with SMTP id u1-20020a17090a0c4100b0028d4fcfc224mr1845285pje.60.1704810708453; Tue, 09 Jan 2024 06:31:48 -0800 (PST) MIME-Version: 1.0 References: <20240103221217.18954-1-jspewock@iol.unh.edu> <20240103223206.23129-1-jspewock@iol.unh.edu> <20240103223206.23129-2-jspewock@iol.unh.edu> In-Reply-To: From: Jeremy Spewock Date: Tue, 9 Jan 2024 09:31:37 -0500 Message-ID: Subject: Re: [PATCH v6 1/7] dts: add startup verification and forwarding modes to testpmd shell To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, wathsala.vithanage@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.arm.com, lylavoie@iol.unh.edu, ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000afe6d5060e842ca8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000afe6d5060e842ca8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Jan 9, 2024 at 6:55=E2=80=AFAM Juraj Linke=C5=A1 wrote: > On Mon, Jan 8, 2024 at 5:36=E2=80=AFPM Jeremy Spewock > wrote: > > > > > > > > On Mon, Jan 8, 2024 at 6:35=E2=80=AFAM Juraj Linke=C5=A1 > wrote: > >> > >> On Wed, Jan 3, 2024 at 11:33=E2=80=AFPM wrote: > >> > > >> > From: Jeremy Spewock > >> > > >> > Added commonly used methods in testpmd such as starting and stopping > >> > packet forwarding, changing forward modes, and verifying link status > of > >> > ports so that developers can configure testpmd and start forwarding > >> > through the provided class rather than sending commands to the testp= md > >> > session directly. > >> > > >> > Signed-off-by: Jeremy Spewock > >> > --- > >> > dts/framework/exception.py | 7 + > >> > dts/framework/remote_session/testpmd_shell.py | 149 > +++++++++++++++++- > >> > 2 files changed, 155 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/dts/framework/exception.py b/dts/framework/exception.py > >> > index 658eee2c38..cce1e0231a 100644 > >> > --- a/dts/framework/exception.py > >> > +++ b/dts/framework/exception.py > >> > >> > @@ -65,9 +108,66 @@ class TestPmdShell(InteractiveShell): > >> > _command_extra_chars: ClassVar[str] =3D "\n" > >> > > >> > def _start_application(self, get_privileged_command: > Callable[[str], str] | None) -> None: > >> > - self._app_args +=3D " -- -i" > >> > + """Overrides :meth:`~.interactive_shell._start_application`= . > >> > + > >> > + Add flags for starting testpmd in interactive mode and > disabling messages for link state > >> > + change events before starting the application. Link state i= s > verified before starting > >> > + packet forwarding and the messages create unexpected > newlines in the terminal which > >> > + complicates output collection. > >> > + > >> > >> We should adjust the collection so that it can handle the newlines. > >> Also, can you explain exactly why we are disabling the initial link > >> state messages? > > > > > > The problem really comes from the newlines causing the prompt to exist > in the buffer before any command is sent. So, what ends up happening is > after starting the application these link state change events happen at > some point, and they cause an empty "testpmd>" line to exist in the buffe= r > and the next time you send a command it will stop as soon as it encounter= s > that line. > > These buffer issues keep cropping up. We should think about making > this more robust. Can we flush the buffer before sending a new command > (because any previous output is irrelevant)? This probably won't fix > all the problems, but it sounds like it could help. Maybe it could > I definitely think this would be something that is useful because it allows us to at least make some more helpful assumptions like the buffer only contains the output after you sent your command. When I was first looking into making the interactive shells I was looking for ways to do this and the only way I could really find for flushing the buffer is just consuming everything that's in it. One way to do this is just to keep collecting output until you reach the timeout which occurs when there is nothing left. Waiting for a timeout has its flaws as well, but I believe it was the cleanest way to make sure the buffer was empty. Overall though, I agree this would be something good to look into and I'd happily explore expanding this and also looking into things we discussed earlier like changing the delimiter on the buffer. > help with the scapy docstring issue we're seen in the past as well. > I'm not sure it would do much for this issue because this stemmed more from the multiline command I believe. However, making output collection more robust does also make the opportunity to fix these issues by doing things like removing empty lines before sending as a sanitizing step. > In this patch though, we should just make this functional (I > understand disabling the messages achieves that) and address the > buffer issues in a separate patch. > > > An additional issue with this prompt is it is put in the buffer before > the link state change event occurs, and there is no prompt that appears > after the event messages, just an empty line. This makes it much more > difficult to detect when the link state change event occurs and consume i= t > because the event isn't captured the next time you collect output, all th= at > is consumed is a line containing the prompt.. So, this makes you > essentially one command's worth of output behind because the next time yo= u > send a command you will consume what you were supposed to get from the la= st > command where you stopped early, and this causes false positives for thin= gs > like the link state detection method and failures in output verification. > > This puts you in a position where the only way you can really detect > that one of these events happened is either assuming that only getting an > empty prompt means one of these events happened, or trying to consume > output twice and looking ahead to see if one of these events happened. > However, because we wouldn't be doing anything with these events and we > verify link status before starting anyway, it seemed like the less comple= x > but still functional solution would just be to mask these events. > > Right, these are basically random events, which makes it hard to > collect (but not impossible). Checking the status explicitly is way > better. Thanks for the explanation. > > > > >> > >> > >> > + Also find the number of pci addresses which were allowed on > the command line when the app > >> > + was started. > >> > + """ > >> > + self._app_args +=3D " -- -i --mask-event intr_lsc" > >> > + self.number_of_ports =3D self._app_args.count("-a ") > >> > super()._start_application(get_privileged_command) > >> > > >> > + def start(self, verify: bool =3D True) -> None: > >> > + """Start packet forwarding with the current configuration. > >> > + > >> > + Args: > >> > + verify: If :data:`True` , a second start command will b= e > sent in an attempt to verify > >> > + packet forwarding started as expected. > >> > + > >> > + Raises: > >> > + InteractiveCommandExecutionError: If `verify` is > :data:`True` and forwarding fails to > >> > + start or ports fail to come up. > >> > + """ > >> > + self.send_command("start") > >> > + if verify: > >> > + # If forwarding was already started, sending "start" > again should tell us > >> > + start_cmd_output =3D self.send_command("start") > >> > + if "Packet forwarding already started" not in > start_cmd_output: > >> > + self._logger.debug(f"Failed to start packet > forwarding: \n{start_cmd_output}") > >> > + raise InteractiveCommandExecutionError("Testpmd > failed to start packet forwarding.") > >> > + > >> > + for port_id in range(self.number_of_ports): > >> > + if not self.wait_link_status_up(port_id): > >> > + raise InteractiveCommandExecutionError( > >> > + "Not all ports came up after starting packe= t > forwarding in testpmd." > >> > + ) > >> > + > >> > + def stop(self, verify: bool =3D True) -> None: > >> > + """Stop packet forwarding. > >> > + > >> > + Args: > >> > + verify: If :data:`True` , the output of the stop comman= d > is scanned to verify that > >> > + forwarding was stopped successfully or not started. > If neither is found, it is > >> > + considered an error. > >> > + > >> > + Raises: > >> > + InteractiveCommandExecutionError: If `verify` is > :data:`True` and the command to stop > >> > + forwarding results in an error. > >> > + """ > >> > + stop_cmd_output =3D self.send_command("stop") > >> > + if verify: > >> > + if ( > >> > + "Done." not in stop_cmd_output > >> > + and "Packet forwarding not started" not in > stop_cmd_output > >> > + ): > >> > >> I want to make sure I understand this condition. When none of these > >> appear, it's an error. When just "Done." appears, we successfully > >> stopped ongoing forwarding and when "Packet forwarding not started" > >> appears, we're trying to stop forwarding that didn't start (or isn't > >> ongoing - it could've stopped in the meantime)? > >> I'm thinking about false failures here (Is there a string that would > >> indicate a failure even if one of the strings is printed?) - we're > >> basically looking at "not success" instead of looking for strings > >> telling us about a failure explicitly. Does the stop command not > >> produce such output? Or do we not know all of the failure strings or > >> is looking for the above two strings sufficient to rule out false > >> failures? > > > > > > You are correct that essentially what I am looking for here is if we > succeeded and else, it's a failure. When I looked through some of the > source code for testpmd from the method stop_packet_forwarding, I didn't > see any explicit error messages other than displaying that there was an > error printing statistics. So this was something where I both didn't know > the error messages but it doesn't look like there are any that are > explicitly printed. In the case of false failures however, the strings I = am > detecting are always encountered in our two success cases (forwarding not > currently started and successfully stopped). The "Done." message does als= o > get printed in the case of statistics failing to print for a core as well= . > > Ok, seems like the check is robust enough. > --000000000000afe6d5060e842ca8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


<= div dir=3D"ltr" class=3D"gmail_attr">On Tue, Jan 9, 2024 at 6:55=E2=80=AFAM= Juraj Linke=C5=A1 <juraj.linkes@pantheon.tech> wrote:
On Mon, Jan 8, 2024 at 5:36=E2= =80=AFPM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
>
> On Mon, Jan 8, 2024 at 6:35=E2=80=AFAM Juraj Linke=C5=A1 <juraj.lin= kes@pantheon.tech> wrote:
>>
>> On Wed, Jan 3, 2024 at 11:33=E2=80=AFPM <jspewock@iol.unh.edu> wrote:
>> >
>> > From: Jeremy Spewock <jspewock@iol.unh.edu>
>> >
>> > Added commonly used methods in testpmd such as starting and s= topping
>> > packet forwarding, changing forward modes, and verifying link= status of
>> > ports so that developers can configure testpmd and start forw= arding
>> > through the provided class rather than sending commands to th= e testpmd
>> > session directly.
>> >
>> > Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
>> > ---
>> >=C2=A0 dts/framework/exception.py=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 =C2=A07 +
>> >=C2=A0 dts/framework/remote_session/testpmd_shell.py | 149 +++= ++++++++++++++-
>> >=C2=A0 2 files changed, 155 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/dts/framework/exception.py b/dts/framework/excep= tion.py
>> > index 658eee2c38..cce1e0231a 100644
>> > --- a/dts/framework/exception.py
>> > +++ b/dts/framework/exception.py
>> <snip>
>> > @@ -65,9 +108,66 @@ class TestPmdShell(InteractiveShell):
>> >=C2=A0 =C2=A0 =C2=A0 _command_extra_chars: ClassVar[str] =3D &= quot;\n"
>> >
>> >=C2=A0 =C2=A0 =C2=A0 def _start_application(self, get_privileg= ed_command: Callable[[str], str] | None) -> None:
>> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._app_args +=3D " -- -i= "
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Overrides :met= h:`~.interactive_shell._start_application`.
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Add flags for starting testpmd i= n interactive mode and disabling messages for link state
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 change events before starting th= e application. Link state is verified before starting
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 packet forwarding and the messag= es create unexpected newlines in the terminal which
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 complicates output collection. >> > +
>>
>> We should adjust the collection so that it can handle the newlines= .
>> Also, can you explain exactly why we are disabling the initial lin= k
>> state messages?
>
>
> The problem really comes from the newlines causing the prompt to exist= in the buffer before any command is sent. So, what ends up happening is af= ter starting the application these link state change events happen at some = point, and they cause an empty "testpmd>" line to exist in the= buffer and the next time you send a command it will stop as soon as it enc= ounters that line.

These buffer issues keep cropping up. We should think about making
this more robust. Can we flush the buffer before sending a new command
(because any previous output is irrelevant)? This probably won't fix all the problems, but it sounds like it could help. Maybe it could

I definitely think this would be something that i= s useful because it allows us to at least make some more helpful assumption= s like the buffer only contains the output after you sent your command. Whe= n I was first looking into making the interactive shells I was looking for = ways to do this and the only way I could really find for flushing the buffe= r is just consuming everything that's in it. One way to do this is just= to keep collecting output until you reach the timeout which occurs when th= ere is nothing left. Waiting for a timeout has its flaws as well, but I bel= ieve it was the cleanest way to make sure the buffer was empty. Overall tho= ugh, I agree this would be something good to look into and I'd happily = explore expanding this and also looking into things we discussed earlier li= ke changing the delimiter on the buffer.
=C2=A0
=
help with the scapy docstring issue we're seen in the past as well.
=
=C2=A0
I'm not sure it would do much for this issu= e because this stemmed more from the multiline command I believe. However, = making output collection more robust does also make the opportunity to fix = these issues by doing things like removing empty lines before sending as a = sanitizing step.


In this patch though, we should just make this functional (I
understand disabling the messages achieves that) and address the
buffer issues in a separate patch.

> An additional issue with this prompt is it is put in the buffer before= the link state change event occurs, and there is no prompt that appears af= ter the event messages, just an empty line. This makes it much more difficu= lt to detect when the link state change event occurs and consume it because= the event isn't captured the next time you collect output, all that is= consumed is a line containing the prompt.. So, this makes you essentially = one command's worth of output behind because the next time you send a c= ommand you will consume what you were supposed to get from the last command= where you stopped early, and this causes false positives for things like t= he link state detection method and failures in output verification.
> This puts you in a position where the only way you can really detect t= hat one of these events happened is either assuming that only getting an em= pty prompt means one of these events happened, or trying to consume output = twice and looking ahead to see if one of these events happened. However, be= cause we wouldn't be doing anything with these events and we verify lin= k status before starting anyway, it seemed like the less complex but still = functional solution would just be to mask these events.

Right, these are basically random events, which makes it hard to
collect (but not impossible). Checking the status explicitly is way
better. Thanks for the explanation.

>
>>
>>
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Also find the number of pci addr= esses which were allowed on the command line when the app
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 was started.
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._app_args +=3D " -- -i= --mask-event intr_lsc"
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.number_of_ports =3D self._a= pp_args.count("-a ")
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super()._start_application(= get_privileged_command)
>> >
>> > +=C2=A0 =C2=A0 def start(self, verify: bool =3D True) -> N= one:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Start packet f= orwarding with the current configuration.
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify: If :data:`= True` , a second start command will be sent in an attempt to verify
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pack= et forwarding started as expected.
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Raises:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 InteractiveCommand= ExecutionError: If `verify` is :data:`True` and forwarding fails to
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 star= t or ports fail to come up.
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.send_command("start&qu= ot;)
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if verify:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # If forwarding wa= s already started, sending "start" again should tell us
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 start_cmd_output = =3D self.send_command("start")
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if "Packet fo= rwarding already started" not in start_cmd_output:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self= ._logger.debug(f"Failed to start packet forwarding: \n{start_cmd_outpu= t}")
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 rais= e InteractiveCommandExecutionError("Testpmd failed to start packet for= warding.")
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for port_id in ran= ge(self.number_of_ports):
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if n= ot self.wait_link_status_up(port_id):
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 raise InteractiveCommandExecutionError(
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 "Not all ports came up after starting packet = forwarding in testpmd."
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 )
>> > +
>> > +=C2=A0 =C2=A0 def stop(self, verify: bool =3D True) -> No= ne:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """Stop packet fo= rwarding.
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Args:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 verify: If :data:`= True` , the output of the stop command is scanned to verify that
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 forw= arding was stopped successfully or not started. If neither is found, it is<= br> >> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cons= idered an error.
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 Raises:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 InteractiveCommand= ExecutionError: If `verify` is :data:`True` and the command to stop
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 forw= arding results in an error.
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 """
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 stop_cmd_output =3D self.send_co= mmand("stop")
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if verify:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &quo= t;Done." not in stop_cmd_output
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 and = "Packet forwarding not started" not in stop_cmd_output
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ):
>>
>> I want to make sure I understand this condition. When none of thes= e
>> appear, it's an error. When just "Done." appears, we= successfully
>> stopped ongoing forwarding and when "Packet forwarding not st= arted"
>> appears, we're trying to stop forwarding that didn't start= (or isn't
>> ongoing - it could've stopped in the meantime)?
>> I'm thinking about false failures here (Is there a string that= would
>> indicate a failure even if one of the strings is printed?) - we= 9;re
>> basically looking at "not success" instead of looking fo= r strings
>> telling us about a failure explicitly. Does the stop command not >> produce such output? Or do we not know all of the failure strings = or
>> is looking for the above two strings sufficient to rule out false<= br> >> failures?
>
>
> You are correct that essentially what I am looking for here is if we s= ucceeded and else, it's a failure. When I looked through some of the so= urce code for testpmd from the method stop_packet_forwarding, I didn't = see any explicit error messages other than displaying that there was an err= or printing statistics. So this was something where I both didn't know = the error messages but it doesn't look like there are any that are expl= icitly printed. In the case of false failures however, the strings I am det= ecting are always encountered in our two success cases (forwarding not curr= ently started and successfully stopped). The "Done." message does= also get printed in the case of statistics failing to print for a core as = well.

Ok, seems like the check is robust enough.
--000000000000afe6d5060e842ca8--