[Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Ian Jackson]

Previously, network scripts would have to do all of their work based
only on the script name and interface name.  If the script's behaviour
is supposed to be different for different network interfaces this
might involve constructing a special script for each interface with
the associated need to delete it etc.

With this patch it is possible to specify
  -net=...,scriptarg=<extra-info>
which gets passed as a $2 to the qemu-ifup script.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
 vl.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/vl.c b/vl.c
index 229b536..28f6fe0 100644
--- a/vl.c
+++ b/vl.c
@@ -3963,6 +3963,7 @@ typedef struct TAPState {
     VLANClientState *vc;
     int fd;
     char down_script[1024];
+    char script_arg[1024];
 } TAPState;
 
 static void tap_receive(void *opaque, const uint8_t *buf, int size)
@@ -4198,10 +4199,11 @@ static int tap_open(char *ifname, int ifname_size)
 }
 #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,
+			 const char *script_arg, int fd)
 {
     int pid, status;
-    char *args[3];
+    char *args[4];
     char **parg;
 
         /* try to launch network script */
@@ -4219,6 +4221,8 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
                 parg = args;
                 *parg++ = (char *)setup_script;
                 *parg++ = (char *)ifname;
+                if (script_arg && script_arg[0])
+                    *parg++ = (char *)script_arg;
                 *parg++ = NULL;
                 execv(setup_script, args);
                 _exit(1);
@@ -4235,7 +4239,8 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
 }
 
 static int net_tap_init(VLANState *vlan, const char *ifname1,
-                        const char *setup_script, const char *down_script)
+                        const char *setup_script, const char *down_script,
+                        const char *script_arg)
 {
     TAPState *s;
     int fd;
@@ -4252,7 +4257,7 @@ static int net_tap_init(VLANState *vlan, const char *ifname1,
     if (!setup_script || !strcmp(setup_script, "no"))
         setup_script = "";
     if (setup_script[0] != '\0') {
-	if (launch_script(setup_script, ifname, fd))
+	if (launch_script(setup_script, ifname, script_arg, fd))
 	    return -1;
     }
     s = net_tap_fd_init(vlan, fd);
@@ -4262,6 +4267,8 @@ static int net_tap_init(VLANState *vlan, const char *ifname1,
              "tap: ifname=%s setup_script=%s", ifname, setup_script);
     if (down_script && strcmp(down_script, "no"))
         snprintf(s->down_script, sizeof(s->down_script), "%s", down_script);
+    if (script_arg && script_arg[0])
+        snprintf(s->script_arg, sizeof(s->script_arg), "%s", script_arg);
     return 0;
 }
 
@@ -4854,7 +4861,7 @@ static int net_client_init(const char *str)
 #else
     if (!strcmp(device, "tap")) {
         char ifname[64];
-        char setup_script[1024], down_script[1024];
+        char setup_script[1024], down_script[1024], script_arg[1024];
         int fd;
         vlan->nb_host_devs++;
         if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
@@ -4873,7 +4880,10 @@ static int net_client_init(const char *str)
             if (get_param_value(down_script, sizeof(down_script), "downscript", p) == 0) {
                 pstrcpy(down_script, sizeof(down_script), DEFAULT_NETWORK_DOWN_SCRIPT);
             }
-            ret = net_tap_init(vlan, ifname, setup_script, down_script);
+            if (get_param_value(script_arg, sizeof(script_arg), "scriptarg", p) == 0) {
+                pstrcpy(script_arg, sizeof(script_arg), "");
+            }
+            ret = net_tap_init(vlan, ifname, setup_script, down_script, script_arg);
         }
     } else
 #endif
@@ -7191,11 +7201,12 @@ static void help(int exitcode)
            "-net tap[,vlan=n],ifname=name\n"
            "                connect the host TAP network interface to VLAN 'n'\n"
 #else
-           "-net tap[,vlan=n][,fd=h][,ifname=name][,script=file][,downscript=dfile]\n"
+           "-net tap[,vlan=n][,fd=h][,ifname=name][,script=file][,downscript=dfile][,scriptarg=extraargument]\n"
            "                connect the host TAP network interface to VLAN 'n' and use the\n"
            "                network scripts 'file' (default=%s)\n"
            "                and 'dfile' (default=%s);\n"
            "                use '[down]script=no' to disable script execution;\n"
+           "                use 'scriptarg=...' to pass an additional (nonempty) argument;\n"
            "                use 'fd=h' to connect to an already opened TAP interface\n"
 #endif
            "-net socket[,vlan=n][,fd=h][,listen=[host]:port][,connect=host:port]\n"
@@ -8638,7 +8649,7 @@ int main(int argc, char **argv)
 
                 if (sscanf(vc->info_str, "tap: ifname=%63s ", ifname) == 1 &&
                     s->down_script[0])
