Re: [PATCH v7 16/21] dts: posix and linux sessions docstring update

[Yoan Picchi <yoan.picchi@foss.arm.com>]

On 11/15/23 13:09, Juraj Linkeš wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   dts/framework/testbed_model/linux_session.py | 63 ++++++++++-----
>   dts/framework/testbed_model/posix_session.py | 81 +++++++++++++++++---
>   2 files changed, 113 insertions(+), 31 deletions(-)
> 
> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
> index f472bb8f0f..279954ff63 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -2,6 +2,13 @@
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2023 University of New Hampshire
>   
> +"""Linux OS translator.
> +
> +Translate OS-unaware calls into Linux calls/utilities. Most of Linux distributions are mostly
> +compliant with POSIX standards, so this module only implements the parts that aren't.
> +This intermediate module implements the common parts of mostly POSIX compliant distributions.
> +"""
> +
>   import json
>   from ipaddress import IPv4Interface, IPv6Interface
>   from typing import TypedDict, Union
> @@ -17,43 +24,51 @@
>   
>   
>   class LshwConfigurationOutput(TypedDict):
> +    """The relevant parts of ``lshw``'s ``configuration`` section."""
> +
> +    #:
>       link: str
>   
>   
>   class LshwOutput(TypedDict):
> -    """
> -    A model of the relevant information from json lshw output, e.g.:
> -    {
> -    ...
> -    "businfo" : "pci@0000:08:00.0",
> -    "logicalname" : "enp8s0",
> -    "version" : "00",
> -    "serial" : "52:54:00:59:e1:ac",
> -    ...
> -    "configuration" : {
> -      ...
> -      "link" : "yes",
> -      ...
> -    },
> -    ...
> +    """A model of the relevant information from ``lshw``'s json output.
> +
> +    e.g.::
> +
> +        {
> +        ...
> +        "businfo" : "pci@0000:08:00.0",
> +        "logicalname" : "enp8s0",
> +        "version" : "00",
> +        "serial" : "52:54:00:59:e1:ac",
> +        ...
> +        "configuration" : {
> +          ...
> +          "link" : "yes",
> +          ...
> +        },
> +        ...
>       """
>   
> +    #:
>       businfo: str
> +    #:
>       logicalname: NotRequired[str]
> +    #:
>       serial: NotRequired[str]
> +    #:
>       configuration: LshwConfigurationOutput
>   
>   
>   class LinuxSession(PosixSession):
> -    """
> -    The implementation of non-Posix compliant parts of Linux remote sessions.
> -    """
> +    """The implementation of non-Posix compliant parts of Linux."""
>   
>       @staticmethod
>       def _get_privileged_command(command: str) -> str:
>           return f"sudo -- sh -c '{command}'"
>   
>       def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]:
> +        """Overrides :meth:`~.os_session.OSSession.get_remote_cpus`."""
>           cpu_info = self.send_command("lscpu -p=CPU,CORE,SOCKET,NODE|grep -v \\#").stdout
>           lcores = []
>           for cpu_line in cpu_info.splitlines():
> @@ -65,18 +80,20 @@ def get_remote_cpus(self, use_first_core: bool) -> list[LogicalCore]:
>           return lcores
>   
>       def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str:
> +        """Overrides :meth:`~.os_session.OSSession.get_dpdk_file_prefix`."""
>           return dpdk_prefix
>   
> -    def setup_hugepages(self, hugepage_amount: int, force_first_numa: bool) -> None:
> +    def setup_hugepages(self, hugepage_count: int, force_first_numa: bool) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.setup_hugepages`."""
>           self._logger.info("Getting Hugepage information.")
>           hugepage_size = self._get_hugepage_size()
>           hugepages_total = self._get_hugepages_total()
>           self._numa_nodes = self._get_numa_nodes()
>   
> -        if force_first_numa or hugepages_total != hugepage_amount:
> +        if force_first_numa or hugepages_total != hugepage_count:
>               # when forcing numa, we need to clear existing hugepages regardless
>               # of size, so they can be moved to the first numa node
> -            self._configure_huge_pages(hugepage_amount, hugepage_size, force_first_numa)
> +            self._configure_huge_pages(hugepage_count, hugepage_size, force_first_numa)
>           else:
>               self._logger.info("Hugepages already configured.")
>           self._mount_huge_pages()
> @@ -140,6 +157,7 @@ def _configure_huge_pages(
>           )
>   
>       def update_ports(self, ports: list[Port]) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.update_ports`."""
>           self._logger.debug("Gathering port info.")
>           for port in ports:
>               assert (
> @@ -178,6 +196,7 @@ def _update_port_attr(
>               )
>   
>       def configure_port_state(self, port: Port, enable: bool) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.configure_port_state`."""
>           state = "up" if enable else "down"
>           self.send_command(
>               f"ip link set dev {port.logical_name} {state}", privileged=True
> @@ -189,6 +208,7 @@ def configure_port_ip_address(
>           port: Port,
>           delete: bool,
>       ) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.configure_port_ip_address`."""
>           command = "del" if delete else "add"
>           self.send_command(
>               f"ip address {command} {address} dev {port.logical_name}",
> @@ -197,5 +217,6 @@ def configure_port_ip_address(
>           )
>   
>       def configure_ipv4_forwarding(self, enable: bool) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.configure_ipv4_forwarding`."""
>           state = 1 if enable else 0
>           self.send_command(f"sysctl -w net.ipv4.ip_forward={state}", privileged=True)
> diff --git a/dts/framework/testbed_model/posix_session.py b/dts/framework/testbed_model/posix_session.py
> index 1d1d5b1b26..a4824aa274 100644
> --- a/dts/framework/testbed_model/posix_session.py
> +++ b/dts/framework/testbed_model/posix_session.py
> @@ -2,6 +2,15 @@
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2023 University of New Hampshire
>   
> +"""POSIX compliant OS translator.
> +
> +Translates OS-unaware calls into POSIX compliant calls/utilities. POSIX is a set of standards
> +for portability between Unix operating systems which not all Linux distributions
> +(or the tools most frequently bundled with said distributions) adhere to. Most of Linux
> +distributions are mostly compliant though.
> +This intermediate module implements the common parts of mostly POSIX compliant distributions.
> +"""
> +
>   import re
>   from collections.abc import Iterable
>   from pathlib import PurePath, PurePosixPath
> @@ -15,13 +24,21 @@
>   
>   
>   class PosixSession(OSSession):
> -    """
> -    An intermediary class implementing the Posix compliant parts of
> -    Linux and other OS remote sessions.
> -    """
> +    """An intermediary class implementing the POSIX standard."""
>   
>       @staticmethod
>       def combine_short_options(**opts: bool) -> str:
> +        """Combine shell options into one argument.
> +
> +        These are options such as ``-x``, ``-v``, ``-f`` which are combined into ``-xvf``.
> +
> +        Args:
> +            opts: The keys are option names (usually one letter) and the bool values indicate
> +                whether to include the option in the resulting argument.
> +
> +        Returns:
> +            The options combined into one argument.
> +        """
>           ret_opts = ""
>           for opt, include in opts.items():
>               if include:
> @@ -33,17 +50,19 @@ def combine_short_options(**opts: bool) -> str:
>           return ret_opts
>   
>       def guess_dpdk_remote_dir(self, remote_dir: str | PurePath) -> PurePosixPath:
> +        """Overrides :meth:`~.os_session.OSSession.guess_dpdk_remote_dir`."""
>           remote_guess = self.join_remote_path(remote_dir, "dpdk-*")
>           result = self.send_command(f"ls -d {remote_guess} | tail -1")
>           return PurePosixPath(result.stdout)
>   
>       def get_remote_tmp_dir(self) -> PurePosixPath:
> +        """Overrides :meth:`~.os_session.OSSession.get_remote_tmp_dir`."""
>           return PurePosixPath("/tmp")
>   
>       def get_dpdk_build_env_vars(self, arch: Architecture) -> dict:
> -        """
> -        Create extra environment variables needed for i686 arch build. Get information
> -        from the node if needed.
> +        """Overrides :meth:`~.os_session.OSSession.get_dpdk_build_env_vars`.
> +
> +        Supported architecture: ``i686``.
>           """
>           env_vars = {}
>           if arch == Architecture.i686:
> @@ -63,6 +82,7 @@ def get_dpdk_build_env_vars(self, arch: Architecture) -> dict:
>           return env_vars
>   
>       def join_remote_path(self, *args: str | PurePath) -> PurePosixPath:
> +        """Overrides :meth:`~.os_session.OSSession.join_remote_path`."""
>           return PurePosixPath(*args)
>   
>       def copy_from(
> @@ -70,6 +90,7 @@ def copy_from(
>           source_file: str | PurePath,
>           destination_file: str | PurePath,
>       ) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.copy_from`."""
>           self.remote_session.copy_from(source_file, destination_file)
>   
>       def copy_to(
> @@ -77,6 +98,7 @@ def copy_to(
>           source_file: str | PurePath,
>           destination_file: str | PurePath,
>       ) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.copy_to`."""
>           self.remote_session.copy_to(source_file, destination_file)
>   
>       def remove_remote_dir(
> @@ -85,6 +107,7 @@ def remove_remote_dir(
>           recursive: bool = True,
>           force: bool = True,
>       ) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.remove_remote_dir`."""
>           opts = PosixSession.combine_short_options(r=recursive, f=force)
>           self.send_command(f"rm{opts} {remote_dir_path}")
>   
> @@ -93,6 +116,7 @@ def extract_remote_tarball(
>           remote_tarball_path: str | PurePath,
>           expected_dir: str | PurePath | None = None,
>       ) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.extract_remote_tarball`."""
>           self.send_command(
>               f"tar xfm {remote_tarball_path} "
>               f"-C {PurePosixPath(remote_tarball_path).parent}",
> @@ -110,6 +134,7 @@ def build_dpdk(
>           rebuild: bool = False,
>           timeout: float = SETTINGS.compile_timeout,
>       ) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.build_dpdk`."""
>           try:
>               if rebuild:
>                   # reconfigure, then build
> @@ -140,12 +165,14 @@ def build_dpdk(
>               raise DPDKBuildError(f"DPDK build failed when doing '{e.command}'.")
>   
>       def get_dpdk_version(self, build_dir: str | PurePath) -> str:
> +        """Overrides :meth:`~.os_session.OSSession.get_dpdk_version`."""
>           out = self.send_command(
>               f"cat {self.join_remote_path(build_dir, 'VERSION')}", verify=True
>           )
>           return out.stdout
>   
>       def kill_cleanup_dpdk_apps(self, dpdk_prefix_list: Iterable[str]) -> None:
> +        """Overrides :meth:`~.os_session.OSSession.kill_cleanup_dpdk_apps`."""
>           self._logger.info("Cleaning up DPDK apps.")
>           dpdk_runtime_dirs = self._get_dpdk_runtime_dirs(dpdk_prefix_list)
>           if dpdk_runtime_dirs:
> @@ -159,6 +186,14 @@ def kill_cleanup_dpdk_apps(self, dpdk_prefix_list: Iterable[str]) -> None:
>       def _get_dpdk_runtime_dirs(
>           self, dpdk_prefix_list: Iterable[str]
>       ) -> list[PurePosixPath]:
> +        """Find runtime directories DPDK apps are currently using.
> +
> +        Args:
> +              dpdk_prefix_list: The prefixes DPDK apps were started with.
> +
> +        Returns:
> +            The paths of DPDK apps' runtime dirs.
> +        """
>           prefix = PurePosixPath("/var", "run", "dpdk")
>           if not dpdk_prefix_list:
>               remote_prefixes = self._list_remote_dirs(prefix)
> @@ -170,9 +205,13 @@ def _get_dpdk_runtime_dirs(
>           return [PurePosixPath(prefix, dpdk_prefix) for dpdk_prefix in dpdk_prefix_list]
>   
>       def _list_remote_dirs(self, remote_path: str | PurePath) -> list[str] | None:
> -        """
> -        Return a list of directories of the remote_dir.
> -        If remote_path doesn't exist, return None.
> +        """Contents of remote_path.
> +
> +        Args:
> +            remote_path: List the contents of this path.
> +
> +        Returns:
> +            The contents of remote_path. If remote_path doesn't exist, return None.
>           """
>           out = self.send_command(
>               f"ls -l {remote_path} | awk '/^d/ {{print $NF}}'"
> @@ -183,6 +222,17 @@ def _list_remote_dirs(self, remote_path: str | PurePath) -> list[str] | None:
>               return out.splitlines()
>   
>       def _get_dpdk_pids(self, dpdk_runtime_dirs: Iterable[str | PurePath]) -> list[int]:
> +        """Find PIDs of running DPDK apps.
> +
> +        Look at each "config" file found in dpdk_runtime_dirs and find the PIDs of processes
> +        that opened those file.
> +
> +        Args:
> +            dpdk_runtime_dirs: The paths of DPDK apps' runtime dirs.
> +
> +        Returns:
> +            The PIDs of running DPDK apps.
> +        """
>           pids = []
>           pid_regex = r"p(\d+)"
>           for dpdk_runtime_dir in dpdk_runtime_dirs:
> @@ -203,6 +253,14 @@ def _remote_files_exists(self, remote_path: PurePath) -> bool:
>       def _check_dpdk_hugepages(
>           self, dpdk_runtime_dirs: Iterable[str | PurePath]
>       ) -> None:
> +        """Check there aren't any leftover hugepages.
> +
> +        If any hugegapes are found, emit a warning. The hugepages are investigated in the

hugegapes -> hugepages

> +        "hugepage_info" file of dpdk_runtime_dirs.
> +
> +        Args:
> +            dpdk_runtime_dirs: The paths of DPDK apps' runtime dirs.
> +        """
>           for dpdk_runtime_dir in dpdk_runtime_dirs:
>               hugepage_info = PurePosixPath(dpdk_runtime_dir, "hugepage_info")
>               if self._remote_files_exists(hugepage_info):
> @@ -220,9 +278,11 @@ def _remove_dpdk_runtime_dirs(
>               self.remove_remote_dir(dpdk_runtime_dir)
>   
>       def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str:
> +        """Overrides :meth:`~.os_session.OSSession.get_dpdk_file_prefix`."""
>           return ""
>   
>       def get_compiler_version(self, compiler_name: str) -> str:
> +        """Overrides :meth:`~.os_session.OSSession.get_compiler_version`."""
>           match compiler_name:
>               case "gcc":
>                   return self.send_command(
> @@ -240,6 +300,7 @@ def get_compiler_version(self, compiler_name: str) -> str:
>                   raise ValueError(f"Unknown compiler {compiler_name}")
>   
>       def get_node_info(self) -> NodeInfo:
> +        """Overrides :meth:`~.os_session.OSSession.get_node_info`."""
>           os_release_info = self.send_command(
>               "awk -F= '$1 ~ /^NAME$|^VERSION$/ {print $2}' /etc/os-release",
>               SETTINGS.timeout,