| Message ID | 20190630192933.30743-1-mcroce@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [RFC,iproute2] netns: add mounting state file for each netns | expand |
Le 30/06/2019 à 21:29, Matteo Croce a écrit : > When ip creates a netns, there is a small time interval between the > placeholder file creation in NETNS_RUN_DIR and the bind mount from /proc. > > Add a temporary file named .mounting-$netns which gets deleted after the > bind mount, so watching for delete event matching the .mounting-* name > will notify watchers only after the bind mount has been done. Probably a naive question, but why creating those '.mounting-$netns' files in the directory where netns are stored? Why not another directory, something like /var/run/netns-monitor/? Regards, Nicolas
On Mon, Jul 1, 2019 at 2:38 PM Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > > Le 30/06/2019 à 21:29, Matteo Croce a écrit : > > When ip creates a netns, there is a small time interval between the > > placeholder file creation in NETNS_RUN_DIR and the bind mount from /proc. > > > > Add a temporary file named .mounting-$netns which gets deleted after the > > bind mount, so watching for delete event matching the .mounting-* name > > will notify watchers only after the bind mount has been done. > Probably a naive question, but why creating those '.mounting-$netns' files in > the directory where netns are stored? Why not another directory, something like > /var/run/netns-monitor/? > > > Regards, > Nicolas Yes, would work too. But ideally I'd wait for the mount inotify notifications.
Le 01/07/2019 à 15:50, Matteo Croce a écrit : > On Mon, Jul 1, 2019 at 2:38 PM Nicolas Dichtel > <nicolas.dichtel@6wind.com> wrote: >> >> Le 30/06/2019 à 21:29, Matteo Croce a écrit : >>> When ip creates a netns, there is a small time interval between the >>> placeholder file creation in NETNS_RUN_DIR and the bind mount from /proc. >>> >>> Add a temporary file named .mounting-$netns which gets deleted after the >>> bind mount, so watching for delete event matching the .mounting-* name >>> will notify watchers only after the bind mount has been done. >> Probably a naive question, but why creating those '.mounting-$netns' files in >> the directory where netns are stored? Why not another directory, something like >> /var/run/netns-monitor/? >> >> >> Regards, >> Nicolas > > Yes, would work too. But ideally I'd wait for the mount inotify notifications. > Yes, I agree.
Hi Matteo, On Sun, Jun 30, 2019 at 09:29:33PM +0200, Matteo Croce wrote: > When ip creates a netns, there is a small time interval between the > placeholder file creation in NETNS_RUN_DIR and the bind mount from /proc. > > Add a temporary file named .mounting-$netns which gets deleted after the > bind mount, so watching for delete event matching the .mounting-* name > will notify watchers only after the bind mount has been done. > > Signed-off-by: Matteo Croce <mcroce@redhat.com> thanks for working on it and making my mess better! Would be nice to have it upstream. - Alex
On Sun, 30 Jun 2019 21:29:33 +0200 Matteo Croce <mcroce@redhat.com> wrote: > @@ -737,6 +746,14 @@ static int netns_add(int argc, char **argv, bool create) > } > close(fd); > > + fd = open(tmp_path, O_RDONLY|O_CREAT|O_EXCL, 0); > + if (fd < 0) { > + fprintf(stderr, "Cannot create namespace file \"%s\": %s\n", > + tmp_path, strerror(errno)); > + goto out_delete; > + } > + close(fd); > + > if (create) { > netns_save(); > if (unshare(CLONE_NEWNET) < 0) { > @@ -757,6 +774,7 @@ static int netns_add(int argc, char **argv, bool create) > goto out_delete; > } > netns_restore(); > + unlink(tmp_path); This looks like yet another source of potential errors and races. What if the program is killed or other issues. Maybe using abstract unix domain socket (which doesn't exist in filesystem and auto-deletes on exit) would be better.
diff --git a/ip/ipnetns.c b/ip/ipnetns.c index a883f210..23b95173 100644 --- a/ip/ipnetns.c +++ b/ip/ipnetns.c @@ -24,6 +24,8 @@ #include "namespace.h" #include "json_print.h" +#define TMP_PREFIX ".mounting-" + static int usage(void) { fprintf(stderr, @@ -47,6 +49,10 @@ static struct rtnl_handle rtnsh = { .fd = -1 }; static int have_rtnl_getnsid = -1; static int saved_netns = -1; +static int is_mounting_stab(const char *name) { + return !strncmp(name, TMP_PREFIX, sizeof(TMP_PREFIX) - 1); +} + static int ipnetns_accept_msg(struct rtnl_ctrl_data *ctrl, struct nlmsghdr *n, void *arg) { @@ -379,6 +385,8 @@ static int netns_list(int argc, char **argv) continue; if (strcmp(entry->d_name, "..") == 0) continue; + if (is_mounting_stab(entry->d_name)) + continue; open_json_object(NULL); print_string(PRINT_ANY, "name", @@ -676,7 +684,7 @@ static int netns_add(int argc, char **argv, bool create) * userspace tweaks like remounting /sys, or bind mounting * a new /etc/resolv.conf can be shared between users. */ - char netns_path[PATH_MAX], proc_path[PATH_MAX]; + char netns_path[PATH_MAX], tmp_path[PATH_MAX], proc_path[PATH_MAX]; const char *name; pid_t pid; int fd; @@ -701,6 +709,7 @@ static int netns_add(int argc, char **argv, bool create) name = argv[0]; snprintf(netns_path, sizeof(netns_path), "%s/%s", NETNS_RUN_DIR, name); + snprintf(tmp_path, sizeof(tmp_path), "%s/"TMP_PREFIX"%s", NETNS_RUN_DIR, name); if (create_netns_dir()) return -1; @@ -737,6 +746,14 @@ static int netns_add(int argc, char **argv, bool create) } close(fd); + fd = open(tmp_path, O_RDONLY|O_CREAT|O_EXCL, 0); + if (fd < 0) { + fprintf(stderr, "Cannot create namespace file \"%s\": %s\n", + tmp_path, strerror(errno)); + goto out_delete; + } + close(fd); + if (create) { netns_save(); if (unshare(CLONE_NEWNET) < 0) { @@ -757,6 +774,7 @@ static int netns_add(int argc, char **argv, bool create) goto out_delete; } netns_restore(); + unlink(tmp_path); return 0; out_delete: @@ -767,6 +785,10 @@ out_delete: fprintf(stderr, "Cannot remove namespace file \"%s\": %s\n", netns_path, strerror(errno)); } + if (unlink(tmp_path) < 0) { + fprintf(stderr, "Cannot remove namespace file \"%s\": %s\n", + tmp_path, strerror(errno)); + } return -1; } @@ -849,7 +871,7 @@ static int netns_monitor(int argc, char **argv) if (create_netns_dir()) return -1; - if (inotify_add_watch(fd, NETNS_RUN_DIR, IN_CREATE | IN_DELETE) < 0) { + if (inotify_add_watch(fd, NETNS_RUN_DIR, IN_DELETE) < 0) { fprintf(stderr, "inotify_add_watch failed: %s\n", strerror(errno)); return -1; @@ -865,9 +887,9 @@ static int netns_monitor(int argc, char **argv) for (event = (struct inotify_event *)buf; (char *)event < &buf[len]; event = (struct inotify_event *)((char *)event + sizeof(*event) + event->len)) { - if (event->mask & IN_CREATE) - printf("add %s\n", event->name); - if (event->mask & IN_DELETE) + if (is_mounting_stab(event->name)) + printf("add %s\n", event->name + sizeof(TMP_PREFIX) - 1); + else printf("delete %s\n", event->name); } } @@ -876,8 +898,9 @@ static int netns_monitor(int argc, char **argv) static int invalid_name(const char *name) { - return !*name || strlen(name) > NAME_MAX || - strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, ".."); + return !*name || strlen(name) + sizeof(TMP_PREFIX) - 1 > NAME_MAX || + strchr(name, '/') || !strcmp(name, ".") || !strcmp(name, "..") || + is_mounting_stab(name); } int do_netns(int argc, char **argv)
When ip creates a netns, there is a small time interval between the placeholder file creation in NETNS_RUN_DIR and the bind mount from /proc. Add a temporary file named .mounting-$netns which gets deleted after the bind mount, so watching for delete event matching the .mounting-* name will notify watchers only after the bind mount has been done. Signed-off-by: Matteo Croce <mcroce@redhat.com> --- ip/ipnetns.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-)