-                    launch_script(s->down_script, ifname, s->fd);
+                    launch_script(s->down_script, ifname, s->script_arg, s->fd);
             }
         }
     }
-- 
1.4.4.4

Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Anthony Liguori]

Ian Jackson wrote:
> Previously, network scripts would have to do all of their work based
> only on the script name and interface name.  If the script's behaviour
> is supposed to be different for different network interfaces this
> might involve constructing a special script for each interface with
> the associated need to delete it etc.
>   

env args should get passed down to the script although I can see an 
argument for being able to specify this in -net.  My only suggestion 
would be to use an env arg instead of "$2" as it will make it easier to 
extend in the future.

Regards,

Anthony Liguori

> With this patch it is possible to specify
>   -net=...,scriptarg=<extra-info>
> which gets passed as a $2 to the qemu-ifup script.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> ---
>  vl.c |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 229b536..28f6fe0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3963,6 +3963,7 @@ typedef struct TAPState {
>      VLANClientState *vc;
>      int fd;
>      char down_script[1024];
> +    char script_arg[1024];
>  } TAPState;
>  
>  static void tap_receive(void *opaque, const uint8_t *buf, int size)
> @@ -4198,10 +4199,11 @@ static int tap_open(char *ifname, int ifname_size)
>  }
>  #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,
> +			 const char *script_arg, int fd)
>  {
>      int pid, status;
> -    char *args[3];
> +    char *args[4];
>      char **parg;
>  
>          /* try to launch network script */
> @@ -4219,6 +4221,8 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
>                  parg = args;
>                  *parg++ = (char *)setup_script;
>                  *parg++ = (char *)ifname;
> +                if (script_arg && script_arg[0])
> +                    *parg++ = (char *)script_arg;
>                  *parg++ = NULL;
>                  execv(setup_script, args);
>                  _exit(1);
> @@ -4235,7 +4239,8 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
>  }
>  
>  static int net_tap_init(VLANState *vlan, const char *ifname1,
> -                        const char *setup_script, const char *down_script)
> +                        const char *setup_script, const char *down_script,
> +                        const char *script_arg)
>  {
>      TAPState *s;
>      int fd;
> @@ -4252,7 +4257,7 @@ static int net_tap_init(VLANState *vlan, const char *ifname1,
>      if (!setup_script || !strcmp(setup_script, "no"))
>          setup_script = "";
>      if (setup_script[0] != '\0') {
> -	if (launch_script(setup_script, ifname, fd))
> +	if (launch_script(setup_script, ifname, script_arg, fd))
>  	    return -1;
>      }
>      s = net_tap_fd_init(vlan, fd);
> @@ -4262,6 +4267,8 @@ static int net_tap_init(VLANState *vlan, const char *ifname1,
>               "tap: ifname=%s setup_script=%s", ifname, setup_script);
>      if (down_script && strcmp(down_script, "no"))
>          snprintf(s->down_script, sizeof(s->down_script), "%s", down_script);
> +    if (script_arg && script_arg[0])
> +        snprintf(s->script_arg, sizeof(s->script_arg), "%s", script_arg);
>      return 0;
>  }
>  
> @@ -4854,7 +4861,7 @@ static int net_client_init(const char *str)
>  #else
>      if (!strcmp(device, "tap")) {
>          char ifname[64];
> -        char setup_script[1024], down_script[1024];
> +        char setup_script[1024], down_script[1024], script_arg[1024];
>          int fd;
>          vlan->nb_host_devs++;
>          if (get_param_value(buf, sizeof(buf), "fd", p) > 0) {
> @@ -4873,7 +4880,10 @@ static int net_client_init(const char *str)
>              if (get_param_value(down_script, sizeof(down_script), "downscript", p) == 0) {
>                  pstrcpy(down_script, sizeof(down_script), DEFAULT_NETWORK_DOWN_SCRIPT);
>              }
> -            ret = net_tap_init(vlan, ifname, setup_script, down_script);
> +            if (get_param_value(script_arg, sizeof(script_arg), "scriptarg", p) == 0) {
> +                pstrcpy(script_arg, sizeof(script_arg), "");
> +            }
> +            ret = net_tap_init(vlan, ifname, setup_script, down_script, script_arg);
>          }
>      } else
>  #endif
> @@ -7191,11 +7201,12 @@ static void help(int exitcode)
>             "-net tap[,vlan=n],ifname=name\n"
>             "                connect the host TAP network interface to VLAN 'n'\n"
>  #else
> -           "-net tap[,vlan=n][,fd=h][,ifname=name][,script=file][,downscript=dfile]\n"
> +           "-net tap[,vlan=n][,fd=h][,ifname=name][,script=file][,downscript=dfile][,scriptarg=extraargument]\n"
>             "                connect the host TAP network interface to VLAN 'n' and use the\n"
>             "                network scripts 'file' (default=%s)\n"
>             "                and 'dfile' (default=%s);\n"
>             "                use '[down]script=no' to disable script execution;\n"
> +           "                use 'scriptarg=...' to pass an additional (nonempty) argument;\n"
>             "                use 'fd=h' to connect to an already opened TAP interface\n"
>  #endif
>             "-net socket[,vlan=n][,fd=h][,listen=[host]:port][,connect=host:port]\n"
> @@ -8638,7 +8649,7 @@ int main(int argc, char **argv)
>  
>                  if (sscanf(vc->info_str, "tap: ifname=%63s ", ifname) == 1 &&
>                      s->down_script[0])
> -                    launch_script(s->down_script, ifname, s->fd);
> +                    launch_script(s->down_script, ifname, s->script_arg, s->fd);
>              }
>          }
>      }
>   

Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Avi Kivity]

