Skip site navigation (1)Skip section navigation (2)
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>