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

[Anthony Liguori <anthony@codemonkey.ws>]

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