Anthony Liguori wrote:
> Ian Jackson wrote:
>> Previously, network scripts would have to do all of their work based
>> only on the script name and interface name.  If the script's behaviour
>> is supposed to be different for different network interfaces this
>> might involve constructing a special script for each interface with
>> the associated need to delete it etc.
>>   
>
> env args should get passed down to the script although I can see an 
> argument for being able to specify this in -net.  My only suggestion 
> would be to use an env arg instead of "$2" as it will make it easier 
> to extend in the future.

Environment variables are global, whereas scriptarg is per-interface.  
Consider the case where scriptarg denotes the bridge name we want the 
interface to attach to.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Ian Jackson wrote:
>>> Previously, network scripts would have to do all of their work based
>>> only on the script name and interface name.  If the script's behaviour
>>> is supposed to be different for different network interfaces this
>>> might involve constructing a special script for each interface with
>>> the associated need to delete it etc.
>>>   
>>
>> env args should get passed down to the script although I can see an 
>> argument for being able to specify this in -net.  My only suggestion 
>> would be to use an env arg instead of "$2" as it will make it easier 
>> to extend in the future.
>
> Environment variables are global, whereas scriptarg is per-interface.  
> Consider the case where scriptarg denotes the bridge name we want the 
> interface to attach to.

Right, what I'm suggesting is that scriptarg=... use an environmental 
variable (for instance, named SCRIPTARG) when executing the actual 
script instead of passing it as the second parameter.

It's nicer IMHO because it extends well to passing more data to the 
script (like whatever the -name parameter is).

I was originally going to suggest arbitrary ,key=value be passed by 
default as environmental variables like QEMU_key=value but I think that 
would get too complicated given the current code.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Avi Kivity]

Anthony Liguori wrote:
>>
>> Environment variables are global, whereas scriptarg is 
>> per-interface.  Consider the case where scriptarg denotes the bridge 
>> name we want the interface to attach to.
>
> Right, what I'm suggesting is that scriptarg=... use an environmental 
> variable (for instance, named SCRIPTARG) when executing the actual 
> script instead of passing it as the second parameter.
>
> It's nicer IMHO because it extends well to passing more data to the 
> script (like whatever the -name parameter is).

Ah, okay.  I don't much like environment variables but I do see the 
advantages here.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument"):
> Avi Kivity wrote:
> > Environment variables are global, whereas scriptarg is per-interface.  
> > Consider the case where scriptarg denotes the bridge name we want the 
> > interface to attach to.
...
> It's nicer IMHO because it extends well to passing more data to the 
> script (like whatever the -name parameter is).

I agree that all of the parameters to -net ought to be passed as
environment variables.

> I was originally going to suggest arbitrary ,key=value be passed by 
> default as environmental variables like QEMU_key=value but I think that 
> would get too complicated given the current code.

I think there's no particular reason why the current tap structure
couldn't be augmented by a linked list.  I'd be happy to make a patch
to do that.

Ian.

[Qemu-devel] [PATCH] Pass all parameters to -net tap, ... as env vars to if script

[Ian Jackson]

iwj writes ("Re: [Qemu-devel] [PATCH] New scriptarg=... -net parameter allows passing of an argument"):
> I agree that all of the parameters to -net ought to be passed as
> environment variables.

This is what I've done in the attached patch.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>


qemu_ifup and qemu_ifdown now get in environment variables the values
of all of the settings specified on the command line.  Each parameter
<foo> actually specified appears in an environment variable
QEMU_IF_<foo>.  (NB this variable typically has some lowercase
letters.)

