7.0 - ifconfig create is not working as expected?

[Sam Leffler]

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.

    Sam