Re: [PATCH v2 2/5] lnst.NetTestSlave: fix namespace creation/deletion

[Ondrej Lichtner]

On Mon, Apr 19, 2021 at 02:47:38PM +0200, Jan Tluka wrote:
>  Inspired by pyroute2 project.
>  
>  The libc calls require bytestrings as arguments and additional encoding.
>  The patch also adds few return code checks.
>  
>  Signed-off-by: Jan Tluka <jtluka(a)redhat.com&gt;
>  ---
>   lnst/Slave/NetTestSlave.py | 47 +++++++++++++++++++++++++++++---------
>   1 file changed, 36 insertions(+), 11 deletions(-)
>  
>  diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py
>  index 710dbce0..153aad8d 100644
>  --- a/lnst/Slave/NetTestSlave.py
>  +++ b/lnst/Slave/NetTestSlave.py
>  @@ -538,26 +538,50 @@ class SlaveMethods:
>                   MS_BIND = 4096
>                   MS_SLAVE = 1<<19
>                   MS_REC = 16384
>  +                MS_SHARED = 1 << 20
>                   libc = ctypes.CDLL(libc_name)
>   
>                   #based on ipnetns.c from the iproute2 project
>                   #bind to named namespace
>  -                netns_path = "/var/run/netns/"
>  -                if not os.path.exists(netns_path):
>  -                    os.mkdir(netns_path, stat.S_IRWXU | stat.S_IRGRP |
>  +                netns_dir = "/var/run/netns/"
>  +                if not os.path.exists(netns_dir):
>  +                    os.mkdir(netns_dir, stat.S_IRWXU | stat.S_IRGRP |
>                                            stat.S_IXGRP | stat.S_IROTH |
>                                            stat.S_IXOTH)
>  -                netns_path = netns_path + netns
>  -                f = os.open(netns_path, os.O_RDONLY | os.O_CREAT | os.O_EXCL, 0)
>  +
>  +                done = False
>  +                while libc.mount(b'', netns_dir, b'none', MS_SHARED |
> MS_REC, None) != 0: 
what's the purpose of this while loop? it seems like it runs exactly
twice where on the second run it raises an exception? Why not check the
return code of the first libc.mount call?

Second comment, the use of string encoding to bytes is inconsistent in
this patch, this libc.mount call uses `netns_dir` without encoding.

>  +                    if done:
>  +                        raise OSError(ctypes.get_errno(), 'share rundir failed',
> netns)
>  +                    if libc.mount(netns_dir, netns_dir, b'none', MS_BIND |
> MS_REC,
>  +                                  None) != 0: 
netns_dir also used without encoding

>  +                        raise OSError(ctypes.get_errno(), 'mount
> rundir failed', netns)
>  +                    done = True
>  +
>  +                netns_path = netns_dir + netns
>  +                netns_path = netns_path.encode('ascii') 
now netns_path IS encoded

>  +                try:
>  +                    f = os.open(netns_path, os.O_RDONLY | os.O_CREAT | os.O_EXCL, 0)
>  +                except FileExistsError:
>  +                    raise Exception("Network namespace {} already
> exists".format(netns))
>  +
>                   os.close(f)
>  -                libc.unshare(CLONE_NEWNET)
>  -                libc.mount("/proc/self/ns/net", netns_path, "none",
> MS_BIND, 0) 
but this "/proc/self/ns/net" string is not encoded and neither is
"none"?

>  +                if libc.unshare(CLONE_NEWNET) < 0:
>  +                    raise OSError(ctypes.get_errno(), 'unshare failed', netns)
>  +
>  +                if libc.mount(
>  +                        '/proc/self/ns/net'.encode('utf-8'), 
now it is encoded? but with 'utf-8' instead of 'ascii' which was used
before???

Is this intentional? Can you explain these inconsistencies? Can we make
it more consistent so that encoding is used for all the libc.mount calls
and always uses 'ascii' or 'utf-8'?

-Ondrej

>  +                        netns_path,
>  +                        b'none',
>  +                        MS_BIND,
>  +                        None) < 0:
>  +                    raise OSError(ctypes.get_errno(), 'mount failed', netns)
>   
>                   #map network sysfs to new net
>                   libc.unshare(CLONE_NEWNS)
>  -                libc.mount("", "/", "none", MS_SLAVE |
> MS_REC, 0)
>  +                libc.mount(b'', "/", b'none', MS_SLAVE |
> MS_REC, None)
>                   libc.umount2("/sys", MNT_DETACH)
>  -                libc.mount(netns, "/sys", "sysfs", 0, 0)
>  +                libc.mount(netns, "/sys", "sysfs", 0, None)
>   
>                   #set ctl socket to pipe to main netns
>                   self._server_handler.close_s_sock()
>  @@ -588,6 +612,7 @@ class SlaveMethods:
>               libc_name = ctypes.util.find_library("c")
>               libc = ctypes.CDLL(libc_name)
>               netns_path = "/var/run/netns/" + netns
>  +            netns_path = netns_path.encode('ascii')
>   
>               netns_pid = self._net_namespaces[netns]["pid"]
>               os.kill(netns_pid, signal.SIGUSR1)
>  @@ -597,8 +622,8 @@ class SlaveMethods:
>               try:
>                   libc.umount2(netns_path, MNT_DETACH)
>                   os.unlink(netns_path)
>  -            except:
>  -                logging.warning("Unable to remove named namespace %s." %
> netns_path)
>  +            except Exception as e:
>  +                logging.warning("Unable to remove named namespace %s. %s" %
> (netns_path, e))
>   
>               logging.debug("Network namespace %s removed." % netns)
>   
>  -- 
>  2.26.3
>  _______________________________________________
>  LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
>  To unsubscribe send an email to lnst-developers-leave(a)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/[e-mail]...
>  Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure