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 BABFA43E33; Wed, 10 Apr 2024 13:35:41 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3BA54402CF; Wed, 10 Apr 2024 13:35:41 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 08940402C7 for ; Wed, 10 Apr 2024 13:35:40 +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 4B08D1480; Wed, 10 Apr 2024 04:36:09 -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 BA6703F6C4; Wed, 10 Apr 2024 04:35:38 -0700 (PDT) Message-ID: <1db1b2b8-fac0-41ad-9ba2-911365385a9b@arm.com> Date: Wed, 10 Apr 2024 12:35:37 +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?= , Jeremy Spewock 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:41, Juraj Linkeš wrote: >> >>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None >>> if self._app_args.app_params is None: >>> self._app_args.app_params = TestPmdParameters() >>> >>> - self.number_of_ports = len(self._app_args.ports) if self._app_args.ports is not None else 0 >>> + assert isinstance(self._app_args.app_params, TestPmdParameters) >>> + >> >> This is tricky because ideally we wouldn't have the assertion here, >> but I understand why it is needed because Eal parameters have app args >> which can be any instance of params. I'm not sure of the best way to >> solve this, because making testpmd parameters extend from eal would >> break the general scheme that you have in place, and having an >> extension of EalParameters that enforces this app_args is >> TestPmdParameters would solve the issues, but might be a little >> clunky. Is there a way we can use a generic to get python to just >> understand that, in this case, this will always be TestPmdParameters? >> If not I might prefer making a private class where this is >> TestPmdParameters, just because there aren't really any other >> assertions that we use elsewhere and an unexpected exception from this >> (even though I don't think that can happen) could cause people some >> issues. >> >> It might be the case that an assertion is the easiest way to deal with >> it though, what do you think? >> > > We could change the signature (just the type of app_args) of the init > method - I think we should be able to create a type that's > EalParameters with .app_params being TestPmdParameters or None. The > init method would just call super(). > > Something like the above is basically necessary with inheritance where > subclasses are all extensions (not just implementations) of the > superclass (having differences in API). > I believe this is indeed a tricky one. But, unfortunately, I am not understanding the solution that is being proposed. To me, it just feels like using a generic factory like: self.sut_node.create_interactive_shell(..) is one of the reasons to bring in the majority of these complexities. What do you mean by creating this new type that combines EalParams and TestPmdParams?