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 CC00EA0032; Wed, 14 Sep 2022 14:03:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 634FA4021D; Wed, 14 Sep 2022 14:03:16 +0200 (CEST) Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) by mails.dpdk.org (Postfix) with ESMTP id CF35A40156 for ; Wed, 14 Sep 2022 14:03:14 +0200 (CEST) Received: by mail-pj1-f43.google.com with SMTP id ge9so2830761pjb.1 for ; Wed, 14 Sep 2022 05:03:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=FSRuW9xU9ghy9BnEBMddtB36GpRTsNzpBg3UwVlcaqY=; b=Bb6Vf67yjiZMFqEiQz9IhrhWc7nmOCJzFnQrzznNxmAWbdfNa0ceBDuUlnwq7IZQTm d+YqhM8PGecdJSNjK2sdeD0jXGebd9focKk+eOlb9OFdmbXdNZUghhxmpri9DIOix+0g v1Ne+3UZt27sGuh9BuRvClhT0Dd7YrOUDBc2I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=FSRuW9xU9ghy9BnEBMddtB36GpRTsNzpBg3UwVlcaqY=; b=CThMBxrbf2tWpp+C4pOENnIv6pwSEMA7sqp11ebrFT7em/5yGPZYWzhH7Y/8LCz/ca v0tBFTg1PnsCw/0NXT+fmBzFUCstkSCQubhNswP7O+AeqIKJGpBttEMZKUVMp2fXBLH9 4JMnjftQQC4OY/R0kJ89MrNU2p0koSNI2uTjvwjkXCpDavC25TQFhE0Poy1UguLmX8ck bZg4lEZDgMfMHETul3H07T1Ic2RAGTqPInTHLLVTSMem6xLs05IYaXwRJ+oB850mWRlz 9gQ52K9Ngt6SmfGG+hrpbx33ptb/W4vBkbjEO1Uig1d2z5xfJHxn2ShCsGV7OH597ai+ nwbw== X-Gm-Message-State: ACrzQf28Mb4Di9Rxa0gloqRI/4s4xiSlPpn6AzSO7O+wwNfML0P4/Nfc cOTYMFz6UeBf8nn892gAKXBRNtRMaDbiJjYsiZeUEw== X-Google-Smtp-Source: AMsMyM70U2yqyzzP8SBdldvXdnV8aM+evoHd7QdAn7lZjXlSVruyJa40DI7mPfZwPCjaNKc5QoLepXuMclw2Sy8EJtw= X-Received: by 2002:a17:90b:264a:b0:1fd:f88d:dbad with SMTP id pa10-20020a17090b264a00b001fdf88ddbadmr4496216pjb.93.1663156993805; Wed, 14 Sep 2022 05:03:13 -0700 (PDT) MIME-Version: 1.0 References: <20220728100044.1318484-1-juraj.linkes@pantheon.tech> <20220729105550.1382664-1-juraj.linkes@pantheon.tech> <20220729105550.1382664-6-juraj.linkes@pantheon.tech> In-Reply-To: From: Owen Hilyard Date: Wed, 14 Sep 2022 08:02:37 -0400 Message-ID: Subject: Re: [PATCH v4 5/9] dts: add ssh connection extension To: Bruce Richardson Cc: =?UTF-8?Q?Juraj_Linke=C5=A1?= , Thomas Monjalon , David Marchand , ronan.randles@intel.com, Honnappa Nagarahalli , "Tu, Lijuan" , dev Content-Type: multipart/alternative; boundary="000000000000d2695805e8a1e90d" 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 --000000000000d2695805e8a1e90d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable > > On Wed, Sep 14, 2022 at 3:46 AM Bruce Richardson < > bruce.richardson@intel.com> wrote: > > On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linke=C5=A1 wrote: > > > The class adds logging and history records to existing pexpect > > methods. > > > > > > Signed-off-by: Owen Hilyard <[1]ohilyard@iol.unh.edu> > > > Signed-off-by: Juraj Linke=C5=A1 > > > --- > > > 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 clo= se > > 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. > > > I'm a little confused by that explanation - can you perhaps clarify? This > close function in part of an class, and the logger member is assigned its > value > in the constructor for that class, so how is it possible to call > obj.close() before obj has been constructed? Due to how we are forced to add the hooks to flush logger buffers to disk before shutdown, even in the case of failures or SIGTERM, close can run WHILE the constructor is in the middle of executing. All of the resources except for the logger can be freed by python, but the logger requires special handling, so we check if it is null and then flush the buffers to disk if it is not. The actual logger object is only assigned to self.logger after it is completely initalized, so if it's not null, then it is in a good state and can be safely flushed. Here's a sequence of events that would lead to that check being needed: 1. Start constructing logging handle 2. Execute first line of the constructor 3. SIGTERM self.logger =3D=3D None, since the entire world stops and moves to the clea= nup functions registered with the interpreter. If we don't do this check, then the cleanup context crashes in this case. --000000000000d2695805e8a1e90d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Wed, = Sep 14, 2022 at 3:46 AM Bruce Richardson <bruce.richardson@intel.com> wrote:
>=C2=A0 =C2=A0 =C2=A0 On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Li= nke=C5=A1 wrote:
>=C2=A0 =C2=A0 =C2=A0 > The class adds logging and history records to= existing pexpect
>=C2=A0 =C2=A0 =C2=A0 methods.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > Signed-off-by: Owen Hilyard <[1]ohilyard@iol.unh.edu>= ;
>=C2=A0 =C2=A0 =C2=A0 > Signed-off-by: Juraj Linke=C5=A1 <juraj.li= nkes@pantheon.tech>
>=C2=A0 =C2=A0 =C2=A0 > ---
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 dts/framework/ssh_connection.py | 70 >=C2=A0 =C2=A0 =C2=A0 +++++++++++++++++++++++++++++++++
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 1 file changed, 70 insertions(+)
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 create mode 100644 dts/framework/ssh_co= nnection.py
>=C2=A0 =C2=A0 =C2=A0 >
>
>=C2=A0 =C2=A0 =C2=A0 One comment inline below.
>
>=C2=A0 =C2=A0 =C2=A0 /Bruce
>
>=C2=A0 =C2=A0 =C2=A0 Two questions on this function:
>
>=C2=A0 =C2=A0 =C2=A0 * Is the getattr() check not equivalent to "i= f self.logger:"?
>
>=C2=A0 =C2=A0 It is. I missed it when looking over this code. I know th= at this close
>=C2=A0 =C2=A0 function can run in a context where it loses the ability = to make system
>=C2=A0 =C2=A0 calls (an exit hook), but that doesn't matter for thi= s as far as I
>=C2=A0 =C2=A0 know.
>
>=C2=A0 =C2=A0 =C2=A0 * Why the check for a non-none logger in this func= tion, when other
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 functions above always seem to call the log= ger directly without
>=C2=A0 =C2=A0 =C2=A0 checking?
>
>=C2=A0 =C2=A0 "close" can be called before "init_log&quo= t; if the program crashes early
>=C2=A0 =C2=A0 enough, so this is avoiding calling a function on a null = object. No
>=C2=A0 =C2=A0 other function can have that issue because by the time co= ntrol is
>=C2=A0 =C2=A0 returned to the user the logger is properly initalized. T= his is
>=C2=A0 =C2=A0 especially important because an early failure in the comm= unity lab will
>=C2=A0 =C2=A0 only be able to use logs to figure out what happened.
>
I'm a little confused by that explanation - can you perhaps clarify? Th= is
close function in part of an class, and the logger member is assigned its v= alue
in the constructor for that class, so how is it possible to call
obj.close() before obj has been constructed?

Due to ho= w we are forced to add the hooks to flush logger buffers to disk before shu= tdown, even in the case of failures or SIGTERM, close can run WHILE the con= structor is in the middle of executing. All of the resources except for the= logger can be freed by python, but the logger requires special handling, s= o we check if it is null and then flush the buffers to disk if it is not. T= he actual logger object is only assigned to self.logger after it is complet= ely initalized, so if it's not null, then it is in a good state and can= be safely flushed. Here's a sequence of events that would lead to that= check being needed:

1. Start constructing logging handle
= 2. Execute first line of the constructor
3. SIGTERM

self.logger = =3D=3D None, since the entire world stops and moves to the cleanup function= s registered with the interpreter. If we don't do this check, then the = cleanup context crashes in this case.=C2=A0
--000000000000d2695805e8a1e90d--