On Wed, Apr 3, 2024 at 5:00 AM Juraj Linkeš <juraj.lin...@pantheon.tech> wrote: > > On Tue, Mar 12, 2024 at 6:26 PM <jspew...@iol.unh.edu> wrote: > > > > From: Jeremy Spewock <jspew...@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 > >
