From owner-freebsd-net@FreeBSD.ORG Sun Mar 30 19:40:24 2008 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 258AD1065677; Sun, 30 Mar 2008 19:40:24 +0000 (UTC) (envelope-from sam@freebsd.org) Received: from ebb.errno.com (ebb.errno.com [69.12.149.25]) by mx1.freebsd.org (Postfix) with ESMTP id C3CD68FC28; Sun, 30 Mar 2008 19:40:23 +0000 (UTC) (envelope-from sam@freebsd.org) Received: from trouble.errno.com (trouble.errno.com [10.0.0.248]) (authenticated bits=0) by ebb.errno.com (8.13.6/8.12.6) with ESMTP id m2UJeBls094160 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 30 Mar 2008 12:40:12 -0700 (PDT) (envelope-from sam@freebsd.org) Message-ID: <47EFEC9B.8040205@freebsd.org> Date: Sun, 30 Mar 2008 12:40:11 -0700 From: Sam Leffler Organization: FreeBSD Project User-Agent: Thunderbird 2.0.0.9 (X11/20071125) MIME-Version: 1.0 To: Eugene Grosbein References: <47EE42C8.3070100@quip.cz> <20080329204344.GA66910@lor.one-eyed-alien.net> <20080330072137.GA35435@svzserv.kemerovo.su> <47EF69F0.1050304@FreeBSD.org> <20080330104525.GA57135@svzserv.kemerovo.su> <9C5282E0-B44F-4A07-A606-1783D7725B5A@elvandar.org> <20080330142951.GA80768@svzserv.kemerovo.su> <47EFD222.2050008@freebsd.org> <47EFD39D.8030107@freebsd.org> In-Reply-To: <47EFD39D.8030107@freebsd.org> Content-Type: multipart/mixed; boundary="------------090706030204090709050703" X-DCC-Misty-Metrics: ebb.errno.com; whitelist Cc: FreeBSD-Net mailing list , Remko Lodder , Brooks Davis , "Bruce M. Simpson" , Miroslav Lachman <000.fbsd@quip.cz> Subject: Re: 7.0 - ifconfig create is not working as expected? X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 30 Mar 2008 19:40:24 -0000 This is a multi-part message in MIME format. --------------090706030204090709050703 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sam Leffler wrote: > Sam Leffler wrote: >> Eugene Grosbein wrote: >>> On Sun, Mar 30, 2008 at 01:14:36PM +0200, Remko Lodder wrote: >>> >>> >>>> Given that the idea is that we dont expect to get to this anytime >>>> soon, we welcome the person who does the analysis for us so that >>>> we might be able to fix this quicker (if possible with all the >>>> changes involved). >>>> >>> >>> Here is a patch for RELENG_7. I ask Miroslav Lachman to test it. >>> Apply: >>> >>> cd /usr/src/sbin/ifconfig >>> patch < /path/to/patchfile >>> make >>> >>> Test: >>> >>> ./ifconfig lo1 create inet 5.5.5.5 netmask 255.255.255.0 >>> >>> Or full-blown syntax: >>> >>> ./ifconfig gif0 create inet 6.6.6.6 7.7.7.7 tunnel 1.1.1.1 2.2.2.2 \ >>> netmask 255.255.255.255 mtu 1500 link2 >>> >>> Index: ifclone.c >>> =================================================================== >>> RCS file: /home/ncvs/src/sbin/ifconfig/ifclone.c,v >>> retrieving revision 1.3 >>> diff -u -r1.3 ifclone.c >>> --- ifclone.c 12 Aug 2006 18:07:17 -0000 1.3 >>> +++ ifclone.c 30 Mar 2008 14:19:08 -0000 >>> @@ -131,7 +131,9 @@ >>> static >>> DECL_CMD_FUNC(clone_create, arg, d) >>> { >>> - callback_register(ifclonecreate, NULL); >>> + if (strstr(name, "vlan") == name) >>> + callback_register(ifclonecreate, NULL); >>> + else ifclonecreate(s, NULL); >>> >> >> This breaks other cloning operations (e.g. wlan vaps that are about >> to show up in HEAD). In general it is wrong to embed knowledge about >> one type of cloning op in the common clone code. If you want to add >> the notion of cloning operations that should be done immediately vs. >> ones that should be deferred then do it generically; not by hacks >> like this. Understand however that now !vlan clone operations behave >> differently than vlans and many people will be utterly confused by >> the inconsistency. >> >>> } >>> >>> static >>> Index: ifconfig.c >>> =================================================================== >>> RCS file: /home/ncvs/src/sbin/ifconfig/ifconfig.c,v >>> retrieving revision 1.134 >>> diff -u -r1.134 ifconfig.c >>> --- ifconfig.c 4 Oct 2007 09:45:41 -0000 1.134 >>> +++ ifconfig.c 30 Mar 2008 14:22:00 -0000 >>> @@ -247,7 +247,12 @@ >>> if (iflen >= sizeof(name)) >>> errx(1, "%s: cloning name too long", >>> ifname); >>> - ifconfig(argc, argv, NULL); >>> + if (argc > 1) { >>> + afp = af_getbyname(argv[1]); >>> + if (afp != NULL) >>> + argv[1] = NULL; >>> + } >>> + ifconfig(argc, argv, afp); >>> exit(0); >>> } >>> errx(1, "interface %s does not exist", ifname); >>> @@ -451,6 +456,9 @@ >>> while (argc > 0) { >>> const struct cmd *p; >>> >>> + if(*argv == NULL) >>> + goto next; >>> + p = cmd_lookup(*argv); >>> if (p == NULL) { >>> /* >>> @@ -479,6 +487,7 @@ >>> } else >>> p->c_u.c_func(*argv, p->c_parameter, s, afp); >>> } >>> + next: >>> argc--, argv++; >>> } >>> >> >> Aside from not maintaining prevailing style and breaking cloning of >> other devices you seem to understand the issue. How to handle it is >> however unclear. I considered making 2 passes over the arguments to >> collect those required for a clone operation but never got to it. >> That still seems like the correct approach. > > It might be simpler to just do 1 pass over the args and push the clone > callback on the first non-clone parameter. Right now however there's > no way to tell what is clone-related and what is not. I think the attached patch against HEAD DTRT. Should work on RELENG_7 too. Sam --------------090706030204090709050703 Content-Type: text/plain; name="ifconfig.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="ifconfig.patch" Index: ifconfig.c =================================================================== RCS file: /usr/ncvs/src/sbin/ifconfig/ifconfig.c,v retrieving revision 1.135 diff -u -r1.135 ifconfig.c --- ifconfig.c 10 Dec 2007 02:31:00 -0000 1.135 +++ ifconfig.c 30 Mar 2008 19:29:39 -0000 @@ -93,7 +93,8 @@ int supmedia = 0; int printkeys = 0; /* Print keying material for interfaces. */ -static int ifconfig(int argc, char *const *argv, const struct afswtch *afp); +static int ifconfig(int argc, char *const *argv, int iscreate, + const struct afswtch *afp); static void status(const struct afswtch *afp, const struct sockaddr_dl *sdl, struct ifaddrs *ifa); static void tunnel_status(int s); @@ -247,7 +248,7 @@ if (iflen >= sizeof(name)) errx(1, "%s: cloning name too long", ifname); - ifconfig(argc, argv, NULL); + ifconfig(argc, argv, 1, NULL); exit(0); } errx(1, "interface %s does not exist", ifname); @@ -305,7 +306,7 @@ } if (argc > 0) - ifconfig(argc, argv, afp); + ifconfig(argc, argv, 0, afp); else status(afp, sdl, ifa); } @@ -433,17 +434,19 @@ DEF_CMD("ifdstaddr", 0, setifdstaddr); static int -ifconfig(int argc, char *const *argv, const struct afswtch *afp) +ifconfig(int argc, char *const *argv, int iscreate, const struct afswtch *afp) { + const struct afswtch *nafp; struct callback *cb; int s; + strncpy(ifr.ifr_name, name, sizeof ifr.ifr_name); +top: if (afp == NULL) afp = af_getbyname("inet"); ifr.ifr_addr.sa_family = afp->af_af == AF_LINK || afp->af_af == AF_UNSPEC ? AF_INET : afp->af_af; - strncpy(ifr.ifr_name, name, sizeof ifr.ifr_name); if ((s = socket(ifr.ifr_addr.sa_family, SOCK_DGRAM, 0)) < 0) err(1, "socket(family %u,SOCK_DGRAM", ifr.ifr_addr.sa_family); @@ -460,6 +463,32 @@ p = (setaddr ? &setifdstaddr_cmd : &setifaddr_cmd); } if (p->c_u.c_func || p->c_u.c_func2) { + if (iscreate && !p->c_iscloneop) { + /* + * Push the clone create callback so the new + * device is created and can be used for any + * remaining arguments. + */ + cb = callbacks; + if (cb == NULL) + errx(1, "internal error, no callback"); + callbacks = cb->cb_next; + cb->cb_func(s, cb->cb_arg); + /* + * Handle any address family spec that + * immediately follows and potentially + * recreate the socket. + */ + nafp = af_getbyname(*argv); + if (nafp != NULL) { + argc--, argv++; + if (nafp != afp) { + close(s); + afp = nafp; + goto top; + } + } + } if (p->c_parameter == NEXTARG) { if (argv[1] == NULL) errx(1, "'%s' requires argument", Index: ifconfig.h =================================================================== RCS file: /usr/ncvs/src/sbin/ifconfig/ifconfig.h,v retrieving revision 1.21 diff -u -r1.21 ifconfig.h --- ifconfig.h 13 Jun 2007 18:07:59 -0000 1.21 +++ ifconfig.h 30 Mar 2008 19:38:31 -0000 @@ -52,6 +52,7 @@ c_func *c_func; c_func2 *c_func2; } c_u; + int c_iscloneop; struct cmd *c_next; }; void cmd_register(struct cmd *); @@ -71,6 +72,8 @@ #define DEF_CMD_ARG(name, func) { name, NEXTARG, { .c_func = func } } #define DEF_CMD_OPTARG(name, func) { name, OPTARG, { .c_func = func } } #define DEF_CMD_ARG2(name, func) { name, NEXTARG2, { .c_func2 = func } } +#define DEF_CLONE_CMD(name, param, func) { name, param, { .c_func = func }, 1 } +#define DEF_CLONE_CMD_ARG(name, func) { name, NEXTARG, { .c_func = func }, 1 } struct ifaddrs; struct addrinfo; Index: ifvlan.c =================================================================== RCS file: /usr/ncvs/src/sbin/ifconfig/ifvlan.c,v retrieving revision 1.12 diff -u -r1.12 ifvlan.c --- ifvlan.c 9 Jul 2006 06:10:23 -0000 1.12 +++ ifvlan.c 30 Mar 2008 19:25:26 -0000 @@ -172,8 +172,8 @@ } static struct cmd vlan_cmds[] = { - DEF_CMD_ARG("vlan", setvlantag), - DEF_CMD_ARG("vlandev", setvlandev), + DEF_CLONE_CMD_ARG("vlan", setvlantag), + DEF_CLONE_CMD_ARG("vlandev", setvlandev), /* XXX For compatibility. Should become DEF_CMD() some day. */ DEF_CMD_OPTARG("-vlandev", unsetvlandev), DEF_CMD("vlanmtu", IFCAP_VLAN_MTU, setifcap), --------------090706030204090709050703--