Comments:
Suggestion to fix race condition present on this bug.
Testing this approach and will get back to this soon.
(Will run previous test cases for some hours).
I have also other change to put together lock and stage
file on different files for each interface but the change
is big and not suitable for a "SRU" (stable release update).
I'd like you to consider this first, saying if this looks
good enough for me to send you the final patch.
Thank you.
-Rafael
---
This patch introduces a temporary per interface lock file to avoid
race conditions on parallel ifupdown executions.
This fixes race conditions like this:
(a)ifup bond0 (b)ifup -a
- ---------------------------------------------------------------
1. Update statefile about bond0.
1. Does nothing about bond0
because statefile is already
updated about it.
2. ifenslave::setup_master()
sysfs_change_down mode 1
and link down bond0.
2. Link up bond0 by the vlan
script on the processing
for linking up bond0.201(*1).
3. "echo 1 > .../mode" fails.
This is meant to be a stable release fix because the final approach
should be to use the te lockfile as an independent state file.
---
header.h | 5 +++
main.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 132 insertions(+)
diff --git a/header.h b/header.h
index a10a240..233bb9f 100644
--- a/header.h
+++ b/header.h
@@ -60,6 +60,8 @@ struct interface_defn
{
interface_defn *next;
+ int lock;
+
char *logical_iface;
char *real_iface;
@@ -99,6 +101,9 @@ struct mapping_defn
#ifndef RUN_DIR
#define RUN_DIR "/run/network/"
#endif
+#ifndef TMP_DIR
+#define TMP_DIR "/tmp/"
+#endif
#ifndef LO_IFACE
#define LO_IFACE "lo"
diff --git a/main.c b/main.c
index 9f1aac1..09a33d3 100644
--- a/main.c
+++ b/main.c
@@ -420,6 +420,126 @@ bool make_pidfile_name(char *name, size_t size,
return true;
}
+//
+// functions to avoid race conditions: BEGIN
+//
+
+int
+string_compare(const void *p1, const void *p2)
+{
+ return strcmp(* (char * const *) p1, * (char * const *) p2);
+}
+
+int
+lock_iface(struct interface_defn *intf, bool tolock)
+{
+ // (un)lock a given interface
+
+ struct flock fl;
+ char tobelocked[80];
+ struct interface_defn *currif;
+
+ fl.l_type = F_WRLCK;
+ fl.l_whence = SEEK_SET;
+ fl.l_start = 0;
+ fl.l_len = 1;
+
+ memset(&tobelocked, 0, 80);
+ sprintf(&tobelocked, TMP_DIR ".ifupdown-%s.lock", intf->logical_iface);
+
+ if(tolock)
+ {
+ intf->lock = -1;
+ intf->lock = open(tobelocked, O_RDWR | O_CREAT, 0666);
+ }
+
+ if (intf->lock == -1)
+ fprintf(stderr, "cannot open/create lock file\n"), exit(1);
+
+ if(tolock)
+ {
+ if((fcntl(intf->lock, F_SETLK, &fl)) == -1)
+ {
+ usleep(250000);
+ return 1;
+ }
+ }
+ else
+ {
+ if(fcntl(intf->lock, F_UNLCK, &fl) == -1)
+ fprintf(stderr, "cannot unlock file\n"), exit(1);
+
+ /* sometimes lock file can be removed twice (testcase 7), no problem */
+ if(remove(&tobelocked) == -1) { }
+
+ close(intf->lock);
+ }
+
+ return 0;
+}
+
+void
+lock_ifaces(char **ifaces, int ifacesn, bool tolock)
+{
+ // (un)lock target interfaces
+
+ int i;
+ char *ptr, *end;
+ char *tifaces[ifacesn], *sifaces[ifacesn];
+ struct interface_defn *currif;
+
+ // mapping or interface name ?
+
+ for (i = 0; i < ifacesn; i++)
+ {
+ tifaces[i] = strchr(*(ifaces + i), '=');
+
+ if(!tifaces[i])
+ {
+ tifaces[i] = *(ifaces +i);
+ continue;
+ }
+ else
+ tifaces[i]++;
+ }
+
+ // new target iface pointers
+
+ for (i = 0; i < ifacesn; i++)
+ sifaces[i] = *(tifaces + i);
+
+ // sort new target iface pointers so locking will avoid race conditions
+
+ qsort(sifaces, ifacesn, sizeof(char *), string_compare);
+
+ if(tolock)
+ {
+ // original order
+ for(i = 0; i < ifacesn; i++)
+ for (currif = defn->ifaces; currif; currif = currif->next)
+ if(strcmp(currif->logical_iface, sifaces[i]) == 0)
+ {
+ while(lock_iface(currif, 1))
+ continue;
+ }
+ }
+
+ else
+ {
+ // reverse order
+ for(i = ifacesn-1; i >= 0; i--)
+ for (currif = defn->ifaces; currif; currif = currif->next)
+ if(strcmp(currif->logical_iface, sifaces[i]) == 0)
+ {
+ lock_iface(currif, 0);
+ }
+ }
+}
+
+//
+// functions to avoid race conditions: END
+//
+
int main(int argc, char **argv)
{
int (*cmds) (interface_defn *) = NULL;
@@ -658,6 +778,10 @@ int main(int argc, char **argv)
target_iface = argv + optind;
n_target_ifaces = argc - optind;
}
+
+ if ((cmds == iface_up) || (cmds == iface_down))
+ lock_ifaces(target_iface, n_target_ifaces, 1);
+
interface_defn meta_iface = {
.next = NULL,
.real_iface = "--all",
@@ -1040,5 +1164,8 @@ int main(int argc, char **argv)
}
}
+ if ((cmds == iface_up) || (cmds == iface_down))
+ lock_ifaces(target_iface, n_target_ifaces, 0);
+
return 0;
}
--
1.9.1
On Jul 23, 2014, at 04:05 AM, Andrew Shadura <and...@shadura.me> wrote:
> Hello,
>
> On Tue, 22 Jul 2014 21:32:40 -0300
> Rafael David Tinoco <rafael.tin...@canonical.com> wrote:
>
>> Instead of using the locking mechanism implemented today (locking
>> ifstate.lock file), or the big lock approach (which I suggested in
>> the beginning of this bug), I'll change the locking mechanism to
>> create one lock file per interface (suggestions ?). With that, when
>> an interface is being setup no other call to ifupdown binary can
>> change that interface parameters. Obs: For "-a" (all) option, it will
>> have to obtain every interface lock file to run.
>
> If you're going to implement this change, please also try to split the
> state file into multiple. I was going to introduce this change anyway,
> but haven't found time yet. I'm thinking about having a
> subdirectory, /run/network/state/ with files like eth0.state or
> similar. Ifquery also needs two command-line arguments to read and set
> them.
>
>> Hopefully this will guarantee that 1 interface configuration will not
>> be raced by other ifupdown call from the beginning of the interface
>> configuration (even when calling if-pre-up.d/if-down.d scripts) to
>> the end of that interface configuration (even if it calls
>> if-up.d/if-post-down.d) -> to be tested before patch submission
>> (soon).
>
> --
> Cheers,
> Andrew
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
