Re: [PATCH v1 1/2] dts: Improve output gathering in interactive shells

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Mon, Apr 8, 2024 at 6:20 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
> 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?

Would this work? I'm thinking we may need to send another extra
newline character to put the solitary prompt into the buffer if the
command output doesn't contain a newline.

> 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.
>

If it works with checking just the end of the line let's leave it like
this (because of the possibility above). I think there shouldn't be
any prompts without something after them in docstrings.

> >
> <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.
>

There is probably some value to distinguish between them. We could
just mirror the non-interactive exceptions and keep the messages the
same.

> >
> <snip>
> > > 2.43.2
> > >