Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Mike Lovell <mike@dev-zero.net>]

On 10/12/2012 12:49 AM, Mike Lovell wrote:
> This makes a few changes to allow ifname to be specified when using
> qemu-bridge-helper with both the bridge and tap network interfaces. It adds
> the --ifname option to qemu-bridge-helper, removes the restriction that ifname
> cannot be specified with helper for the tap interface, and adds logic to
> specify the --ifname option when exec'ing the helper.
>
> Signed-off-by: Mike Lovell <mike@dev-zero.net>
> ---
>
> This feature was originally requested by Mario De Chenno on the qemu-devel
> mailing list. Seems pretty simple and figured it was something I could throw
> together pretty quickly. I have tested the following combinations of invoking
> qemu (where qbr is qemu-bridge-helper)
>
> qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1"
> qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1",ifname=vm1
> qemu-system-x86_64 -net nic -net tap,helper=qbr
> qemu-system-x86_64 -net nic -net tap,helper=qbr,ifname=vm1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1,br=test1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,br=test1
>
>   net/tap.c            |   39 ++++++++++++++++++++++++++++-----------
>   qapi-schema.json     |    3 ++-
>   qemu-bridge-helper.c |   10 +++++++---
>   3 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index a88ae8f..cfb5bff 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -417,11 +417,13 @@ static int recv_fd(int c)
>       return len;
>   }
>   
> -static int net_bridge_run_helper(const char *helper, const char *bridge)
> +static int net_bridge_run_helper(const char *helper,
> +                                 const char *bridge,
> +                                 const char *ifname)
>   {
>       sigset_t oldmask, mask;
>       int pid, status;
> -    char *args[5];
> +    char *args[6];
>       char **parg;
>       int sv[2];
>   
> @@ -439,7 +441,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>           int open_max = sysconf(_SC_OPEN_MAX), i;
>           char fd_buf[6+10];
>           char br_buf[6+IFNAMSIZ] = {0};
> -        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
> +        char ifname_buf[10+IFNAMSIZ] = {0};
> +        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) +
> +                        sizeof(ifname_buf) + 15];
>   
>           for (i = 0; i < open_max; i++) {
>               if (i != STDIN_FILENO &&
> @@ -459,8 +463,13 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>                   snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
>               }
>   
> -            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> -                     helper, "--use-vnet", fd_buf, br_buf);
> +            if ((strstr(helper, "--ifname=") == NULL) && (ifname != NULL)) {
> +                snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" ,
> +                         "--ifname=", ifname);
> +            }
> +
> +            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s %s",
> +                     helper, "--use-vnet", fd_buf, br_buf, ifname_buf);
>   
>               parg = args;
>               *parg++ = (char *)"sh";
> @@ -473,12 +482,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>               /* assume helper is just the executable path name */
>   
>               snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> +            if (ifname != NULL) {
> +                snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" ,
> +                         "--ifname=", ifname);
> +            }
>   
>               parg = args;
>               *parg++ = (char *)helper;
>               *parg++ = (char *)"--use-vnet";
>               *parg++ = fd_buf;
>               *parg++ = br_buf;
> +            *parg++ = ifname_buf;
>               *parg++ = NULL;
>   
>               execv(helper, args);
> @@ -517,7 +531,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>                       NetClientState *peer)
>   {
>       const NetdevBridgeOptions *bridge;
> -    const char *helper, *br;
> +    const char *helper, *br, *ifname;
>   
>       TAPState *s;
>       int fd, vnet_hdr;
> @@ -527,8 +541,9 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>   
>       helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
>       br     = bridge->has_br     ? bridge->br     : DEFAULT_BRIDGE_INTERFACE;
> +    ifname = bridge->has_ifname ? bridge->ifname : NULL;
>   
> -    fd = net_bridge_run_helper(helper, br);
> +    fd = net_bridge_run_helper(helper, br, ifname);
>       if (fd == -1) {
>           return -1;
>       }
> @@ -622,14 +637,16 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>           model = "tap";
>   
>       } else if (tap->has_helper) {
> -        if (tap->has_ifname || tap->has_script || tap->has_downscript ||
> -            tap->has_vnet_hdr) {
> -            error_report("ifname=, script=, downscript=, and vnet_hdr= "
> +        if (tap->has_script || tap->has_downscript || tap->has_vnet_hdr) {
> +            error_report("script=, downscript=, and vnet_hdr= "
>                            "are invalid with helper=");
>               return -1;
>           }
>   
> -        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE);
> +        const char *ifname;
> +        ifname = tap->has_ifname ? tap->ifname : NULL;
> +        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE,
> +                                   ifname);
>           if (fd == -1) {
>               return -1;
>           }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f9dbdae..feaac9e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2432,7 +2432,8 @@
>   { 'type': 'NetdevBridgeOptions',
>     'data': {
>       '*br':     'str',
> -    '*helper': 'str' } }
> +    '*helper': 'str',
> +    '*ifname': 'str' } }
>   
>   ##
>   # @NetdevHubPortOptions
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index 652eec9..c1d1519 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -67,7 +67,8 @@ typedef QSIMPLEQ_HEAD(ACLList, ACLRule) ACLList;
>   static void usage(void)
>   {
>       fprintf(stderr,
> -            "Usage: qemu-bridge-helper [--use-vnet] --br=bridge --fd=unixfd\n");
> +            "Usage: qemu-bridge-helper [--use-vnet] [--ifname=name] "
> +            "--br=bridge --fd=unixfd\n");
>   }
>   
>   static int parse_acl_file(const char *filename, ACLList *acl_list)
> @@ -239,6 +240,7 @@ int main(int argc, char **argv)
>       ACLList acl_list;
>       int access_allowed, access_denied;
>       int ret = EXIT_SUCCESS;
> +    const char *ifname = NULL;
>   
>   #ifdef CONFIG_LIBCAP
>       /* if we're run from an suid binary, immediately drop privileges preserving
> @@ -259,6 +261,8 @@ int main(int argc, char **argv)
>               bridge = &argv[index][5];
>           } else if (strncmp(argv[index], "--fd=", 5) == 0) {
>               unixfd = atoi(&argv[index][5]);
> +        } else if (strncmp(argv[index], "--ifname=", 9) == 0) {
> +            ifname = &argv[index][9];
>           } else {
>               usage();
>               return EXIT_FAILURE;
> @@ -329,8 +333,8 @@ int main(int argc, char **argv)
>       }
>   
>       /* request a tap device, disable PI, and add vnet header support if
> -     * requested and it's available. */
> -    prep_ifreq(&ifr, "tap%d");
> +     * requested and it's available. use ifname if provided for tap name. */
> +    prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d");
>       ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
>       if (use_vnet && has_vnet_hdr(fd)) {
>           ifr.ifr_flags |= IFF_VNET_HDR;

ping ... or syn. any other thoughts about this?

mike