[Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Sasha Levin]

This allows the user to add custom parameters to the up or down
scripts.

Cc: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 net.c     |    8 ++++++++
 net/tap.c |   37 ++++++++++++++++++++++++++++++-------
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/net.c b/net.c
index d05930c..bb27598 100644
--- a/net.c
+++ b/net.c
@@ -952,10 +952,18 @@ static const struct {
                 .type = QEMU_OPT_STRING,
                 .help = "script to initialize the interface",
             }, {
+                .name = "scriptparams",
+                .type = QEMU_OPT_STRING,
+                .help = "parameters for the initialization script",
+            }, {
                 .name = "downscript",
                 .type = QEMU_OPT_STRING,
                 .help = "script to shut down the interface",
             }, {
+                .name = "downscriptparams",
+                .type = QEMU_OPT_STRING,
+                .help = "parameters for the deinitialization script",
+            }, {
                 .name = "sndbuf",
                 .type = QEMU_OPT_SIZE,
                 .help = "send buffer limit"
diff --git a/net/tap.c b/net/tap.c
index 1f26dc9..5a9141e 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -52,7 +52,7 @@ typedef struct TAPState {
     VLANClientState nc;
     int fd;
     char down_script[1024];
-    char down_script_arg[128];
+    char down_script_arg[1024];
     uint8_t buf[TAP_BUFSIZE];
     unsigned int read_poll : 1;
     unsigned int write_poll : 1;
@@ -392,7 +392,8 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
 {
     int fd, vnet_hdr_required;
     char ifname[128] = {0,};
-    const char *setup_script;
+    const char *setup_script, *setup_script_params;
+    char setup_script_formatted[1024];
 
     if (qemu_opt_get(opts, "ifname")) {
         pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
@@ -411,10 +412,16 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
     }
 
     setup_script = qemu_opt_get(opts, "script");
+    setup_script_params = qemu_opt_get(opts, "scriptparams");
+    if (setup_script_params == NULL)
+        setup_script_params = "";
+
+    snprintf(setup_script_formatted, sizeof(setup_script_formatted),
+             "%s %s", ifname, setup_script_params);
     if (setup_script &&
         setup_script[0] != '\0' &&
         strcmp(setup_script, "no") != 0 &&
-        launch_script(setup_script, ifname, fd)) {
+        launch_script(setup_script, setup_script_formatted, fd)) {
         close(fd);
         return -1;
     }
@@ -432,9 +439,12 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
     if (qemu_opt_get(opts, "fd")) {
         if (qemu_opt_get(opts, "ifname") ||
             qemu_opt_get(opts, "script") ||
+            qemu_opt_get(opts, "scriptparams") ||
             qemu_opt_get(opts, "downscript") ||
+            qemu_opt_get(opts, "downscriptparams") ||
             qemu_opt_get(opts, "vnet_hdr")) {
-            error_report("ifname=, script=, downscript= and vnet_hdr= is invalid with fd=");
+            error_report("ifname=, script=, downscript=, scriptparams=, "
+                         "downscriptparams= and vnet_hdr= is invalid with fd=");
             return -1;
         }
 
@@ -455,6 +465,14 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
             qemu_opt_set(opts, "downscript", DEFAULT_NETWORK_DOWN_SCRIPT);
         }
 
+        if (!qemu_opt_get(opts, "scriptparams")) {
+            qemu_opt_set(opts, "scriptparams", "");
+        }
+
+        if (!qemu_opt_get(opts, "downscriptparams")) {
+            qemu_opt_set(opts, "downscriptparams", "");
+        }
+
         fd = net_tap_init(opts, &vnet_hdr);
         if (fd == -1) {
             return -1;
@@ -475,18 +493,23 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
         snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
     } else {
         const char *ifname, *script, *downscript;
+	const char *scriptparams, *downscriptparams;
 
         ifname     = qemu_opt_get(opts, "ifname");
         script     = qemu_opt_get(opts, "script");
         downscript = qemu_opt_get(opts, "downscript");
+        scriptparams = qemu_opt_get(opts, "scriptparams");
+        downscriptparams = qemu_opt_get(opts, "downscriptparams");
 
         snprintf(s->nc.info_str, sizeof(s->nc.info_str),
-                 "ifname=%s,script=%s,downscript=%s",
-                 ifname, script, downscript);
+                 "ifname=%s,script=%s,scriptparams=%s,downscript=%s,"
+                 "downscriptparams=%s", ifname, script, scriptparams,
+                 downscript, downscriptparams);
 
         if (strcmp(downscript, "no") != 0) {
             snprintf(s->down_script, sizeof(s->down_script), "%s", downscript);
-            snprintf(s->down_script_arg, sizeof(s->down_script_arg), "%s", ifname);
+            snprintf(s->down_script_arg, sizeof(s->down_script_arg), "%s %s", ifname,
+                     downscriptparams);
         }
     }
 
-- 
1.7.6.1

Re: [Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Jan Kiszka]

On 2011-09-29 15:57, Sasha Levin wrote:
> This allows the user to add custom parameters to the up or down
> scripts.
> 

What kind of parameters would you want to pass? Something that tells VMs
apart (which can be solved without these extensions) or more?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Thomas Jung]

On 2011-09-29 16:11 jan kiszka wrote
> What kind of parameters would you want to pass? Something that tells VMs
> apart (which can be solved without these extensions) or more?
> 
> Jan

In our Case:

We want to simulate an larger environment with multiple switches realized in
openvswitch.
Openvswitch requires a up- and downscript for each switch. So the idea was,
have a single and variable up- and downscript and feed it with parameters
(like switch to use, vlan id and so on) through the scriptparams. 
 
Greetings

Re: [Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Anthony Liguori]

On 09/29/2011 08:57 AM, Sasha Levin wrote:
> This allows the user to add custom parameters to the up or down
> scripts.
>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> Signed-off-by: Sasha Levin<levinsasha928@gmail.com>
> ---
>   net.c     |    8 ++++++++
>   net/tap.c |   37 ++++++++++++++++++++++++++++++-------
>   2 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/net.c b/net.c
> index d05930c..bb27598 100644
> --- a/net.c
> +++ b/net.c
> @@ -952,10 +952,18 @@ static const struct {
>                   .type = QEMU_OPT_STRING,
>                   .help = "script to initialize the interface",
>               }, {
> +                .name = "scriptparams",
> +                .type = QEMU_OPT_STRING,
> +                .help = "parameters for the initialization script",
> +            }, {
>                   .name = "downscript",
>                   .type = QEMU_OPT_STRING,
>                   .help = "script to shut down the interface",
>               }, {
> +                .name = "downscriptparams",
> +                .type = QEMU_OPT_STRING,
> +                .help = "parameters for the deinitialization script",
> +            }, {
>                   .name = "sndbuf",
>                   .type = QEMU_OPT_SIZE,
>                   .help = "send buffer limit"
> diff --git a/net/tap.c b/net/tap.c
> index 1f26dc9..5a9141e 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -52,7 +52,7 @@ typedef struct TAPState {
>       VLANClientState nc;
>       int fd;
>       char down_script[1024];
> -    char down_script_arg[128];
> +    char down_script_arg[1024];
>       uint8_t buf[TAP_BUFSIZE];
>       unsigned int read_poll : 1;
>       unsigned int write_poll : 1;
> @@ -392,7 +392,8 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
>   {
>       int fd, vnet_hdr_required;
>       char ifname[128] = {0,};
> -    const char *setup_script;
> +    const char *setup_script, *setup_script_params;
> +    char setup_script_formatted[1024];
>
>       if (qemu_opt_get(opts, "ifname")) {
>           pstrcpy(ifname, sizeof(ifname), qemu_opt_get(opts, "ifname"));
> @@ -411,10 +412,16 @@ static int net_tap_init(QemuOpts *opts, int *vnet_hdr)
>       }
>
>       setup_script = qemu_opt_get(opts, "script");
> +    setup_script_params = qemu_opt_get(opts, "scriptparams");
> +    if (setup_script_params == NULL)
> +        setup_script_params = "";
> +
> +    snprintf(setup_script_formatted, sizeof(setup_script_formatted),
> +             "%s %s", ifname, setup_script_params);
>       if (setup_script&&
>           setup_script[0] != '\0'&&
>           strcmp(setup_script, "no") != 0&&
> -        launch_script(setup_script, ifname, fd)) {
> +        launch_script(setup_script, setup_script_formatted, fd)) {

This is a little awkward.  launch_script uses execv() so you'll effectively do:

/etc/qemu-ifup "tap0 myargument"

When what you'd expect to happen is:

/etc/qemu-ifup "tap0" "myargument"

I'd suggest adding another parameter to launch script so that you can preserve 
the argument ordering.  As an added bonus, you won't have to truncate at 1024 
bytes anymore.

Regards,

Anthony Liguori

>           close(fd);
>           return -1;
>       }
> @@ -432,9 +439,12 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
>       if (qemu_opt_get(opts, "fd")) {
>           if (qemu_opt_get(opts, "ifname") ||
>               qemu_opt_get(opts, "script") ||
> +            qemu_opt_get(opts, "scriptparams") ||
>               qemu_opt_get(opts, "downscript") ||
> +            qemu_opt_get(opts, "downscriptparams") ||
>               qemu_opt_get(opts, "vnet_hdr")) {
> -            error_report("ifname=, script=, downscript= and vnet_hdr= is invalid with fd=");
> +            error_report("ifname=, script=, downscript=, scriptparams=, "
> +                         "downscriptparams= and vnet_hdr= is invalid with fd=");
>               return -1;
>           }
>
> @@ -455,6 +465,14 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
>               qemu_opt_set(opts, "downscript", DEFAULT_NETWORK_DOWN_SCRIPT);
>           }
>
> +        if (!qemu_opt_get(opts, "scriptparams")) {
> +            qemu_opt_set(opts, "scriptparams", "");
> +        }
> +
> +        if (!qemu_opt_get(opts, "downscriptparams")) {
> +            qemu_opt_set(opts, "downscriptparams", "");
> +        }
> +
>           fd = net_tap_init(opts,&vnet_hdr);
>           if (fd == -1) {
>               return -1;
> @@ -475,18 +493,23 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
>           snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=%d", fd);
>       } else {
>           const char *ifname, *script, *downscript;
> +	const char *scriptparams, *downscriptparams;
>
>           ifname     = qemu_opt_get(opts, "ifname");
>           script     = qemu_opt_get(opts, "script");
>           downscript = qemu_opt_get(opts, "downscript");
> +        scriptparams = qemu_opt_get(opts, "scriptparams");
> +        downscriptparams = qemu_opt_get(opts, "downscriptparams");
>
>           snprintf(s->nc.info_str, sizeof(s->nc.info_str),
> -                 "ifname=%s,script=%s,downscript=%s",
> -                 ifname, script, downscript);
> +                 "ifname=%s,script=%s,scriptparams=%s,downscript=%s,"
> +                 "downscriptparams=%s", ifname, script, scriptparams,
> +                 downscript, downscriptparams);
>
>           if (strcmp(downscript, "no") != 0) {
>               snprintf(s->down_script, sizeof(s->down_script), "%s", downscript);
> -            snprintf(s->down_script_arg, sizeof(s->down_script_arg), "%s", ifname);
> +            snprintf(s->down_script_arg, sizeof(s->down_script_arg), "%s %s", ifname,
> +                     downscriptparams);
>           }
>       }
>

Re: [Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Jan Kiszka]

On 2011-09-29 16:40, Thomas Jung wrote:
> On 2011-09-29 16:11 jan kiszka wrote
>> What kind of parameters would you want to pass? Something that tells VMs
>> apart (which can be solved without these extensions) or more?
>>
>> Jan
> 
> In our Case:
> 
> We want to simulate an larger environment with multiple switches realized in
> openvswitch.
> Openvswitch requires a up- and downscript for each switch. So the idea was,
> have a single and variable up- and downscript and feed it with parameters
> (like switch to use, vlan id and so on) through the scriptparams. 

Sounds reasonable. A short note on the "why" in the description would
have been nice.

The patch has some style issues that should be addressed first
(checkpatch will tell).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] tap: Add optional parameters to up/down script

[Sasha Levin]

On Thu, 2011-09-29 at 17:20 +0200, Jan Kiszka wrote:
> On 2011-09-29 16:40, Thomas Jung wrote:
> > On 2011-09-29 16:11 jan kiszka wrote
> >> What kind of parameters would you want to pass? Something that tells VMs
> >> apart (which can be solved without these extensions) or more?
> >>
> >> Jan
> > 
> > In our Case:
> > 
> > We want to simulate an larger environment with multiple switches realized in
> > openvswitch.
> > Openvswitch requires a up- and downscript for each switch. So the idea was,
> > have a single and variable up- and downscript and feed it with parameters
> > (like switch to use, vlan id and so on) through the scriptparams. 
> 
> Sounds reasonable. A short note on the "why" in the description would
> have been nice.

Sure, I'll add the use case to the patch note.

> The patch has some style issues that should be addressed first
> (checkpatch will tell).

I wasn't aware that there was a checkpatch for qemu, now I do :)

I'll fix it and resend.

-- 

Sasha.