Currently only parameters actually specified appear in the
environment; settings which take their default values are not provided
to the scripts.
---
 vl.c |   44 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index 229b536..7fe0e40 100644
--- a/vl.c
+++ b/vl.c
@@ -3963,6 +3963,7 @@ typedef struct TAPState {
     VLANClientState *vc;
     int fd;
     char down_script[1024];
+    char net_args[2048];
 } TAPState;
 
 static void tap_receive(void *opaque, const uint8_t *buf, int size)
@@ -4198,11 +4199,19 @@ static int tap_open(char *ifname, int ifname_size)
 }
 #endif
 
-static int launch_script(const char *setup_script, const char *ifname, int fd)
+static const char *get_opt_name(char *buf, int buf_size, const char *p);
+static const char *get_opt_value(char *buf, int buf_size, const char *p);
+
+static int launch_script(const char *setup_script, const char *ifname,
+			 const char *net_args, int fd)
 {
-    int pid, status;
+    static const char option_env_prefix[] = "QEMU_IF_";
+#define OPTION_ENV_PREFIX_LEN (sizeof(option_env_prefix)-1)
+    int pid, status, ret;
     char *args[3];
     char **parg;
+    const char *p;
+    char envvar[128 + OPTION_ENV_PREFIX_LEN], value[1024];
 
         /* try to launch network script */
         pid = fork();
@@ -4216,6 +4225,26 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
                         i != fd)
                         close(i);
 
+                p = net_args;
+                memcpy(envvar, option_env_prefix, OPTION_ENV_PREFIX_LEN);
+                for (;;) {
+                    p = get_opt_name(envvar + OPTION_ENV_PREFIX_LEN,
+				     sizeof(envvar) - OPTION_ENV_PREFIX_LEN,
+				     p);
+                    if (*p != '=')
+                        break;
+                    p++;
+                    p = get_opt_value(value, sizeof(value), p);
+                    ret = setenv(envvar, value, 1);
+                    if (ret != 0) {
+                        perror("cannot set option env var");
+                        _exit(-1);
+                    }
+                    if (*p != ',')
+                        break;
+                    p++;
+                }
+
                 parg = args;
                 *parg++ = (char *)setup_script;
                 *parg++ = (char *)ifname;
@@ -4235,7 +4264,8 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
 }
 
 static int net_tap_init(VLANState *vlan, const char *ifname1,
-                        const char *setup_script, const char *down_script)
+                        const char *setup_script, const char *down_script,
+                        const char *net_args)
 {
     TAPState *s;
     int fd;
@@ -4252,7 +4282,7 @@ static int net_tap_init(VLANState *vlan, const char *ifname1,
     if (!setup_script || !strcmp(setup_script, "no"))
         setup_script = "";
     if (setup_script[0] != '\0') {
-	if (launch_script(setup_script, ifname, fd))
+	if (launch_script(setup_script, ifname, net_args, fd))
 	    return -1;
     }
     s = net_tap_fd_init(vlan, fd);
@@ -4262,6 +4292,7 @@ static int net_tap_init(VLANState *vlan, const char *ifname1,
              "tap: ifname=%s setup_script=%s", ifname, setup_script);
     if (down_script && strcmp(down_script, "no"))
         snprintf(s->down_script, sizeof(s->down_script), "%s", down_script);
+    snprintf(s->net_args, sizeof(s->net_args), "%s", net_args);
     return 0;
 }
 
@@ -4873,7 +4904,7 @@ static int net_client_init(const char *str)
             if (get_param_value(down_script, sizeof(down_script), "downscript", p) == 0) {
                 pstrcpy(down_script, sizeof(down_script), DEFAULT_NETWORK_DOWN_SCRIPT);
             }
-            ret = net_tap_init(vlan, ifname, setup_script, down_script);
+            ret = net_tap_init(vlan, ifname, setup_script, down_script, p);
         }
     } else
 #endif
@@ -7196,6 +7227,7 @@ static void help(int exitcode)
            "                network scripts 'file' (default=%s)\n"
            "                and 'dfile' (default=%s);\n"
            "                use '[down]script=no' to disable script execution;\n"
+           "                script will have QEMU_IF_vlan=... and so on in its environment;\n"
            "                use 'fd=h' to connect to an already opened TAP interface\n"
 #endif
            "-net socket[,vlan=n][,fd=h][,listen=[host]:port][,connect=host:port]\n"
@@ -8638,7 +8670,7 @@ int main(int argc, char **argv)
 
                 if (sscanf(vc->info_str, "tap: ifname=%63s ", ifname) == 1 &&
                     s->down_script[0])
-                    launch_script(s->down_script, ifname, s->fd);
+                    launch_script(s->down_script, ifname, s->net_args, s->fd);
             }
         }
     }
-- 
1.4.4.4