From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37180) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TeKjz-0004KP-9t for qemu-devel@nongnu.org; Fri, 30 Nov 2012 02:10:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TeKjx-0007VZ-Gr for qemu-devel@nongnu.org; Fri, 30 Nov 2012 02:10:27 -0500 Received: from 69.169.164.127.provo.static.broadweavenetworks.net ([69.169.164.127]:41451 helo=baldr.dev-zero.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TeKjx-0007VV-8O for qemu-devel@nongnu.org; Fri, 30 Nov 2012 02:10:25 -0500 Message-ID: <50B85BEA.6000107@dev-zero.net> Date: Fri, 30 Nov 2012 00:10:34 -0700 From: Mike Lovell MIME-Version: 1.0 References: <5076E640.4090003@linux.vnet.ibm.com> <1350024543-26211-1-git-send-email-mike@dev-zero.net> In-Reply-To: <1350024543-26211-1-git-send-email-mike@dev-zero.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, stefanha@gmail.com 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 > --- > > 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