From: Jeremy Spewock <jspewock@iol.unh.edu>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
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,
Luca.Vizzarro@arm.com, dev@dpdk.org
Subject: Re: [PATCH v1 1/2] dts: Improve output gathering in interactive shells
Date: Mon, 8 Apr 2024 12:20:38 -0400 [thread overview]
Message-ID: <CAAA20USozCrgtAKpSU50mKFVJwUzviCOdFJ7TBDCxkv2=B9nRg@mail.gmail.com> (raw)
In-Reply-To: <CAOb5WZYuF+4cMRRUOtSkwQFaBsSyW-CkiNN+mRf83hP6Z3160Q@mail.gmail.com>
On Wed, Apr 3, 2024 at 5:00 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Tue, Mar 12, 2024 at 6:26 PM <jspewock@iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock@iol.unh.edu>
> >
> > The current implementation of consuming output from interactive shells
> > relies on being able to find an expected prompt somewhere within the
> > output buffer after sending the command. This is useful in situations
> > where the prompt does not appear in the output itself, but in some
> > practical cases (such as the starting of an XML-RPC server for scapy)
> > the prompt exists in one of the commands sent to the shell and this can
> > cause the command to exit early and creates a race condition between the
> > server starting and the first command being sent to the server.
> >
> > This patch addresses this problem by searching for a line that strictly
> > ends with the provided prompt, rather than one that simply contains it,
> > so that the detection that a command is finished is more consistent. It
> > also adds a catch to detect when a command times out before finding the
> > prompt so that the exception can be wrapped into a more explicit one and
> > display the output that it did manage to gather before timing out.
> >
>
> This could still cause problems if the prompt appears at the end of a
> line in the output. Maybe we don't need to worry about that until we
> actually hit that problem. In any case, I'd like to test this, so
> please rebase the patch. :-)
I will rebase and send out a v2, but that is a good point. it would be
just as easy to instead check to see that the prompt is the only thing
on the line, so we could do that instead, which do you think is
better? I'm sort of indifferent, I can see how verifying that the line
only contains the prompt would be useful in cases like it appears in a
docstring or something similar (and it's nice to be more explicit in
general), and I think that leaving it as the end of the line could
potentially save some verbosity if the line you are looking for is
long so you can just detect the end of it. Another problem that I
could think of with long lines potentially is if, somehow, you were
looking for a prompt that the pseudo-terminal split across a few lines
and it split your prompt, but I'm not sure this is really relevant to
us at all since we only really expect prompts that are usually short.
>
<snip>
> > + try:
> > + for line in self._stdout:
> > + out += line
> > + if line.rstrip().endswith(prompt):
> > + break
> > + except TimeoutError:
>
> I like this addition, but maybe we could do better. In the regular SSH
> session, we're expected to raise:
> * SSHSessionDeadError if the session is not alive,
> * SSHTimeoutError if the command execution times out.
>
> Can we do that here as well?
This is a good point, I don't see why we couldn't and thinking about
it I like the idea of making this more consistent, good catch.
>
> > + raise InteractiveCommandExecutionError(
>
> We could just reuse the SSHTimeoutError exception. Is there a reason
> to distinguish between interactive and non-interactive timeouts?
I wanted to add a distinction between interactive and non-interactive
just because in general the way we log messages when sending commands
to interactive shells looks pretty close to what it looks like when
you do the same for non-interactive, so if there was an error I
thought it might be more helpful in the logs to know which session you
were sending the command to when an error was encountered. Maybe,
however, a better approach to this would just be keep the exception
types the same but change the messages.
>
<snip>
> > 2.43.2
> >
next prev parent reply other threads:[~2024-04-08 16:20 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-12 17:25 [PATCH v1 0/2] Improve interactive shell output gathering jspewock
2024-03-12 17:25 ` [PATCH v1 1/2] dts: Improve output gathering in interactive shells jspewock
2024-04-03 9:00 ` Juraj Linkeš
2024-04-08 16:20 ` Jeremy Spewock [this message]
2024-04-10 10:20 ` Juraj Linkeš
2024-03-12 17:25 ` [PATCH v1 2/2] dts: Add missing docstring from XML-RPC server jspewock
2024-04-24 13:42 ` Patrick Robb
2024-05-01 16:16 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging jspewock
2024-05-01 16:16 ` [PATCH v2 1/3] dts: Improve output gathering in interactive shells jspewock
2024-05-09 9:57 ` Luca Vizzarro
2024-05-13 14:58 ` Juraj Linkeš
2024-05-15 19:13 ` Jeremy Spewock
2024-05-01 16:16 ` [PATCH v2 2/3] dts: Add missing docstring from XML-RPC server jspewock
2024-05-09 9:57 ` Luca Vizzarro
2024-05-13 14:58 ` Juraj Linkeš
2024-05-01 16:16 ` [PATCH v2 3/3] dts: Improve logging for interactive shells jspewock
2024-05-09 9:57 ` Luca Vizzarro
2024-05-13 15:02 ` Juraj Linkeš
2024-05-15 19:23 ` Jeremy Spewock
2024-05-09 9:59 ` [PATCH v2 0/3] Improve interactive shell output gathering and logging Luca Vizzarro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAA20USozCrgtAKpSU50mKFVJwUzviCOdFJ7TBDCxkv2=B9nRg@mail.gmail.com' \
--to=jspewock@iol.unh.edu \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Luca.Vizzarro@arm.com \
--cc=dev@dpdk.org \
--cc=juraj.linkes@pantheon.tech \
--cc=paul.szczepanek@arm.com \
--cc=probb@iol.unh.edu \
--cc=thomas@monjalon.net \
--cc=wathsala.vithanage@arm.com \
--cc=yoan.picchi@foss.arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).