Re: [PATCH v4 5/9] dts: add ssh connection extension

[Owen Hilyard <ohilyard@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

>
> On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> > The class adds logging and history records to existing pexpect methods.
> >
> > Signed-off-by: Owen Hilyard <ohilyard@iol.unh.edu>
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> >  dts/framework/ssh_connection.py | 70 +++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 dts/framework/ssh_connection.py
> >


> One comment inline below.


> /Bruce

Two questions on this function:

* Is the getattr() check not equivalent to "if self.logger:"?


It is. I missed it when looking over this code. I know that this close
function can run in a context where it loses the ability to make system
calls (an exit hook), but that doesn't matter for this as far as I know.


> * Why the check for a non-none logger in this function, when other

  functions above always seem to call the logger directly without checking?


"close" can be called before "init_log" if the program crashes early
enough, so this is avoiding calling a function on a null object. No other
function can have that issue because by the time control is returned to the
user the logger is properly initalized. This is especially important
because an early failure in the community lab will only be able to use logs
to figure out what happened.


>
> > +
> > +        self.session.close(force)
> > --

> 2.30.2
> >

[-- Attachment #2: Type: text/html, Size: 4449 bytes --]