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 772E243E33; Wed, 10 Apr 2024 13:37:48 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6644C402EB; Wed, 10 Apr 2024 13:37:48 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id E8DF7402C7 for ; Wed, 10 Apr 2024 13:37:46 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 564DF139F; Wed, 10 Apr 2024 04:38:16 -0700 (PDT) Received: from [10.1.32.34] (FVFG51LCQ05N.cambridge.arm.com [10.1.32.34]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E12763F6C4; Wed, 10 Apr 2024 04:37:45 -0700 (PDT) Message-ID: Date: Wed, 10 Apr 2024 12:37:44 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 6/6] dts: add statefulness to TestPmdShell Content-Language: en-GB To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-7-luca.vizzarro@arm.com> From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 On 10/04/2024 08:50, Juraj Linkeš wrote: > On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro wrote: >> >> This commit provides a state container for TestPmdShell. It currently >> only indicates whether the packet forwarding has started >> or not, and the number of ports which were given to the shell. >> > > A reminder, the commit message should explain why we're doing this > change, not what the change is. > >> This also fixes the behaviour of `wait_link_status_up` to use the >> command timeout as inherited from InteractiveShell. >> >> Signed-off-by: Luca Vizzarro >> Reviewed-by: Jack Bond-Preston >> Reviewed-by: Honnappa Nagarahalli >> --- >> dts/framework/remote_session/testpmd_shell.py | 41 +++++++++++++------ >> 1 file changed, 28 insertions(+), 13 deletions(-) >> >> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py >> index a823dc53be..ea1d254f86 100644 >> --- a/dts/framework/remote_session/testpmd_shell.py >> +++ b/dts/framework/remote_session/testpmd_shell.py >> @@ -678,19 +678,27 @@ def __str__(self) -> str: >> return self.pci_address >> >> >> +@dataclass(slots=True) >> +class TestPmdState: >> + """Session state container.""" >> + >> + #: >> + packet_forwarding_started: bool = False > > The same question as in the previous patch, do you anticipate this > being needed and should we add this only when it's actually used? > As answered in the previous patch. We can always drop it and do it as needed of course. >> + >> + #: The number of ports which were allowed on the command-line when testpmd was started. >> + number_of_ports: int = 0 >> + >> + >> class TestPmdShell(InteractiveShell): >> """Testpmd interactive shell. >> >> The testpmd shell users should never use >> the :meth:`~.interactive_shell.InteractiveShell.send_command` method directly, but rather >> call specialized methods. If there isn't one that satisfies a need, it should be added. >> - >> - Attributes: >> - number_of_ports: The number of ports which were allowed on the command-line when testpmd >> - was started. >> """ >> >> - number_of_ports: int >> + #: Current state >> + state: TestPmdState = TestPmdState() > > Assigning a value makes this a class variable, shared across all > instances. This should be initialized in __init__(). > > But do we actually want to do this via composition? We'd need to > access the attributes via .state all the time and I don't really like > that. We could just put them into TestPmdShell directly, initializing > them in __init__(). No problem. I separated them in fear of bloating TestPmdShell. But I agree on the bother of adding .state >> >> #: The path to the testpmd executable. >> path: ClassVar[PurePath] = PurePath("app", "dpdk-testpmd")