[CRIU] [PATCH] unix: sysctl -- Preserve max_dgram_qlen value

[Pavel Tikhomirov]

чт, 31 окт. 2019 г. в 12:20, Alexander Mikhalitsyn 
<alexander.mikhalitsyn at virtuozzo.com>:

 >The /proc/sys/net/unix/max_dgram_qlen is a per-net variable and
 >we already noticed that systemd inside a container may change its value
 >(for example it sets it to 512 by now instead of kernel's default
 >value 10), thus we need keep it inside image and restore then.

 >Based-on-patch-by: Cyrill Gorcunov <gorcunov at gmail.com>
 >Signed-off-by: Alexander Mikhalitsyn <alexander at mihalicyn.com>
 >Signed-off-by: Alexander Mikhalitsyn <alexander.mikhalitsyn at virtuozzo.com>
 >---
 > criu/net.c                             | 102 ++++++++++++++++++++++++-
 > images/netdev.proto                    |   1 +
 > test/zdtm/static/Makefile              |   1 +
 > test/zdtm/static/netns_sub_sysctl.c    |  62 +++++++++++++++
 > test/zdtm/static/netns_sub_sysctl.desc |   5 ++
 > 5 files changed, 170 insertions(+), 1 deletion(-)
 > create mode 100644 test/zdtm/static/netns_sub_sysctl.c
 > create mode 100644 test/zdtm/static/netns_sub_sysctl.des

Better split all test/zdtm/... staff to a separate patch. Look on the 
git/mail history, we always send tests as a separate letters(patches). 
In the same series, but separate from criu related letters(patches).

 >diff --git a/test/zdtm/static/Makefile b/test/zdtm/static/Makefile
 >index a38482f4..887bf4ac 100644
 >--- a/test/zdtm/static/Makefile
 >+++ b/test/zdtm/static/Makefile
 >@@ -207,6 +207,7 @@ TST_NOFILE  :=                              \
 >                pipe03                          \
 >                netns_sub                       \
 >                netns_sub_veth                  \
 >+               netns_sub_sysctl        \
 >                unlink_multiple_largefiles      \
 >                config_inotify_irmap            \
 >                thp_disable                     \
 >diff --git a/test/zdtm/static/netns_sub_sysctl.c 
b/test/zdtm/static/netns_sub_sysctl.c
 >new file mode 100644
 >index 00000000..ea84cd30
 >--- /dev/null
 >+++ b/test/zdtm/static/netns_sub_sysctl.c
 >@@ -0,0 +1,62 @@
 >+#include <sched.h>
 >+#include <errno.h>
 >+#include <stdlib.h>
 >+#include <string.h>
 >+#include <unistd.h>
 >+#include <sys/types.h>
 >+#include <sys/wait.h>
 >+#include <sys/un.h>
 >+#include <sys/socket.h>
 >+#include <sys/types.h>
 >+#include <sys/stat.h>
 >+#include <fcntl.h>
 >+#include <stdbool.h>

Looks like not all of them are realy needed, e.g. why you need socket.h?

 >+
 >+#include "zdtmtst.h"
 >+
 >+const char *test_doc   = "Check dump and restore a 
net.unix.max_dgram_qlen sysctl parameter in subns";
 >+
 >+#define CONF_UNIX_PARAM "/proc/sys/net/unix/max_dgram_qlen"
 >+
 >+int main(int argc, char **argv)
 >+{
 >+       char cmd[128];
 >+       FILE *fp;
 >+       int ret, test_max_dgram_qlen = 321, max_dgram_qlen = 0;
 >+       test_init(argc, argv);
 >+
 >+       if (unshare(CLONE_NEWNET)) {
 >+               perror("unshare");
 >+               return 1;
 >+       }
 >+
 >+       sprintf(cmd, "echo %d > %s", test_max_dgram_qlen, 
CONF_UNIX_PARAM);
 >+       if (system(cmd)) {

Do we really need system here, may be just open/write?

 >+               pr_perror("Can't change %s", CONF_UNIX_PARAM);
 >+               return -1;
 >+       }
 >+
 >+       fp = fopen(CONF_UNIX_PARAM, "r+");
 >+       if (fp == NULL) {
 >+               pr_perror("fopen");
 >+               return -1;
 >+       }
 >+
 >+       ret = fscanf(fp, "%d", &max_dgram_qlen);
 >+       if (ret != 1) {
 >+               pr_perror("fscanf");
 >+               fclose(fp);
 >+               return -1;
 >+       }
 >+
 >+       test_daemon();
 >+       test_waitsig();
 >+
 >+       if (test_max_dgram_qlen != max_dgram_qlen) {
 >+               fail();
 >+               return 1;
 >+       }

You are actually not checking that sysctl is persistent after c/r. 
test_waitsig() helper allows you to wait until zdtm makes c/r, so I 
think it is better to read max_dgram_qlen after these. I think these 
test should pass even before your criu-part of the patch.

 >+
 >+       pass();
 >+       return 0;
 >+}
 >diff --git a/test/zdtm/static/netns_sub_sysctl.desc 
b/test/zdtm/static/netns_sub_sysctl.desc
 >new file mode 100644
 >index 00000000..e13a65b6
 >--- /dev/null
 >+++ b/test/zdtm/static/netns_sub_sysctl.desc
 >@@ -0,0 +1,5 @@
 >+{
 >+    'deps': ['/bin/sh', '/sbin/ip|/bin/ip'],

If we don't call system() above we don't need these.

 >+    'flavor': 'ns',
 >+    'flags': 'suid'
 >+}
 >--
 >2.17.1
 >_______________________________________________
 >CRIU mailing list
 >CRIU at openvz.org
 >https://lists.openvz.org/mailman/listinfo/criu

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.