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 4E35A43E2B; Tue, 9 Apr 2024 16:56:39 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DA911402C7; Tue, 9 Apr 2024 16:56:38 +0200 (CEST) Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) by mails.dpdk.org (Postfix) with ESMTP id 7DAFB402C6 for ; Tue, 9 Apr 2024 16:56:36 +0200 (CEST) Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-56e509baddaso2488441a12.1 for ; Tue, 09 Apr 2024 07:56:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712674596; x=1713279396; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=YX1XHvtbPzr9iKl+sePt71LPCGB3GghOzveBTOZgJ/Y=; b=Fntx2ujm/4XFv+2QJiPqGx8fV7I70Ze/sN91J1VqwGZf9ljUFnoNTAZeiJpV/goOVw 24nMWR0lsDmIY/uMfzqlLV5S19t9AiO+6rH8h/bTipDXnKioSa4PeyG44BxSgdx8kZSc VtwG1DWwUCZCmGcdcnQFCsRB9+rgnCTGAVNDJaVPUBUVMyxlDT96GTlSjW1G8L2My7QD z/rvHS9FFn2/2M0kLBjDQuqLyJ8+lPD3aUY0WkP0xmTC2vgYGwWTd/JNugOEJ/lyl3/V Yejn/2sICUfsSLfEuAxbdxrvvp76vPRKtyajwlKo8dVd0wo8X/+QkZZ9OFZ2r0BX0Ol3 RkLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712674596; x=1713279396; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=YX1XHvtbPzr9iKl+sePt71LPCGB3GghOzveBTOZgJ/Y=; b=oanpj4gT6j2tFjzabWCt1o2R3unSPkvOOumDPlKP9ceCxtYVchz0km6ApAMzuw+9D0 YeTeBcbarIe9HxCEdWH0G3m6QulHLOqTxbcmV7HqOQNmKoyX6OPA/RbM6EnI/5kekcz0 teHJs7mM4KFvhnkHfD1quM2rKmaXJX8Z0BIpFBxFyB5aOHsQyeX8I78VH8muAoNeSy/x r5cbiWDfkljBsn6We2cGkZ5sFXWuKfo9+E8nTZoGudPFKGQ4WoS/1h//tCsbPGUktGie wyuUZtdQKJm/KU6KsD9NZGGal4exX/pt2UIUrGW6gyXKV7LNAMrcwVMfse/VKB8EjqwL 90gA== X-Forwarded-Encrypted: i=1; AJvYcCXdPM0sMi5axiSvqgiKgPTKTFDWnu7dfeRErYq8fE9Xu7GUkE0ClnTC/916214Lzh40bmK67om0Re98eA8= X-Gm-Message-State: AOJu0Yyq37ZmHZoDQOeitmdFNAemkDj5nCYNpiJnjT3ekneWSWEvI558 kkLlhZ0hNoVXU5TqDRzbK4jOIjrIB0RZUnEa0XwPjtUEcMAA/Pc46EM/tEhmphTx4iq5EV1FBN/ KKb7mW30/h1NBamdz/1LPSnjNn+nbaZIXKuXifg== X-Google-Smtp-Source: AGHT+IF0B3xWykoavzFVvWJ26K2PJph7zPghEPIGIDn71B4NzIUGPknF7QdkoWb8SpyO6s5wwQEhBFtZ0ik0zoRbXyg= X-Received: by 2002:a50:d7d1:0:b0:568:a05e:eafd with SMTP id m17-20020a50d7d1000000b00568a05eeafdmr8735054edj.39.1712674596154; Tue, 09 Apr 2024 07:56:36 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-3-luca.vizzarro@arm.com> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Tue, 9 Apr 2024 16:56:25 +0200 Message-ID: Subject: Re: [PATCH 2/6] dts: use Params for interactive shells To: Jeremy Spewock Cc: Luca Vizzarro , dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Thu, Mar 28, 2024 at 5:48=E2=80=AFPM Jeremy Spewock wrote: > > On Tue, Mar 26, 2024 at 3:04=E2=80=AFPM Luca Vizzarro wrote: > > > > Make it so that interactive shells accept an implementation of `Params` > > for app arguments. Convert EalParameters to use `Params` instead. > > > > String command line parameters can still be supplied by using the > > `StrParams` implementation. > > > > Signed-off-by: Luca Vizzarro > > Reviewed-by: Jack Bond-Preston > > Reviewed-by: Honnappa Nagarahalli > > --- > > > @@ -40,7 +42,7 @@ class InteractiveShell(ABC): > > _ssh_channel: Channel > > _logger: DTSLogger > > _timeout: float > > - _app_args: str > > + _app_args: Params | None > > > > I'm not sure if allowing None should be the solution for these shells > as opposed to just supplying an empty parameter object. Maybe > something that could be done is the factory method in sut_node allows > it to be None, but when it comes time to make the abstract shell it > creates an empty one if it doesn't exist, or the init method makes > this an optional parameter but creates it if it doesn't exist. > > I suppose this logic would have to exist somewhere because the > parameters aren't always required, it's just a question of where we > should put it and if we should just assume that the interactive shell > class just knows how to accept some parameters and put them into the > shell. I would maybe leave this as something that cannot be None and > handle it outside of the shell, but I'm not sure it's something really > required and I don't have a super strong opinion on it. > I think this is an excellent idea. An empty Params (or StrParams or EalParams if we want to make Params truly abstract and thus not instantiable) would work as the default value and it would be an elegant solution since dev will only pass non-empty Params. > > #: Prompt to expect at the end of output when sending a command. > > #: This is often overridden by subclasses. > > > @@ -118,8 +119,15 @@ def _start_application(self, get_privileged_comman= d: Callable[[str], str] | None > > Also find the number of pci addresses which were allowed on th= e command line when the app > > was started. > > """ > > - self._app_args +=3D " -i --mask-event intr_lsc" > > - self.number_of_ports =3D self._app_args.count("-a ") > > + from framework.testbed_model.sut_node import EalParameters > > + > > + assert isinstance(self._app_args, EalParameters) > > + > > + if isinstance(self._app_args.app_params, StrParams): > > + self._app_args.app_params.value +=3D " -i --mask-event int= r_lsc" > > + > > + self.number_of_ports =3D len(self._app_args.ports) if self._ap= p_args.ports is not None else 0 > > I we should override the _app_args parameter in the testpmd shell to > always be EalParameters instead of doing this import and assertion. > It's a DPDK app, so we will always need EalParameters anyway, so we > might as well put that as our typehint to start off as what we expect > instead. > > The checking of an instance of StrParams also feels a little strange > here, it might be more ideal if we could just add the parameters > without this check. Maybe something we can do, just because these > parameters are meant to be CLI commands anyway and will be rendered as > a string, is replace the StrParams object with a method on the base > Params dataclass that allows you to just add any string as a value > only field. Then, we don't have to bother validating anything about > the app parameters and we don't care what they are, we can just add a > string to them of new parameters. > > I think this is something that likely also gets solved when you > replace this with testpmd parameters, but it might be a good way to > make the Params object more versatile in general so that people can > diverge and add their own flags to it if needed. > I'd say this is done because of circular imports. If so, we could move EalParameters to params.py, that should help. And when we're at it, either rename it to EalParams or rename the other classes to use the whole word. > > + > > super()._start_application(get_privileged_command) > > > > def start(self, verify: bool =3D True) -> None: > > > @@ -134,7 +136,7 @@ def create_interactive_shell( > > shell_cls: Type[InteractiveShellType], > > timeout: float, > > privileged: bool, > > - app_args: str, > > + app_args: Params | None, > > This also falls in line with what I was saying before about where this > logic is handled. This was made to always be a string originally > because by this point it being optional was already handled by the > sut_node.create_interactive_shell() and we should have some kind of > value here (even if that value is an empty parameters dataclass) to > pass into the application. It sort of semantically doesn't really > change much, but at this point it might as well not be None, so we can > simplify this type. > > > ) -> InteractiveShellType: > > """Factory for interactive session handlers. > > > > > +@dataclass(kw_only=3DTrue) > > +class EalParameters(Params): > > """The environment abstraction layer parameters. > > > > The string representation can be created by converting the instanc= e to a string. > > """ > > > > - def __init__( > > - self, > > - lcore_list: LogicalCoreList, > > - memory_channels: int, > > - prefix: str, > > - no_pci: bool, > > - vdevs: list[VirtualDevice], > > - ports: list[Port], > > - other_eal_param: str, > > - ): > > - """Initialize the parameters according to inputs. > > - > > - Process the parameters into the format used on the command lin= e. > > + lcore_list: LogicalCoreList =3D field(metadata=3Dparams.short("l")= ) > > + """The list of logical cores to use.""" > > > > - Args: > > - lcore_list: The list of logical cores to use. > > - memory_channels: The number of memory channels to use. > > - prefix: Set the file prefix string with which to start DPD= K, e.g.: ``prefix=3D'vf'``. > > - no_pci: Switch to disable PCI bus e.g.: ``no_pci=3DTrue``. > > - vdevs: Virtual devices, e.g.:: > > + memory_channels: int =3D field(metadata=3Dparams.short("n")) > > + """The number of memory channels to use.""" > > > > - vdevs=3D[ > > - VirtualDevice('net_ring0'), > > - VirtualDevice('net_ring1') > > - ] > > - ports: The list of ports to allow. > > - other_eal_param: user defined DPDK EAL parameters, e.g.: > > - ``other_eal_param=3D'--single-file-segments'`` > > - """ > > - self._lcore_list =3D f"-l {lcore_list}" > > - self._memory_channels =3D f"-n {memory_channels}" > > - self._prefix =3D prefix > > - if prefix: > > - self._prefix =3D f"--file-prefix=3D{prefix}" > > - self._no_pci =3D "--no-pci" if no_pci else "" > > - self._vdevs =3D " ".join(f"--vdev {vdev}" for vdev in vdevs) > > - self._ports =3D " ".join(f"-a {port.pci}" for port in ports) > > - self._other_eal_param =3D other_eal_param > > - > > - def __str__(self) -> str: > > - """Create the EAL string.""" > > - return ( > > - f"{self._lcore_list} " > > - f"{self._memory_channels} " > > - f"{self._prefix} " > > - f"{self._no_pci} " > > - f"{self._vdevs} " > > - f"{self._ports} " > > - f"{self._other_eal_param}" > > - ) > > + prefix: str =3D field(metadata=3Dparams.long("file-prefix")) > > + """Set the file prefix string with which to start DPDK, e.g.: ``pr= efix=3D"vf"``.""" > > Just a small note on docstrings, I believe generally in DTS we use the > google docstring > (https://google.github.io/styleguide/pyguide.html#38-comments-and-docstri= ngs) > format where it applies with some small differences. Because these > attributes aren't class vars however, I believe they should be in the > docstring for the class in the `Attributes` section. I generally have > trouble remembering exactly how it should look, but Juraj documented > it in `doc/guides/tools/dts.rst` > I noticed this right away. Here's the bullet point that applies: * The ``dataclass.dataclass`` decorator changes how the attributes are processed. The dataclass attributes which result in instance variables/attributes should also be recorded in the ``Attributes:`` section. > > + > > + no_pci: params.Option > > + """Switch to disable PCI bus e.g.: ``no_pci=3DTrue``.""" > > > > -- > > 2.34.1 > >