Date: Wed, 27 Jun 2001 11:12:00 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: net@FreeBSD.ORG, audit@FreeBSD.ORG Subject: Re: review request: network interface cloning Message-ID: <20010627111159.E2097@sunbay.com> In-Reply-To: <20010626144313.A7909@Odin.AC.HMC.Edu>; from brooks@one-eyed-alien.net on Tue, Jun 26, 2001 at 02:43:15PM -0700 References: <20010626144313.A7909@Odin.AC.HMC.Edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jun 26, 2001 at 02:43:15PM -0700, Brooks Davis wrote: > Please review the attached patch. It does the following: > > - implementes network interface cloning via ifconfig > - adds cloning support to gif > - removes gif dependencies from stf > - makes gif and stf modular > > Notes: > > The cloning API isn't quite that of NetBSD because the NetBSD API only > supported the creation of staticaly numbered interfaces which can lead > to races and starvation in theory. This patch instead allows interfaces > to implement wildcard interface creation via "ifconfig gif# create". > > Hajimu UMEMOTO found a bug related to deletion of gif IPv6 tunnels in > certain situations. We're fairly certain it's in the IPv6 routing code > rather then in gif. Since even with this bug, the world will be a better > place with this patch, I've documented it in gif(4)'s BUGS section and > suggest we commit anyway to get cloning into the system. > > -- Brooks > > P.S. The patch can also be found at: > > http://people.freebsd.org/~brooks/patches/gif.diff > > -- > Any statement of the form "X is the one, true Y" is FALSE. > PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4 > > > Index: sbin/ifconfig/ifconfig.8 > +.It Cm plumb > +Another name for the > +.Fl create > +parameter. Included for Solaris compatability. > +.It Cm unplumb > +Another name for the > +.Fl destroy > +parameter. Included for Solaris compatability. Please don't use hard sentence breaks. Always start new sentence at new line. Should not the "compatability" be spelled as "compatibility"? > +.Fl C > +flag may be used to list all of the interface cloners available on > +the system, with no additional information. Use of this flag is > +mutually exclusive with all other flags and commands. Get rid of hard-sentence break. > Index: sbin/ifconfig/ifconfig.c > - while ((c = getopt(argc, argv, "adlmu" > + while ((c = getopt(argc, argv, "Cadlmu" > #ifdef INET6 > "L" > #endif > @@ -428,6 +445,9 @@ > case 'm': /* show media choices in status */ > supmedia = 1; > break; > + case 'C': > + listcloners = 1; > + break; > default: > usage(); > break; Especially with the long getopt() list, it might be a good idea to put `case' statements in the same order as they were in getopt() call. > @@ -448,6 +478,7 @@ > if (!namesonly && argc < 1) > all = 1; > > + > /* -a and -l allow an address family arg to limit the output */ > if (all || namesonly) { > if (argc > 1) Unnecessary newline. > +void > +list_cloners(void) > +{ > + struct if_clonereq ifcr; > + char *cp, *buf; > + int idx; > + int s; > + > + s = socket(AF_INET, SOCK_DGRAM, 0); > + if (s < 0) > + err(1, "socket"); > + Syscalls return -1 on error, not <0. > + putchar('\n'); > + free(buf); > + return; > +} void functions do not need `return' at the end. > +void > +clone_create() > +{ > + int s; > + > + s = socket(AF_INET, SOCK_DGRAM, 0); > + if (s < 0) > + err(1, "socket"); > + > + bzero(&ifr, sizeof(ifr)); You need to decide, memset(3) or bzero(3). :-) > +void > +clone_destroy(val, d, s, rafp) > + const char *val; > + int d; > + int s; > + const struct afswtch *rafp; > +{ > + (void) strncpy(ifr.ifr_name, name, sizeof(ifr.ifr_name)); > + if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) > + err(1, "SIOCIFDESTROY"); > } > Missing newline after `{'. > Index: sys/net/if.c [...] > /* > + * Create a clone network interface. > + */ > +int > +if_clone_create(name, len) > + char *name; > + int len; > +{ > + struct if_clone *ifc; > + char *dp; > + int wildcard = 0; How about moving the initialization part below? > + int unit; > + int err; > + > + ifc = if_clone_lookup(name, &unit); > + if (ifc == NULL) > + return (EINVAL); > + > + if (ifunit(name) != NULL) > + return (EEXIST); > + > + if (unit < 0) > + wildcard = 1; wildcard = (unit < 0); > + > + err = (*ifc->ifc_create)(ifc, &unit); > + if (err != 0) > + return (err); > + if (err) return (err); is more traditional :-) > + /* In the wildcard case, we need to update the name. */ > + if (wildcard) { > + for (dp = name; *dp != '#'; dp++); > + if (snprintf(dp, len - (dp-name), "%d", unit) > > + len - (dp-name) - 1) { > + /* > + * This can only be a programmer error and > + * there's no straightforward way to recover if > + * it happens. > + */ > + panic("interface name too long"); Probably worth including the function name in the panic() string. > +/* > + * Look up a network interface cloner. > + */ > +struct if_clone * > +if_clone_lookup(name, unitp) > + const char *name; > + int *unitp; > +{ > + struct if_clone *ifc; > + const char *cp; > + int i; > + > + for (ifc = LIST_FIRST(&if_cloners); ifc != NULL;) { > + for (cp = name, i = 0; i < ifc->ifc_namelen; i++, cp++) { > + if (ifc->ifc_name[i] != *cp) > + goto next_ifc; > + } > + goto found_name; > + next_ifc: > + ifc = LIST_NEXT(ifc, ifc_list); > + } > + > + /* No match. */ > + return (NULL); > + return ((struct if_clone *)NULL); IMO looks better. > Index: sys/sys/sockio.h > =================================================================== > RCS file: /home/ncvs/src/sys/sys/sockio.h,v > retrieving revision 1.18 > diff -u -u -r1.18 sockio.h > --- sys/sys/sockio.h 2001/06/11 20:34:19 1.18 > +++ sys/sys/sockio.h 2001/06/21 21:22:58 > @@ -100,4 +100,8 @@ > #define SIOCGIFSTATUS _IOWR('i', 59, struct ifstat) /* get IF status */ > #define SIOCSIFLLADDR _IOW('i', 60, struct ifreq) /* set link level addr */ > > +#define SIOCIFCREATE _IOWR('i', 122, struct ifreq) /* create clone if */ > +#define SIOCIFDESTROY _IOW('i', 121, struct ifreq) /* destroy clone if */ > +#define SIOCIFGCLONERS _IOWR('i', 120, struct if_clonereq) /* get cloners */ > + > #endif /* !_SYS_SOCKIO_H_ */ These should be documented in netintro(4). > Index: share/man/man4/gif.4 > +In some cases, when > +.Cm ifconfig gifN destroy > +is executed, an IPv6 default route is removed. > +This problem occures when the following conditions are met: "occurs"? > +.Bl -dash -offset indent -compact > +.It > +The host doesn't accept RA (i.e. net.inet6.ip6.accept_rtadv=0), Please use the full form of contractions, i.e., "does not". > +.It > +the default route is installed manually, and > +.It > +.Cm ifconfig gifN destroy > +is executed. .Nm ifconfig Ar gifN Cm destroy > +It is thought that this is not actually a bug in gif, but rather lies It is thought that this is not actually a bug in .Nm , but rather lies Cheers, -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010627111159.E2097>