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 61CC84548B; Tue, 18 Jun 2024 11:18:35 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2F26F40E09; Tue, 18 Jun 2024 11:18:35 +0200 (CEST) Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) by mails.dpdk.org (Postfix) with ESMTP id 3B30240684 for ; Tue, 18 Jun 2024 11:18:33 +0200 (CEST) Received: by mail-lf1-f46.google.com with SMTP id 2adb3069b0e04-52c8c0d73d3so5504493e87.1 for ; Tue, 18 Jun 2024 02:18:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1718702312; x=1719307112; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=ruW4P0l+umtRJxW2SvqhZybOPI8KR/u493s2+xNZ5XY=; b=eS1+bTUagiiOi7DfFMsXp8eol/bfuAbA2O97d1A6gJtBPpIpLNvPyGX9LwUu328fFI ydf4UhkdAN+IKjHUhqNkUmggtPdbCoDVSBJXQr+ZtY0afu0gyJeAAJQ2ld2Qlzs5qA2U 2njJeWKkj7e9iypyvm5zuSFvqIiN0MaXWfvyAZevhgMMRZhw2yBON4VAm/2bMg69542i /T578g0eIOvg+HE3UvfyvdwuKWE7wh1SF4urmJ99nFOrv+Gx29nmRqexWkWw999QtxUQ DC6Cl8C9ZQYk8bYhAtLwIGOdCie101UmPIR6aw7xCPG6Yg90Ui5IllS0sDaCDWwy6jkP bdcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718702312; x=1719307112; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ruW4P0l+umtRJxW2SvqhZybOPI8KR/u493s2+xNZ5XY=; b=b7Wpk5e4PXX/9o8kjx9SDXBKbcjjpHFfvqFm9/PThWtnKbh6UXwwprd634SNpox4j/ 5zGpJEjxuFaeMIQ7BV4F/8mg1zg0HlcEX2KDsnwDOlymf12nO6S94D5I9275ppXzYnIt xqCpMzQshv5loqAKNbReFQFmBjWDeiwuqzwagK9piA4FRM6FKYtjJT8G/wUCgp24QNPt wCbEOBrASuUZIteEADqvrFbvPY2cRm2mUDMre0MWS7C+Gck0J35FEdNjyHZYC6KpwN7P OxolqTTlTmCA1dWIMETdhr1gP+NC099rVy4AWQyOFWVg5HIvUSEsTccAnTOq69PFP86O igBA== X-Forwarded-Encrypted: i=1; AJvYcCVG8L0X6/CPOix5cah95jfUF3KWwocnpMTydhUybgfCmq1vCD+PW97n+/IxTNnsFjtkOVy2tG03Xx9I0bs= X-Gm-Message-State: AOJu0YyhFlETgNcjFUlmzg18NV2MjYfaTkgfhw8yzC9zTyVE6P5OIRIx ytDmqRX76zeYVkZH+pF5h6rNZGH6eGBXL1vCuqkvUDqkjqB7fyJsKkBr6FBDLxxMawaAH4BwtCo plG0= X-Google-Smtp-Source: AGHT+IGKL2kDMP8U7w0S/niw3TXdAW5jNzTsddBevbx1NwhOVYiwh9oRe+qWROsAVqWpdwt+cIqdww== X-Received: by 2002:a05:6512:3191:b0:52c:835b:fcb4 with SMTP id 2adb3069b0e04-52cc6ab4a39mr732210e87.69.1718702312510; Tue, 18 Jun 2024 02:18:32 -0700 (PDT) Received: from [10.12.0.137] (81.89.53.154.host.vnet.sk. [81.89.53.154]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3607509c878sm13775561f8f.37.2024.06.18.02.18.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jun 2024 02:18:32 -0700 (PDT) Message-ID: Date: Tue, 18 Jun 2024 11:18:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 7/8] dts: rework interactive shells To: Luca Vizzarro , dev@dpdk.org Cc: Jeremy Spewock , Paul Szczepanek References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240530152505.389167-1-luca.vizzarro@arm.com> <20240530152505.389167-8-luca.vizzarro@arm.com> <4e7b345e-0ad5-4ce4-9c39-7a95fe5770c9@pantheon.tech> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= 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 17. 6. 2024 14:13, Luca Vizzarro wrote: > On 06/06/2024 19:03, Juraj Linkeš wrote: >>> +class DPDKShell(InteractiveShell, ABC): >>> +    """The base class for managing DPDK-based interactive shells. >>> + >>> +    This class shouldn't be instantiated directly, but instead be >>> extended. >>> +    It automatically injects computed EAL parameters based on the >>> node in the >>> +    supplied app parameters. >>> +    """ >>> + >>> +    _node: SutNode >> >> Same here, better to be explicit with _sut_node. > > This should not be changed as it's just overriding the type of the > parent's attribute. > Ah, right, thanks for pointing this out. >>> +    _app_params: EalParams >>> + >>> +    _lcore_filter_specifier: LogicalCoreCount | LogicalCoreList >>> +    _ascending_cores: bool >>> +    _append_prefix_timestamp: bool >>> + >>> +    def __init__( >>> +        self, >>> +        node: SutNode, >>> +        app_params: EalParams, >>> +        privileged: bool = True, >>> +        timeout: float = SETTINGS.timeout, >>> +        lcore_filter_specifier: LogicalCoreCount | LogicalCoreList = >>> LogicalCoreCount(), >>> +        ascending_cores: bool = True, >>> +        append_prefix_timestamp: bool = True, >>> +        start_on_init: bool = True, >>> +    ) -> None: >>> +        """Overrides >>> :meth:`~.interactive_shell.InteractiveShell.__init__`.""" I just noticed this says Overrides, but it should be extends with a brief note on what it's extending the constructor with. >>> +        self._lcore_filter_specifier = lcore_filter_specifier >>> +        self._ascending_cores = ascending_cores >>> +        self._append_prefix_timestamp = append_prefix_timestamp >>> + >>> +        super().__init__(node, app_params, privileged, timeout, >>> start_on_init) >>> + >>> +    def _post_init(self): >>> +        """Computes EAL params based on the node capabilities before >>> start.""" >> >> We could just put this before calling super().__init__() in this class >> if we update path some other way, right? It's probably better to >> override the class method (_update_path()) in subclasses than having >> this _post_init() method. > > It's more complicated than this. The ultimate parent (InteractiveShell) > is what sets all the common attributes. This needs to happen before > compute_eal_params and updating the path with the dpdk folder. This > wouldn't be a problem if we were to call super().__init__ at the > beginning. But that way we'd lose the ability to automatically start the > shell though. Yes, that's why I proposed a solution that takes that into account. :-) If we override _update_path() and change it to a regular method, we can just do: self._lcore_filter_specifier = lcore_filter_specifier self._ascending_cores = ascending_cores self._append_prefix_timestamp = append_prefix_timestamp # Here it's clear we don't need the parent to set the attributes as we have access to them. self._app_params = compute_eal_params( node, app_params, lcore_filter_specifier, ascending_cores, append_prefix_timestamp, ) super().__init__(node, app_params, privileged, timeout, start_on_init) # no longer a class method, it'll create the path instance variable (starting with the value assigned to the path class variable) and we can access self._node def _update_path(self, path: PurePath): """Updates the path.""" self.path = self._node.remote_dpdk_build_dir.joinpath(self.path)) As far as I can tell, this does the same thing without using _post_init and has the added benefit of overriding what we expect the subclasses to override (the path, as that's what should be different across shells).