Hello Maintainers! After some discussions with Rafael and users affected by the race condition I have prepared a patch solving the problem by introducing a new stanza to the interfaces file: allow-group-X-Y <interfaces> where: X is the group index Y is the level in the hierarchy
This stanza allows to group interfaces into hierarchies to ensure hierarchy locking below any given interface while ifupdown operates on it. E.g. if we have a bond0 interface built on top of eth0 and eth1 the user would have to add the following to the interfaces file: allow-group-1-1 bond0 allow-group-1-2 eth0 eth1 This would cause locking of eth0 and eth1 from any modification while bond0 is handled by ifupdown. If this stanza is not used the behaviour of ifupdown should be unchanged. I have tested it together with a group of interested users in several scenarios, but we were focusing mostly on the bonding scenario as this was the case affecting the deployment I was interested in. Please let me know what you think about it. Thanks, Dariusz Gadomski
# HG changeset patch
# User Dariusz Gadomski <dariusz.gadom...@canonical.com>
# Date 1433853683 -7200
# Tue Jun 09 14:41:23 2015 +0200
# Branch hierarchical-lock
# Node ID e19ae508cd99bc374728fab8bc4f915f4088eb15
# Parent e45cc73e740e1517fd0162e02336d648dfbb7b70
Added hierarchical interface locking.
This feature requires new stanza allow-group-X-Y in /etc/network/interfaces.
Without using this stanza the behaviour remains unchanged.
Additionally the interface state files have been splitted into individual
file for each interface residing in /run/network/state.
diff -r e45cc73e740e -r e19ae508cd99 config.c
--- a/config.c Fri Mar 13 13:20:21 2015 +0100
+++ b/config.c Tue Jun 09 14:41:23 2015 +0200
@@ -22,6 +22,11 @@
allowup_defn *add_allow_up(char *filename, int line,
allowup_defn * allow_up, char *iface_name);
+int parse_group(allowup_defn *allow_ups, long *group, long *level);
+int insert_into_hierarchy(interface_hierarchy *hierarchy,
+ char *iface, long level);
+
+
variable *set_variable(char *filename, char *name, char *value,
variable ** var, int *n_vars, int *max_vars)
{
@@ -466,6 +471,7 @@
if (!add_allow_up(filename, line, allow_ups, firstword))
return NULL;
}
+
currently_processing = NONE;
} else {
switch (currently_processing) {
@@ -725,3 +731,117 @@
allow_up->n_interfaces++;
return allow_up;
}
+
+int parse_group(allowup_defn *allow_ups, long *group, long *level)
+{
+ char *next;
+ char *sep = strchr(allow_ups->when, '-');
+
+ if (group == NULL || level == NULL)
+ return -1;
+
+ if (!sep)
+ {
+ *group = -1;
+ *level = -1;
+ return -1;
+ }
+
+ *group = strtol(sep+1, &next, 10);
+ *level = strtol(next+1, NULL, 10);
+
+ if (*group == 0L || *level == 0L)
+ {
+ *group = -1;
+ *level = -1;
+ return -1;
+ }
+
+ return 0;
+}
+
+interface_hierarchy *find_iface_hierarchy(interfaces_file *defn, const char *iface)
+{
+ interface_hierarchy *result=NULL;
+ int iface_num;
+ long base_group;
+ long base_level;
+ long group;
+ long level;
+ int found_group = 0;
+
+ allowup_defn *allowups = defn->allowups;
+ for (; allowups && !found_group; allowups = allowups->next) {
+ if (strncmp(allowups->when, "group-", 6) == 0)
+ {
+ for(iface_num=0; iface_num<allowups->n_interfaces; ++iface_num)
+ {
+ if (iface && !strcmp(allowups->interfaces[iface_num], iface))
+ {
+ if (!parse_group(allowups, &base_group, &base_level))
+ {
+ // identified group of the interface in question
+ result = malloc(sizeof(interface_hierarchy));
+ result->iface = iface;
+ result->level = base_level;
+ result->next = NULL;
+ result->prev = NULL;
+ found_group = 1;
+ break;
+ }
+ }
+ }
+ }
+ }
+
+ if (found_group)
+ {
+ allowup_defn *allowups = defn->allowups;
+ for (; allowups ; allowups = allowups->next) {
+ if (strncmp(allowups->when, "group-", 6) == 0)
+ {
+ if (!parse_group(allowups, &group, &level))
+ {
+ if (group == base_group && level > base_level)
+ {
+ for(iface_num=0; iface_num<allowups->n_interfaces; ++iface_num)
+ {
+ insert_into_hierarchy(result, allowups->interfaces[iface_num], level);
+ }
+ }
+ }
+ }
+ }
+ }
+
+ return result;
+}
+
+int insert_into_hierarchy(interface_hierarchy *hierarchy, char *iface, long level)
+{
+ interface_hierarchy *prev = hierarchy;
+ interface_hierarchy *new_node;
+
+ while (hierarchy)
+ {
+ if (hierarchy->level > level ||
+ (hierarchy->level == level && strcmp(hierarchy->iface, iface) > 0))
+ break;
+
+ prev = hierarchy;
+ hierarchy = hierarchy->next;
+ }
+
+ new_node = (interface_hierarchy*)malloc(sizeof(interface_hierarchy));
+ // the iface string lives long enough, so it's not necessary to make a copy
+ new_node->iface = iface;
+ new_node->level = level;
+
+ prev->next = new_node;
+ new_node->prev = prev;
+ new_node->next = hierarchy;
+ if (hierarchy)
+ hierarchy->prev = new_node;
+
+ return 0;
+}
diff -r e45cc73e740e -r e19ae508cd99 execute.c
--- a/execute.c Fri Mar 13 13:20:21 2015 +0100
+++ b/execute.c Tue Jun 09 14:41:23 2015 +0200
@@ -25,7 +25,7 @@
{
char **environend;
int i;
- const int n_env_entries = iface->n_options + 8;
+ const int n_env_entries = iface->n_options + 9;
{
char **ppch;
@@ -75,6 +75,9 @@
*(environend++) = setlocalenv("%s=%s", "PATH", "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin");
*environend = NULL;
+
+ *(environend++) = setlocalenv("%s=%s", ENV_NO_LOCKING, "1");
+ *environend = NULL;
}
static char *setlocalenv(char *format, char *name, char *value)
diff -r e45cc73e740e -r e19ae508cd99 header.h
--- a/header.h Fri Mar 13 13:20:21 2015 +0100
+++ b/header.h Tue Jun 09 14:41:23 2015 +0200
@@ -2,6 +2,7 @@
#define HEADER_H
#include <stdbool.h>
+#include <stdio.h>
#include <string.h>
typedef struct address_family address_family;
@@ -13,6 +14,8 @@
typedef struct interface_defn interface_defn;
typedef struct variable variable;
typedef struct mapping_defn mapping_defn;
+typedef struct interface_hierarchy interface_hierarchy;
+typedef struct lock_handle lock_handle;
typedef int (execfn)(char *command);
typedef int (command_set)(interface_defn * ifd, execfn * e);
struct address_family
@@ -92,11 +95,25 @@
int n_mappings;
char **mapping;
};
+struct interface_hierarchy
+{
+ char *iface;
+ long level;
+ interface_hierarchy *next;
+ interface_hierarchy *prev;
+};
+struct lock_handle
+{
+ FILE *locked_file;
+ lock_handle *next;
+};
+
#define MAX_OPT_DEPTH 10
#define EUNBALBRACK 10001
#define EUNDEFVAR 10002
#define MAX_VARNAME 32
#define EUNBALPER 10000
+#define ENV_NO_LOCKING "IFUPDOWNNOLOCK"
#ifndef RUN_DIR
#define RUN_DIR "/run/network/"
#endif
@@ -112,6 +129,9 @@
interfaces_file *read_interfaces(char *filename);
interfaces_file *read_interfaces_defn(interfaces_file *defn, char *filename);
allowup_defn *find_allowup(interfaces_file *defn, char *name);
+interface_hierarchy *find_iface_hierarchy(interfaces_file *defn, const char *iface);
+lock_handle *lock_iface(const char *iface);
+int unlock_iface(lock_handle *handle);
int doit(char *str);
int execute_options(interface_defn * ifd, execfn * exec, char *opt);
int execute_scripts(interface_defn * ifd, execfn * exec, char *opt);
diff -r e45cc73e740e -r e19ae508cd99 main.c
--- a/main.c Fri Mar 13 13:20:21 2015 +0100
+++ b/main.c Tue Jun 09 14:41:23 2015 +0200
@@ -10,14 +10,21 @@
#include <fcntl.h>
#include <getopt.h>
#include <fnmatch.h>
+
+#include <sys/file.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <dirent.h>
+
int no_act = 0;
int run_scripts = 1;
int verbose = 0;
bool no_loopback = false;
bool ignore_failures = false;
-char lockfile[] = RUN_DIR ".ifstate.lock";
-char statefile[] = RUN_DIR "ifstate";
-char tmpstatefile[] = RUN_DIR ".ifstate.tmp";
+char statedir[] = RUN_DIR "state/";
+char lockfilepattern[] = RUN_DIR "state/.%s.lock";
+char statefilepattern[] = RUN_DIR "state/%s.state";
+char tmpstatefilepattern[] = RUN_DIR "state/.%s.state.tmp";
interfaces_file *defn;
bool match_patterns(char *string, int argc, char *argv[]);
static void usage(char *execname);
@@ -26,7 +33,9 @@
static const char *read_state(const char *argv0, const char *iface);
static void read_all_state(const char *argv0, char ***ifaces, int *n_ifaces);
static void update_state(const char *argv0, const char *iface, const char *liface);
+FILE *lock_file(const char *iface);
static int lock_fd(int fd);
+int is_ancestor(pid_t other_pid);
bool match_patterns(char *string, int argc, char *argv[])
{
if (!argc || !argv || !string)
@@ -101,8 +110,15 @@
exit(0);
}
-static FILE * lock_state(const char * argv0) {
+static void check_create_state_dir()
+{
+ mkdir(statedir, 0755);
+}
+
+static FILE * lock_state(const char * argv0, const char * iface) {
FILE *lock_fp;
+ char lockfile[80];
+ sprintf(lockfile, lockfilepattern, iface);
lock_fp = fopen(lockfile, no_act ? "r" : "a+");
if (lock_fp == NULL) {
if (!no_act) {
@@ -138,13 +154,18 @@
FILE *state_fp;
char buf[80];
char *p;
+ char statefile[80];
- lock_fp = lock_state(argv0);
+ check_create_state_dir();
+
+ lock_fp = lock_state(argv0, iface);
+
+ sprintf(statefile, statefilepattern, iface);
state_fp = fopen(statefile, no_act ? "r" : "a+");
if (state_fp == NULL) {
if (!no_act) {
- fprintf(stderr, "%s: failed to open statefile %s: %s\n", argv0, statefile, strerror(errno));
+ fprintf(stderr, "%s: failed to open statefile %s: %s (iface: '%s', statefile: '%s')\n", argv0, statefile, strerror(errno), iface, statefile);
exit(1);
} else {
goto noact;
@@ -160,25 +181,12 @@
}
}
- while ((p = fgets(buf, sizeof buf, state_fp)) != NULL) {
- char *pch;
+ if ((p = fgets(buf, sizeof(buf), state_fp)) != NULL)
+ {
+ buf[79] = '\0';
+ ret = buf + strlen(iface) + 1;
+ }
- pch = buf + strlen(buf) - 1;
- while (pch > buf && isspace(*pch))
- pch--;
- *(pch + 1) = '\0';
-
- pch = buf;
- while (isspace(*pch))
- pch++;
-
- if (strncmp(iface, pch, strlen(iface)) == 0) {
- if (pch[strlen(iface)] == '=') {
- ret = pch + strlen(iface) + 1;
- break;
- }
- }
- }
noact:
if (state_fp != NULL) {
@@ -200,49 +208,86 @@
FILE *lock_fp;
FILE *state_fp;
char buf[80];
+ char iface[80];
+ char *sep;
char *p;
+ char *statefile;
+ DIR *dp;
+ struct dirent *ep;
- lock_fp = lock_state(argv0);
+ check_create_state_dir();
- state_fp = fopen(statefile, no_act ? "r" : "a+");
- if (state_fp == NULL) {
- if (!no_act) {
- fprintf(stderr, "%s: failed to open statefile %s: %s\n", argv0, statefile, strerror(errno));
- exit(1);
- } else {
- goto noact;
+ dp = opendir(statedir);
+
+ if (dp != NULL)
+ {
+ *n_ifaces = 0;
+ *ifaces = NULL;
+
+ while ((ep = readdir(dp)))
+ {
+ if (strcmp(ep->d_name, ".") == 0 || strcmp(ep->d_name, "..") == 0)
+ continue;
+
+ sep = strstr(ep->d_name, ".");
+ if (sep)
+ {
+ int len = sep - ep->d_name;
+ strncpy(iface, ep->d_name, len);
+ iface[len] = '\0';
+ }
+
+ lock_fp = lock_state(argv0, iface);
+
+ statefile = ep->d_name;
+ state_fp = fopen(statefile, no_act ? "r" : "a+");
+ if (state_fp == NULL) {
+ if (!no_act) {
+ fprintf(stderr, "%s: failed to open statefile %s: %s (iface: '%s')\n", argv0, statefile, strerror(errno), ep->d_name);
+ exit(1);
+ } else {
+ goto noact;
+ }
+ }
+
+ if (!no_act) {
+ int flags;
+
+ if ((flags = fcntl(fileno(state_fp), F_GETFD)) < 0 || fcntl(fileno(state_fp), F_SETFD, flags | FD_CLOEXEC) < 0) {
+ fprintf(stderr, "%s: failed to set FD_CLOEXEC on statefile %s: %s\n", argv0, statefile, strerror(errno));
+ exit(1);
+ }
+ }
+
+ while ((p = fgets(buf, sizeof buf, state_fp)) != NULL) {
+ char *pch;
+
+ pch = buf + strlen(buf) - 1;
+ while (pch > buf && isspace(*pch))
+ pch--;
+ *(pch + 1) = '\0';
+
+ pch = buf;
+ while (isspace(*pch))
+ pch++;
+
+ (*n_ifaces)++;
+ *ifaces = realloc(*ifaces, sizeof(**ifaces) * *n_ifaces);
+ (*ifaces)[(*n_ifaces) - 1] = strdup(pch);
+ }
+
+ if (state_fp != NULL) {
+ fclose(state_fp);
+ state_fp = NULL;
+ }
+
+ if (lock_fp != NULL) {
+ fclose(lock_fp);
+ lock_fp = NULL;
+ }
}
}
- if (!no_act) {
- int flags;
-
- if ((flags = fcntl(fileno(state_fp), F_GETFD)) < 0 || fcntl(fileno(state_fp), F_SETFD, flags | FD_CLOEXEC) < 0) {
- fprintf(stderr, "%s: failed to set FD_CLOEXEC on statefile %s: %s\n", argv0, statefile, strerror(errno));
- exit(1);
- }
- }
-
- *n_ifaces = 0;
- *ifaces = NULL;
-
- while ((p = fgets(buf, sizeof buf, state_fp)) != NULL) {
- char *pch;
-
- pch = buf + strlen(buf) - 1;
- while (pch > buf && isspace(*pch))
- pch--;
- *(pch + 1) = '\0';
-
- pch = buf;
- while (isspace(*pch))
- pch++;
-
- (*n_ifaces)++;
- *ifaces = realloc(*ifaces, sizeof(**ifaces) * *n_ifaces);
- (*ifaces)[(*n_ifaces) - 1] = strdup(pch);
- }
-
for (i = 0; i < ((*n_ifaces) / 2); i++) {
char *temp = (*ifaces)[i];
(*ifaces)[i] = (*ifaces)[(*n_ifaces) - i - 1];
@@ -250,14 +295,9 @@
}
noact:
- if (state_fp != NULL) {
- fclose(state_fp);
- state_fp = NULL;
- }
-
- if (lock_fp != NULL) {
- fclose(lock_fp);
- lock_fp = NULL;
+ if (dp != NULL)
+ {
+ closedir(dp);
}
}
@@ -267,15 +307,20 @@
FILE *lock_fp;
FILE *state_fp;
- char buf[80];
char *p;
+ char statefile[80];
+ char tmpstatefile[80];
- lock_fp = lock_state(argv0);
+ check_create_state_dir();
+
+ lock_fp = lock_state(argv0, iface);
+
+ sprintf(statefile, statefilepattern, iface);
state_fp = fopen(statefile, no_act ? "r" : "a+");
if (state_fp == NULL) {
if (!no_act) {
- fprintf(stderr, "%s: failed to open statefile %s: %s\n", argv0, statefile, strerror(errno));
+ fprintf(stderr, "%s: failed to open statefile %s: %s (iface: '%s', state: '%s')\n", argv0, statefile, strerror(errno), iface, state);
exit(1);
} else {
goto noact;
@@ -291,7 +336,7 @@
}
if (lock_fd(fileno(state_fp)) < 0) {
- fprintf(stderr, "%s: failed to lock statefile %s: %s\n", argv0, statefile, strerror(errno));
+ fprintf(stderr, "%s: failed to open statefile %s: %s (iface: '%s', state: '%s')\n", argv0, statefile, strerror(errno), iface, state);
exit(1);
}
}
@@ -299,38 +344,14 @@
if (no_act)
goto noact;
+ sprintf(tmpstatefile, tmpstatefilepattern, iface);
+
tmp_fp = fopen(tmpstatefile, "w");
if (tmp_fp == NULL) {
fprintf(stderr, "%s: failed to open temporary statefile %s: %s\n", argv0, tmpstatefile, strerror(errno));
exit(1);
}
- while ((p = fgets(buf, sizeof buf, state_fp)) != NULL) {
- char *pch;
-
- pch = buf + strlen(buf) - 1;
- while (pch > buf && isspace(*pch))
- pch--;
- *(pch + 1) = '\0';
-
- pch = buf;
- while (isspace(*pch))
- pch++;
-
- if (strncmp(iface, pch, strlen(iface)) == 0) {
- if (pch[strlen(iface)] == '=') {
- if (state != NULL) {
- fprintf(tmp_fp, "%s=%s\n", iface, state);
- state = NULL;
- }
-
- continue;
- }
- }
-
- fprintf(tmp_fp, "%s\n", pch);
- }
-
if (state != NULL)
fprintf(tmp_fp, "%s=%s\n", iface, state);
@@ -431,6 +452,7 @@
int max_options = 0;
int n_target_ifaces;
char **target_iface;
+ lock_handle *lock_h = NULL;
{
int i;
@@ -482,7 +504,7 @@
interfaces = strdup(optarg);
break;
case 'v':
- verbose = 1;
+ verbose++;
break;
case 'a':
do_all = 1;
@@ -680,6 +702,13 @@
liface[sizeof(liface) - 1] = '\0';
}
}
+ if (lock_h)
+ {
+ unlock_iface(lock_h);
+ lock_h = NULL;
+ }
+ if (cmds == iface_up || cmds == iface_down)
+ lock_h = lock_iface(iface);
current_state = read_state(argv[0], iface);
if (!force) {
{
@@ -1016,5 +1045,183 @@
}
}
+ unlock_iface(lock_h);
+
return 0;
}
+
+lock_handle *lock_iface(const char *iface)
+{
+ lock_handle *result = NULL;
+ FILE *f;
+
+ if (getenv(ENV_NO_LOCKING))
+ {
+ if (verbose > 1)
+ fprintf(stderr, "locking disabled (env variable %s is set)\n",
+ ENV_NO_LOCKING);
+ return NULL;
+ }
+
+ interface_hierarchy *hierarchy = find_iface_hierarchy(defn, iface);
+ if (hierarchy)
+ {
+ for (; hierarchy; hierarchy = hierarchy->next)
+ {
+ f = lock_file(hierarchy->iface);
+ if (f)
+ {
+ lock_handle *lock = (lock_handle*) malloc(sizeof(lock_handle));
+ lock->next = result;
+ lock->locked_file = f;
+ result = lock;
+ }
+ }
+ }
+ else
+ {
+ f = lock_file(iface);
+ if (f)
+ {
+ lock_handle *lock = (lock_handle*) malloc(sizeof(lock_handle));
+ lock->next = result;
+ lock->locked_file = f;
+ result = lock;
+ }
+ }
+
+ free(hierarchy);
+
+ return result;
+}
+
+FILE *lock_file(const char *iface)
+{
+ char lock_file[100];
+ struct flock lock;
+ int do_lock = 0;
+
+ sprintf(lock_file, "%s%s.lock", RUN_DIR, iface);
+
+ FILE *f = fopen(lock_file, "a+");
+
+ if (f)
+ {
+ lock.l_type = F_WRLCK;
+ lock.l_whence = SEEK_SET;
+ lock.l_start = 0;
+ lock.l_len = 0;
+
+ if (!fcntl(fileno(f), F_SETLK, &lock))
+ {
+ return f;
+ }
+
+ if (!fcntl(fileno(f), F_GETLK, &lock))
+ {
+ if (lock.l_type == F_UNLCK)
+ {
+ // lock may be placed
+ do_lock = 1;
+ }
+ else
+ {
+ // lock already holded
+ // check if it holded by a parent process
+ if (!is_ancestor(lock.l_pid))
+ do_lock = 1;
+ else
+ if (verbose > 1)
+ fprintf(stderr, "Inherited lock (%s), not locking again\n.", iface);
+ }
+ }
+
+ if (do_lock)
+ {
+ lock.l_type = F_WRLCK;
+ lock.l_whence = SEEK_SET;
+ lock.l_start = 0;
+ lock.l_len = 0;
+
+ if (fcntl(fileno(f), F_SETLKW, &lock))
+ {
+ fprintf(stderr, "Failed to take lock for %s: %s\n",
+ iface, strerror(errno));
+ fclose(f);
+ return NULL;
+ }
+ }
+
+ return f;
+ }
+
+ if (verbose > 1)
+ fprintf(stderr, "lock_file failed to lock %s (%s): %s\n", iface, lock_file, strerror(errno));
+
+ return NULL;
+}
+
+int unlock_iface(lock_handle *handle)
+{
+ lock_handle *prev = NULL;
+
+ if (!handle)
+ return 1;
+
+ for (; handle; handle = handle->next)
+ {
+ flock(fileno(handle->locked_file), LOCK_UN);
+ fclose(handle->locked_file);
+ free(prev);
+ prev = handle;
+ }
+
+ free(prev);
+
+ return 0;
+}
+
+pid_t getppid_of(pid_t pid)
+{
+ const int BUFSIZE = 100;
+ char path[100];
+ char buf[BUFSIZE+1];
+ char *s;
+ FILE *status_file;
+ pid_t result = -1;
+ sprintf(path, "/proc/%d/status", pid);
+
+ status_file = fopen(path, "r");
+
+ if (status_file == NULL)
+ {
+ fprintf(stderr, "Can't open status for pid %d\n", pid);
+ return -1;
+ }
+
+ while ((s = fgets(buf, BUFSIZE, status_file)) != NULL)
+ {
+ if (strstr(s, "PPid:\t") == s)
+ {
+ result = strtol(s+6, NULL, 10);
+ break;
+ }
+ }
+
+ fclose(status_file);
+
+ return result;
+}
+
+int is_ancestor(pid_t other_pid)
+{
+ pid_t pid = getpid();
+
+ for (; pid > 1; pid = getppid_of(pid))
+ {
+ if (pid == other_pid)
+ return 1;
+ }
+
+ return 0;
+}
signature.asc
Description: Digital signature
