ifconfig: remove redundant bridge checks

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

ifconfig: remove redundant bridge checks

Klemens Nanni-2
bridge_status() and switch_status() do the regular sanity check with
SIOCGIFFLAGS, but both functions also call is_switch(), bridge_status()
also calls is_bridge().

Those is_*() helpers do the same SIOCGIFFLAGS sanity check, making those
in *_status() entirely redundant, so I'd like to remove them.

I'm here since the tpmr(4) ioctl interface transition from trunk to
bridge semantics is now complete, so ifconfig(8) now requires tpmr bits
to show its members in bridge fashion.

One way would be duplicate code into is_tpmr() and tpmr_status() which
I've already done, but another approach is to unify all bridge like
interfaces under bridge_status().

Either ways, diff below cleans up and makes for simpler code.

Feedback? OK?


Index: brconfig.c
===================================================================
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.25
diff -u -p -r1.25 brconfig.c
--- brconfig.c 22 Jan 2020 06:24:07 -0000 1.25
+++ brconfig.c 28 Jul 2020 17:02:05 -0000
@@ -783,16 +783,11 @@ is_bridge()
 void
 bridge_status(void)
 {
- struct ifreq ifr;
  struct ifbrparam bp1, bp2;
 
  if (!is_bridge() || is_switch())
  return;
 
- strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
- if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
- return;
-
  bridge_cfg("\t");
 
  bridge_list("\t");
@@ -1184,13 +1179,7 @@ switch_cfg(char *delim)
 void
 switch_status(void)
 {
- struct ifreq ifr;
-
  if (!is_switch())
- return;
-
- strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
- if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
  return;
 
  switch_cfg("\t");

Reply | Threaded
Open this post in threaded view
|

Re: ifconfig: remove redundant bridge checks

Klemens Nanni-2
On Tue, Jul 28, 2020 at 07:09:17PM +0200, Klemens Nanni wrote:
> bridge_status() and switch_status() do the regular sanity check with
> SIOCGIFFLAGS, but both functions also call is_switch(), bridge_status()
> also calls is_bridge().
>
> Those is_*() helpers do the same SIOCGIFFLAGS sanity check, making those
> in *_status() entirely redundant, so I'd like to remove them.
Small correction: is_bridge() does SIOCGIFFLAGS, is_switch() does not.

Below is a new diff that removes the SIOCGIFFLAGS check form is_bridge()
as well, leaving the two is_*() helpers to their driver specific ioctls
alone.

> I'm here since the tpmr(4) ioctl interface transition from trunk to
> bridge semantics is now complete, so ifconfig(8) now requires tpmr bits
> to show its members in bridge fashion.
>
> One way would be duplicate code into is_tpmr() and tpmr_status() which
> I've already done, but another approach is to unify all bridge like
> interfaces under bridge_status().
With this in, merging switch_status() into bridge_status() is a trivial
diff, adding tpmr awareness to the mix would then be another diff after
that.

> Either ways, diff below cleans up and makes for simpler code.
So this effectively just removes SIOCGIFFLAGS in brconfig.c which are of
no use, imho.  ifconfig.c:getinfo() already checks interfaces flags,
even more than once, for all interfaces.

> Feedback? OK?


Index: brconfig.c
===================================================================
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.25
diff -u -p -r1.25 brconfig.c
--- brconfig.c 22 Jan 2020 06:24:07 -0000 1.25
+++ brconfig.c 29 Jul 2020 00:58:40 -0000
@@ -762,14 +762,8 @@ bridge_holdcnt(const char *value, int d)
 int
 is_bridge()
 {
- struct ifreq ifr;
  struct ifbaconf ifbac;
 
- strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
-
- if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
- return (0);
-
  ifbac.ifbac_len = 0;
  strlcpy(ifbac.ifbac_name, ifname, sizeof(ifbac.ifbac_name));
  if (ioctl(sock, SIOCBRDGRTS, (caddr_t)&ifbac) == -1) {
@@ -783,16 +777,11 @@ is_bridge()
 void
 bridge_status(void)
 {
- struct ifreq ifr;
  struct ifbrparam bp1, bp2;
 
  if (!is_bridge() || is_switch())
  return;
 
- strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
- if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
- return;
-
  bridge_cfg("\t");
 
  bridge_list("\t");
@@ -1184,13 +1173,7 @@ switch_cfg(char *delim)
 void
 switch_status(void)
 {
- struct ifreq ifr;
-
  if (!is_switch())
- return;
-
- strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
- if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
  return;
 
  switch_cfg("\t");

Reply | Threaded
Open this post in threaded view
|

Re: ifconfig: remove redundant bridge checks

David Gwynne-5
ok.

> On 29 Jul 2020, at 11:38, Klemens Nanni <[hidden email]> wrote:
>
> On Tue, Jul 28, 2020 at 07:09:17PM +0200, Klemens Nanni wrote:
>> bridge_status() and switch_status() do the regular sanity check with
>> SIOCGIFFLAGS, but both functions also call is_switch(), bridge_status()
>> also calls is_bridge().
>>
>> Those is_*() helpers do the same SIOCGIFFLAGS sanity check, making those
>> in *_status() entirely redundant, so I'd like to remove them.
> Small correction: is_bridge() does SIOCGIFFLAGS, is_switch() does not.
>
> Below is a new diff that removes the SIOCGIFFLAGS check form is_bridge()
> as well, leaving the two is_*() helpers to their driver specific ioctls
> alone.
>
>> I'm here since the tpmr(4) ioctl interface transition from trunk to
>> bridge semantics is now complete, so ifconfig(8) now requires tpmr bits
>> to show its members in bridge fashion.
>>
>> One way would be duplicate code into is_tpmr() and tpmr_status() which
>> I've already done, but another approach is to unify all bridge like
>> interfaces under bridge_status().
> With this in, merging switch_status() into bridge_status() is a trivial
> diff, adding tpmr awareness to the mix would then be another diff after
> that.
>
>> Either ways, diff below cleans up and makes for simpler code.
> So this effectively just removes SIOCGIFFLAGS in brconfig.c which are of
> no use, imho.  ifconfig.c:getinfo() already checks interfaces flags,
> even more than once, for all interfaces.
>
>> Feedback? OK?
>
>
> Index: brconfig.c
> ===================================================================
> RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 brconfig.c
> --- brconfig.c 22 Jan 2020 06:24:07 -0000 1.25
> +++ brconfig.c 29 Jul 2020 00:58:40 -0000
> @@ -762,14 +762,8 @@ bridge_holdcnt(const char *value, int d)
> int
> is_bridge()
> {
> - struct ifreq ifr;
> struct ifbaconf ifbac;
>
> - strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
> -
> - if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
> - return (0);
> -
> ifbac.ifbac_len = 0;
> strlcpy(ifbac.ifbac_name, ifname, sizeof(ifbac.ifbac_name));
> if (ioctl(sock, SIOCBRDGRTS, (caddr_t)&ifbac) == -1) {
> @@ -783,16 +777,11 @@ is_bridge()
> void
> bridge_status(void)
> {
> - struct ifreq ifr;
> struct ifbrparam bp1, bp2;
>
> if (!is_bridge() || is_switch())
> return;
>
> - strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
> - if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
> - return;
> -
> bridge_cfg("\t");
>
> bridge_list("\t");
> @@ -1184,13 +1173,7 @@ switch_cfg(char *delim)
> void
> switch_status(void)
> {
> - struct ifreq ifr;
> -
> if (!is_switch())
> - return;
> -
> - strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
> - if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)&ifr) == -1)
> return;
>
> switch_cfg("\t");