Re: Patch for dhclient.8 and dhclient.c

[Steffen Daode Nurpmeso]

Jason McIntyre j...@cava.myzen.co.uk wrote [2011-10-22 16:24+0200]:
 i will look at it. but i'm pretty sure we won;t want that.
 
 jmc

Thanks for looking at *that* anyway!
It was the wrong source diff from somewhere in between.

I can't comment on 'egress' otherwise.
ifconfig.8 talks about default route(s), and get_ifname() fails if
more than one interface is in that group.

This is why i asked: it all may end up as if anyone uses
a network layout which does not fit in this scheme, something is
horribly wrong.  I'm not an administrator, but pretty local in
all aspects.

--steffen

Re: Patch for dhclient.8 and dhclient.c

[Steffen Daode Nurpmeso]

Argh! - besides everything what jmc@ said, it should have been
the following diff *for sure*:

diff --git a/sbin/dhclient/dhclient.8 b/sbin/dhclient/dhclient.8
index 9d77c7e..b6094c1 100644
--- a/sbin/dhclient/dhclient.8
+++ b/sbin/dhclient/dhclient.8
@@ -57,6 +57,16 @@ The name of the network interface that
 .Nm
 should attempt to
 configure must be specified on the command line.
+If the given name starts with the character
+.Cm = ,
+then
+.Nm
+will treat the remaining characters as the name of a group defined by
+.Xr ifconfig 8 ,
+and try to use the interface which is marked in that group.
+See
+.Xr ifconfig 8
+for more on groups.
 .Pp
 The options are as follows:
 .Bl -tag -width -p port
@@ -169,7 +179,8 @@ database of acquired leases
 .Xr dhclient-script 8 ,
 .Xr dhcp 8 ,
 .Xr dhcpd 8 ,
-.Xr dhcrelay 8
+.Xr dhcrelay 8 ,
+.Xr ifconfig 8
 .Sh AUTHORS
 .An -nosplit
 .Nm
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 8d35021..3fb1bf7 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -2073,41 +2073,57 @@ fork_privchld(int fd, int fd2)
 void
 get_ifname(char *ifname, char *arg)
 {
+#ifdef SMALL
+   if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
+   error(Interface name too long);
+#else
struct ifgroupreq ifgr;
struct ifg_req *ifg;
+   char *group;
int s, len;
 
-   if (!strcmp(arg, egress)) {
-   s = socket(AF_INET, SOCK_DGRAM, 0);
-   if (s == -1)
-   error(socket error);
-   bzero(ifgr, sizeof(ifgr));
-   strlcpy(ifgr.ifgr_name, egress, sizeof(ifgr.ifgr_name));
-   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1) {
-   if (errno == ENOENT)
-   error(no interface in group egress found);
-   error(ioctl SIOCGIFGMEMB: %m);
-   }
-   len = ifgr.ifgr_len;
-   if ((ifgr.ifgr_groups = calloc(1, len)) == NULL)
-   error(get_ifname);
-   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1)
-   error(ioctl SIOCGIFGMEMB: %m);
-
-   arg = NULL;
-   for (ifg = ifgr.ifgr_groups;
-   ifg  len = sizeof(struct ifg_req); ifg++) {
-   len -= sizeof(struct ifg_req);
-   if (arg)
-   error(too many interfaces in group egress);
-   arg = ifg-ifgrq_member;
-   }
-
+   if (*arg == '=')
+   ++arg;
+   /* 'egress' for compatibility */
+   else if (strcmp(arg, egress)) {
if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
-   error(Interface name too long: %m);
+   error(Interface name too long);
+   return;
+   }
 
-   free(ifgr.ifgr_groups);
-   close(s);
-   } else if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
+   /* Try deduce interface from if */
+   group = arg;
+
+   s = socket(AF_INET, SOCK_DGRAM, 0);
+   if (s == -1)
+   error(socket error);
+   bzero(ifgr, sizeof(ifgr));
+
+   if (strlcpy(ifgr.ifgr_name, group, IFNAMSIZ) = IFNAMSIZ)
error(Interface name too long);
+
+   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1) {
+   if (errno == ENOENT)
+   error(no interface in group %s found, group);
+   error(ioctl SIOCGIFGMEMB: %m);
+   }
+   len = ifgr.ifgr_len;
+   if ((ifgr.ifgr_groups = calloc(1, len)) == NULL)
+   error(get_ifname);
+   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1)
+   error(ioctl SIOCGIFGMEMB: %m);
+
+   arg = NULL;
+   for (ifg = ifgr.ifgr_groups;
+   ifg  len = sizeof(struct ifg_req); ifg++) {
+   len -= sizeof(struct ifg_req);
+   if (arg)
+   error(too many interfaces in group %s, group);
+   arg = ifg-ifgrq_member;
+   }
+   (void)strlcpy(ifi-name, arg, IFNAMSIZ);
+
+   free(ifgr.ifgr_groups);
+   close(s);
+#endif /* SMALL */
 }

