Re: [PATCH v3 7/8] dts: rework interactive shells

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

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.

>> +    _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__`."""
>> +        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.