[PATCH] Reordering how tap is initialized

[PATCH] Reordering how tap is initialized

[Stephane Bakhos]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 446 bytes --]

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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2331 bytes --]

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",

Re: [PATCH] Reordering how tap is initialized

[Anthony Liguori]

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

Re: [PATCH] Reordering how tap is initialized

[Stephane Bakhos]

> 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.

Re: [PATCH] Reordering how tap is initialized

[Anthony Liguori]

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