Patch for dhclient.8 and dhclient.c

[Steffen Daode Nurpmeso]

Ciao,

49.html states:

dhclient(8) now accepts 'egress' as an interface name, meaning
whichever interface is marked as being in the 'egress' group.

but the manual page doesn't mention it.
Also it seems to me that dhclient.c:get_ifname() uses an error()
call with a %m format without any errno around.   (Nice  fat %m.)

(Since the data was filled in from the kernel, it even seems to be
sufficient to replace the entire conditional with

(void)strlcpy(ifi-name, arg, IFNAMSIZ);)

Btw., ifconfig(8) doesn't state at all that there is a limit
imposed on the length of groupnames.
And ifconfig.c even restricts availability of groups to

#ifndef SMALL 

but neither does ifconfig.8 state that nor does dhclient.c do any
such check; it surely will get ENOSYS or something in the end
though..

--steffen

diff --git a/sbin/dhclient/dhclient.8 b/sbin/dhclient/dhclient.8
index 9d77c7e..3694ff1 100644
--- a/sbin/dhclient/dhclient.8
+++ b/sbin/dhclient/dhclient.8
@@ -57,6 +57,14 @@ The name of the network interface that
 .Nm
 should attempt to
 configure must be specified on the command line.
+The special name
+.Ar egress
+will be automatically expanded to the interface which is assigned to the
+.Em egress
+group.
+See
+.Xr ifconfig 8
+for more about groups.
 .Pp
 The options are as follows:
 .Bl -tag -width -p port
@@ -169,7 +177,8 @@ database of acquired leases
 .Xr dhclient-script 8 ,
 .Xr dhcp 8 ,
 .Xr dhcpd 8 ,
-.Xr dhcrelay 8
+.Xr dhcrelay 8 ,
+.Xr ifconfig 8
 .Sh AUTHORS
 .An -nosplit
 .Nm
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 8d35021..1c6ac40 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -2104,7 +2104,7 @@ get_ifname(char *ifname, char *arg)
}
 
if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
-   error(Interface name too long: %m);
+   error(Interface name too long);
 
free(ifgr.ifgr_groups);
close(s);

Re: Patch for dhclient.8 and dhclient.c

[Steffen Daode Nurpmeso]

Hello again,

ok and while i did the patches i'm replying to, i did't
understand: the code seems to be a real nifty automatic deduction
of configuration data which is made available somewhere else
(ifconfig(8)).

It may be stupid, useless etc.
The appended diff implements a '=GROUPNAME' command line argument
syntax, and thus allows to freely specify groups.
What do you think of that?

--steffen

diff --git a/sbin/dhclient/dhclient.8 b/sbin/dhclient/dhclient.8
index 9d77c7e..b6094c1 100644
--- a/sbin/dhclient/dhclient.8
+++ b/sbin/dhclient/dhclient.8
@@ -57,6 +57,16 @@ The name of the network interface that
 .Nm
 should attempt to
 configure must be specified on the command line.
+If the given name starts with the character
+.Cm = ,
+then
+.Nm
+will treat the remaining characters as the name of a group defined by
+.Xr ifconfig 8 ,
+and try to use the interface which is marked in that group.
+See
+.Xr ifconfig 8
+for more on groups.
 .Pp
 The options are as follows:
 .Bl -tag -width -p port
@@ -169,7 +179,8 @@ database of acquired leases
 .Xr dhclient-script 8 ,
 .Xr dhcp 8 ,
 .Xr dhcpd 8 ,
-.Xr dhcrelay 8
+.Xr dhcrelay 8 ,
+.Xr ifconfig 8
 .Sh AUTHORS
 .An -nosplit
 .Nm
diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 8d35021..fa0cbef 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -2077,37 +2077,47 @@ get_ifname(char *ifname, char *arg)
struct ifg_req *ifg;
int s, len;
 
-   if (!strcmp(arg, egress)) {
-   s = socket(AF_INET, SOCK_DGRAM, 0);
-   if (s == -1)
-   error(socket error);
-   bzero(ifgr, sizeof(ifgr));
-   strlcpy(ifgr.ifgr_name, egress, sizeof(ifgr.ifgr_name));
-   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1) {
-   if (errno == ENOENT)
-   error(no interface in group egress found);
-   error(ioctl SIOCGIFGMEMB: %m);
-   }
-   len = ifgr.ifgr_len;
-   if ((ifgr.ifgr_groups = calloc(1, len)) == NULL)
-   error(get_ifname);
-   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1)
-   error(ioctl SIOCGIFGMEMB: %m);
-
-   arg = NULL;
-   for (ifg = ifgr.ifgr_groups;
-   ifg  len = sizeof(struct ifg_req); ifg++) {
-   len -= sizeof(struct ifg_req);
-   if (arg)
-   error(too many interfaces in group egress);
-   arg = ifg-ifgrq_member;
-   }
-
+   s = (*arg == '=');
+   if (s)
+   ++arg;
+   /* 'egress' for compatibility with 4.9 */
+   else if (strcmp(arg, egress)) {
if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
-   error(Interface name too long: %m);
+   error(Interface name too long);
+   return (0);
+   }
+
+   /* Try deduce interface from if */
+   s = socket(AF_INET, SOCK_DGRAM, 0);
+   if (s == -1)
+   error(socket error);
+   bzero(ifgr, sizeof(ifgr));
 
