Reordering how tap is initialized

Commit Message

Stephane Bakhos Aug. 14, 2009, 12:23 p.m. UTC

This is my first patch, so I apoligize for breaking any convention.

This patch modifies the order used in net.c for tap initialization.
It runs the script before the device is opened. This permits the creation 
of the tap device via a script instead of having to do it in advance.

It also waits for the tap to be closed before running the down script, as 
this permits the destruction of the tap device afterwards.

It applies to git and kvm-88

Comments

Anthony Liguori Aug. 14, 2009, 1:25 p.m. UTC | #1

Stephane Bakhos wrote:
> This is my first patch, so I apoligize for breaking any convention.
>
> This patch modifies the order used in net.c for tap initialization.
> It runs the script before the device is opened.

This will break existing scripts that do not rely on explicitly setting 
ifname= and instead rely on tap_open() to allocate a tap device.  In 
fact, this is one of the most common usages.

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephane Bakhos Aug. 14, 2009, 2:01 p.m. UTC | #2

> Stephane Bakhos wrote:
>>  This is my first patch, so I apoligize for breaking any convention.
>>
>>  This patch modifies the order used in net.c for tap initialization.
>>  It runs the script before the device is opened.
>
> This will break existing scripts that do not rely on explicitly setting 
> ifname= and instead rely on tap_open() to allocate a tap device.  In fact, 
> this is one of the most common usages.

What about adding create and destroy scripts that are executed before 
tap_open / tap_close?

It would achieve my goals of having 2 taps per vm and knowing where the 
tap have to connect to and it wouldn't break any existing functionality.

The 2 problems that I have with the current auto allocation is that I 
cannot have a certain way of connecting the right tap to the right bridge. 
The other problem is that it requires qemu to start as root, which I'd 
rather not do.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anthony Liguori Aug. 14, 2009, 4:28 p.m. UTC | #3

Stephane Bakhos wrote:
>> Stephane Bakhos wrote:
>>>  This is my first patch, so I apoligize for breaking any convention.
>>>
>>>  This patch modifies the order used in net.c for tap initialization.
>>>  It runs the script before the device is opened.
>>
>> This will break existing scripts that do not rely on explicitly 
>> setting ifname= and instead rely on tap_open() to allocate a tap 
>> device.  In fact, this is one of the most common usages.
>
> What about adding create and destroy scripts that are executed before 
> tap_open / tap_close?

Right now, the scripts serve an important purpose.  The run after we 
allocate a tap device but before the guest runs.  They serve as a hook 
in a very specific place in time.

The semantics you describe are basically, run a script some time before 
we open the tap device.  Well, that's effectively equivalent to just 
running the script before running QEMU.  So I don't really see any 
compelling reason to introduce such a hook in QEMU today since you can 
already achieve this functionality.

Regards,

Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net.c b/net.c
index 1e845cf..afbb0f2 100644
--- a/net.c
+++ b/net.c
@@ -1295,7 +1295,7 @@  typedef struct TAPState {
     unsigned int using_vnet_hdr : 1;
 } TAPState;
 
-static int launch_script(const char *setup_script, const char *ifname, int fd);
+static int launch_script(const char *setup_script, const char *ifname);
 
 static int tap_can_send(void *opaque);
 static void tap_send(void *opaque);
@@ -1557,12 +1557,13 @@  static void tap_cleanup(VLANClientState *vc)
 
     qemu_purge_queued_packets(vc);
 
-    if (s->down_script[0])
-        launch_script(s->down_script, s->down_script_arg, s->fd);
-
     tap_read_poll(s, 0);
     tap_write_poll(s, 0);
     close(s->fd);
+
+    if (s->down_script[0])
+        launch_script(s->down_script, s->down_script_arg);
+
     qemu_free(s);
 }
 
@@ -1794,7 +1795,7 @@  static int tap_open(char *ifname, int ifname_size, int *vnet_hdr)
 }
 #endif
 
-static int launch_script(const char *setup_script, const char *ifname, int fd)
+static int launch_script(const char *setup_script, const char *ifname)
 {
     sigset_t oldmask, mask;
     int pid, status;
@@ -1813,8 +1814,7 @@  static int launch_script(const char *setup_script, const char *ifname, int fd)
         for (i = 0; i < open_max; i++) {
             if (i != STDIN_FILENO &&
                 i != STDOUT_FILENO &&
-                i != STDERR_FILENO &&
-                i != fd) {
+                i != STDERR_FILENO) {
                 close(i);
             }
         }
@@ -1852,16 +1852,18 @@  static TAPState *net_tap_init(VLANState *vlan, const char *model,
     else
         ifname[0] = '\0';
     vnet_hdr = 0;
-    TFR(fd = tap_open(ifname, sizeof(ifname), &vnet_hdr));
-    if (fd < 0)
-        return NULL;
 
     if (!setup_script || !strcmp(setup_script, "no"))
         setup_script = "";
     if (setup_script[0] != '\0' &&
-        launch_script(setup_script, ifname, fd)) {
+        launch_script(setup_script, ifname)) {
         return NULL;
     }
+
+    TFR(fd = tap_open(ifname, sizeof(ifname), &vnet_hdr));
+    if (fd < 0)
+        return NULL;
+
     s = net_tap_fd_init(vlan, model, name, fd, vnet_hdr);
     snprintf(s->vc->info_str, sizeof(s->vc->info_str),
              "ifname=%s,script=%s,downscript=%s",