Date: Sun, 30 Mar 2008 12:40:11 -0700 From: Sam Leffler <sam@freebsd.org> To: Eugene Grosbein <eugen@kuzbass.ru> Cc: FreeBSD-Net mailing list <freebsd-net@freebsd.org>, Remko Lodder <remko@elvandar.org>, Brooks Davis <brooks@freebsd.org>, "Bruce M. Simpson" <bms@freebsd.org>, Miroslav Lachman <000.fbsd@quip.cz> Subject: Re: 7.0 - ifconfig create is not working as expected? Message-ID: <47EFEC9B.8040205@freebsd.org> In-Reply-To: <47EFD39D.8030107@freebsd.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47EFEC9B.8040205>