-   free(ifgr.ifgr_groups);
-   close(s);
-   } else if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
+   if (strlcpy(ifgr.ifgr_name, arg, IFNAMSIZ) = IFNAMSIZ)
error(Interface name too long);
+
+   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1) {
+   if (errno == ENOENT)
+   error(no interface in group %s found, arg);
+   error(ioctl SIOCGIFGMEMB: %m);
+   }
+   len = ifgr.ifgr_len;
+   if ((ifgr.ifgr_groups = calloc(1, len)) == NULL)
+   error(get_ifname);
+   if (ioctl(s, SIOCGIFGMEMB, (caddr_t)ifgr) == -1)
+   error(ioctl SIOCGIFGMEMB: %m);
+
+   arg = NULL;
+   for (ifg = ifgr.ifgr_groups;
+   ifg  len = sizeof(struct ifg_req); ifg++) {
+   len -= sizeof(struct ifg_req);
+   if (arg)
+   error(too many interfaces in group %s, arg);
+   arg = ifg-ifgrq_member;
+   }
+
+   (void)strlcpy(ifi-name, arg, IFNAMSIZ);
+
+   free(ifgr.ifgr_groups);
+   close(s);
 }

Re: Patch for dhclient.8 and dhclient.c

[Jason McIntyre]

On Sat, Oct 22, 2011 at 03:24:42PM +0200, Steffen Daode Nurpmeso wrote:
 Ciao,
 
 49.html states:
 
 dhclient(8) now accepts 'egress' as an interface name, meaning
 whichever interface is marked as being in the 'egress' group.
 
 but the manual page doesn't mention it.

i'm not a fan of your diff. i don;t think we should attempt to document
every utility which can use egress - i think we should just expect it to
work in places where it makes sense.

if you want to put a diff together that documents egress for all
utilities which understand egress, i will look at it. but i'm pretty
sure we won;t want that.

having said that, i don;t know much about where egress does and doesn;t
work. so some other developers might disagree and look at your (man)
patch.

jmc

 Also it seems to me that dhclient.c:get_ifname() uses an error()
 call with a %m format without any errno around.   (Nice  fat %m.)
 
 (Since the data was filled in from the kernel, it even seems to be
 sufficient to replace the entire conditional with
 
 (void)strlcpy(ifi-name, arg, IFNAMSIZ);)
 
 Btw., ifconfig(8) doesn't state at all that there is a limit
 imposed on the length of groupnames.
 And ifconfig.c even restricts availability of groups to
 
 #ifndef SMALL 
 
 but neither does ifconfig.8 state that nor does dhclient.c do any
 such check; it surely will get ENOSYS or something in the end
 though..
 
 --steffen
 
 diff --git a/sbin/dhclient/dhclient.8 b/sbin/dhclient/dhclient.8
 index 9d77c7e..3694ff1 100644
 --- a/sbin/dhclient/dhclient.8
 +++ b/sbin/dhclient/dhclient.8
 @@ -57,6 +57,14 @@ The name of the network interface that
  .Nm
  should attempt to
  configure must be specified on the command line.
 +The special name
 +.Ar egress
 +will be automatically expanded to the interface which is assigned to the
 +.Em egress
 +group.
 +See
 +.Xr ifconfig 8
 +for more about groups.
  .Pp
  The options are as follows:
  .Bl -tag -width -p port
 @@ -169,7 +177,8 @@ database of acquired leases
  .Xr dhclient-script 8 ,
  .Xr dhcp 8 ,
  .Xr dhcpd 8 ,
 -.Xr dhcrelay 8
 +.Xr dhcrelay 8 ,
 +.Xr ifconfig 8
  .Sh AUTHORS
  .An -nosplit
  .Nm
 diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
 index 8d35021..1c6ac40 100644
 --- a/sbin/dhclient/dhclient.c
 +++ b/sbin/dhclient/dhclient.c
 @@ -2104,7 +2104,7 @@ get_ifname(char *ifname, char *arg)
   }
  
   if (strlcpy(ifi-name, arg, IFNAMSIZ) = IFNAMSIZ)
 - error(Interface name too long: %m);
 + error(Interface name too long);
  
   free(ifgr.ifgr_groups);
   close(s);