Thanks for the notes, I will work on followup patch

On Thu, Oct 1, 2020 at 12:53 PM Jan Tluka <jtluka@redhat.com> wrote:
Wed, Sep 30, 2020 at 04:14:51PM CEST, jurbanov@redhat.com wrote:
>From: Jozef Urbanovsky <jurbanov@redhat.com>
>
>Signed-off-by: Jozef Urbanovsky <jurbanov@redhat.com>
>---
> lnst/Recipes/ENRT/XfrmTools.py | 36 ++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
>diff --git a/lnst/Recipes/ENRT/XfrmTools.py b/lnst/Recipes/ENRT/XfrmTools.py
>index fd226fc..789b42d 100644
>--- a/lnst/Recipes/ENRT/XfrmTools.py
>+++ b/lnst/Recipes/ENRT/XfrmTools.py
>@@ -1,12 +1,32 @@
> from lnst.Common.LnstError import LnstError
>
> def generate_key(length):
>+    """
>+    Method generates key suitable for the IPsec.
>+
>+    :param length: Desired length of the generated key
>+    :return: key
>+    """
>     key = "0x"
>     key = key + (length//8) * "0b"
>     return key
>
> def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len,
>                              ipsec_mode, spi_vals):
>+    """
>+    Configuration of the IPsec is executed through iproute2's ip-xfrm
>+    tool. Method creates state and policy for the ESP AEAD IPsec.

Could you swap the two sentences? First I'd start with what the method
is used for, so:
"
This method creates states and policies for the ESP AEAD IPsec tunnel
between two endpoinsts according to the specified parameters. The iproute2's
ip-xfrm is used for the configuration.
"

 
Sure, that sounds reasonable.
 
>+
>+    :param m1: :any:`RemoteDevice` class describing tunnel's endpoint

Actually m1/m2 are Namespace instances that represents a host API for the
user. It is an abstraction (and an extension of Host object) of
the root namespace and any created network namespace in LNST. The
reference to RemoteDevice is incorrect here.

As we talked on IRC, I was describing the real class that the parameter uses, instead of how it is meant and how it is being used.
I will fix this and refer to m1/m2 parameters as "handles for the Host API to describe the endpoint".
 

>+    :param ip1: :any:`Device` class containing IP addresses of given device under test

Both ip1/ip2 are instances of BaseIpAddress, so simply an IPv4/IPv6
address in this case is sufficient IMO.

Ok, I will simplify this.
 

>+    :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint
>+    :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel
>+    :param algo: Algorithm chosen for the tunnel's ESP

While we're at it, I'd rephrase this and others to match following
pattern: "(Encryption algorithm) that will be used for the configuration of (ESP)"

Good point, I'm going to fix it.
 

>+    :param algo_key: Generated key for the encryption algorithm
>+    :param icv_len: Length of ICV for the AEAD encryption protocol
>+    :param ipsec_mode: Mode of the ESP
>+    :param spi_vals: SPI value for the identification
>+    """
>     for m, in1, in2,  in [(m1, ip2, ip1), (m2, ip1, ip2)]:
>         m.run("ip xfrm policy flush")
>         m.run("ip xfrm state flush")
>@@ -37,6 +57,22 @@ def configure_ipsec_esp_aead(m1, ip1, m2, ip2, algo, algo_key, icv_len,
>
> def configure_ipsec_esp_ah_comp(m1, ip1, m2, ip2, ciph_alg, ciph_key, hash_alg,
>                                 hash_key, ipsec_mode, spi_vals):
>+    """
>+    Configuration of the IPsec is executed through iproute2's ip-xfrm
>+    tool. Method creates state and policy for the ESP AH IPsec.

This method includes the IPComp in addition to the ESP/AH.

Same comments as for the aead variant of this method applies.

Ok, I will mention the compression protocol as well.
 

>+
>+    :param m1: :any:`RemoteDevice` class describing tunnel's endpoint
>+    :param ip1: :any:`Device` class containing IP addresses of given device under test
>+    :param m2: :any:`RemoteDevice` class describing other side of tunnel endpoint
>+    :param ip2: :any:`Device` class containing IP addresses of other side of the tunnel
>+    :param ciph_alg: Cipher algorithm chosen for the tunnel's ESP
>+    :param ciph_key: Generated key for the encryption algorithm
>+    :param hash_alg: Algorithm for AH part of IPsec
>+    :param hash_key: Generated key for the authentication protocol
>+    :param icv_len: Length of ICV for the AH encryption protocol
>+    :param ipsec_mode: Mode of the ESP
>+    :param spi_vals: SPI value for the identification
>+    """
>     m_keys = []
>     for m in [m1, m2]:
>         res = m.run("rpm -qa iproute")
>--
>2.25.4
>_______________________________________________
>LNST-developers mailing list -- lnst-developers@lists.fedorahosted.org
>To unsubscribe send an email to lnst-developers-leave@lists.fedorahosted.org
>Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>List Archives: https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedorahosted.org