tap: Add optional parameters to up/down script

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(-)

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

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

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);
>           }
>       }
>

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

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.