bgpctl: enlarge columns for 4-byte ASN display

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

bgpctl: enlarge columns for 4-byte ASN display

Gregor Best
Hi people,

I'm running OpenBGPD with a few peers that have large 4-byte AS numbers.
Displaying the table of peers with `bgpctl show` shows the stats just
fine, but the length of the AS number shifts the content of the
statistics confusingly far to the right, such as in the following
example:

  $ bgpctl show
  Neighbor                   AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
  peer 1               64734.12580        316        466     0 00:07:04    364
  peer 2               64734.12578        251        293     0 00:40:06    365
  peer 3                  64828      17545      13466     0 3d17h35m    325

The attached patch makes the 'AS' column 4 characters larger, with the
following result:

  $ bgpctl show
  Neighbor                       AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
  peer 1                64734.12580        317        468     0 00:07:51    364
  peer 2                64734.12578        252        295     0 00:40:53    365
  peer 3                      64828      17547      13467     0 3d17h36m    325

--
        Gregor Best

bgpctl.patch (1004 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Claudio Jeker
Not a big fan since this makes the bgpctl show output no longer fit 80
chars and so will wrap lines on default terminals. While it is OK to
increase the size it should be taken away from other fields in some whay.
An option would be to drop the OutQ since that field has only limited
value IMO.


--
:wq Claudio

PS: you only need 11 chars and not 12 for AS-DOT notation

On Sat, Jul 26, 2014 at 03:57:16PM +0200, Gregor Best wrote:

> Hi people,
>
> I'm running OpenBGPD with a few peers that have large 4-byte AS numbers.
> Displaying the table of peers with `bgpctl show` shows the stats just
> fine, but the length of the AS number shifts the content of the
> statistics confusingly far to the right, such as in the following
> example:
>
>   $ bgpctl show
>   Neighbor                   AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
>   peer 1               64734.12580        316        466     0 00:07:04    364
>   peer 2               64734.12578        251        293     0 00:40:06    365
>   peer 3                  64828      17545      13466     0 3d17h35m    325
>
> The attached patch makes the 'AS' column 4 characters larger, with the
> following result:
>
>   $ bgpctl show
>   Neighbor                       AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
>   peer 1                64734.12580        317        468     0 00:07:51    364
>   peer 2                64734.12578        252        295     0 00:40:53    365
>   peer 3                      64828      17547      13467     0 3d17h36m    325
>
> --
> Gregor Best

> Index: bgpctl.c
> ===================================================================
> RCS file: /mnt/media/cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.174
> diff -u -p -u -r1.174 bgpctl.c
> --- bgpctl.c 18 Mar 2014 13:47:14 -0000 1.174
> +++ bgpctl.c 26 Jul 2014 13:49:17 -0000
> @@ -508,7 +508,7 @@ fmt_peer(const char *descr, const struct
>  void
>  show_summary_head(void)
>  {
> - printf("%-20s %8s %10s %10s %5s %-8s %s\n", "Neighbor", "AS",
> + printf("%-20s %12s %10s %10s %5s %-8s %s\n", "Neighbor", "AS",
>      "MsgRcvd", "MsgSent", "OutQ", "Up/Down", "State/PrfRcvd");
>  }
>  
> @@ -525,7 +525,7 @@ show_summary_msg(struct imsg *imsg, int
>      p->conf.remote_masklen, nodescr);
>   if (strlen(s) >= 20)
>   s[20] = 0;
> - printf("%-20s %8s %10llu %10llu %5u %-8s ",
> + printf("%-20s %12s %10llu %10llu %5u %-8s ",
>      s, log_as(p->conf.remote_as),
>      p->stats.msg_rcvd_open + p->stats.msg_rcvd_notification +
>      p->stats.msg_rcvd_update + p->stats.msg_rcvd_keepalive +

Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Gregor Best
On Sun, Jul 27, 2014 at 11:15:41AM +0200, Claudio Jeker wrote:
> Not a big fan since this makes the bgpctl show output no longer fit 80
> chars and so will wrap lines on default terminals.
> [...]

Agreed, that's not good.

> While it is OK to
> increase the size it should be taken away from other fields in some whay.
> An option would be to drop the OutQ since that field has only limited
> value IMO.
> [...]

Right. I dare say on most links the OutQ column is 0 most of the
time anyway, or the link has problems that are visible in other
ways as well.

> [...]
> PS: you only need 11 chars and not 12 for AS-DOT notation
> [...]

Whoops, off by one.

The attached patch addresses the issues you pointed out by removing the
OutQ field and increasing the AS column width by 3 characters. The
output fits into 77 characters now.

--
        Gregor Best

bgpctl.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Stuart Henderson-6
In reply to this post by Claudio Jeker
On 2014/07/27 11:15, Claudio Jeker wrote:
> Not a big fan since this makes the bgpctl show output no longer fit 80
> chars and so will wrap lines on default terminals. While it is OK to
> increase the size it should be taken away from other fields in some whay.
> An option would be to drop the OutQ since that field has only limited
> value IMO.

oh, I would miss not having OutQ, it's totally useless when things are
working correctly, but I have had a couple of situations (one with openbgp,
one with *cough* bay BCN "some time ago"...) where this has been helpful.
It gives quite a clear pointer to problems transmitting to that particular
peer without having to look in netstat output and work out which peer is
which (some physical layer problems, MTU problems etc).

I think the most usable display would be to have neighbor and AS "share"
a column, with priority to AS... i.e.

somepeer                12345      16419      89603     0 09:42:06    100/8000
somepeer-with-long-name  5678    1604940    1367282     0 13:39:28    100/8000
otherpeer            16584484      16420      89606     0 1d00h58m Active
otherpeer-with-long- 16584485    1604940    1367282     0 13:40:17    100/8000

Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Gregor Best
On Sun, Jul 27, 2014 at 03:36:06PM +0100, Stuart Henderson wrote:

> On 2014/07/27 11:15, Claudio Jeker wrote:
> > Not a big fan since this makes the bgpctl show output no longer fit 80
> > chars and so will wrap lines on default terminals. While it is OK to
> > increase the size it should be taken away from other fields in some whay.
> > An option would be to drop the OutQ since that field has only limited
> > value IMO.
>
> oh, I would miss not having OutQ, it's totally useless when things are
> working correctly, but I have had a couple of situations (one with openbgp,
> one with *cough* bay BCN "some time ago"...) where this has been helpful.
> It gives quite a clear pointer to problems transmitting to that particular
> peer without having to look in netstat output and work out which peer is
> which (some physical layer problems, MTU problems etc).
>
> I think the most usable display would be to have neighbor and AS "share"
> a column, with priority to AS... i.e.
>
> somepeer                12345      16419      89603     0 09:42:06    100/8000
> somepeer-with-long-name  5678    1604940    1367282     0 13:39:28    100/8000
> otherpeer            16584484      16420      89606     0 1d00h58m Active
> otherpeer-with-long- 16584485    1604940    1367282     0 13:40:17    100/8000
>
The attached patch does just that. Maximum line length is now 80
characters (excluding the trailing newline), and the "Neighbor" and "AS"
columns now take a combined maximum of 29 characters, including the
blank separating peer name and AS. The result looks like this:

$ bgpctl show
Neighbor                   AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
other-peer-with-l 16581.44851          0          0     0 Never    Connect
other-peer         1658.44841          0          0     0 Never    Active
some-peer-with-long-name 5678          0          0     0 Never    Active
some-peer               12345          0          0     0 Never    Active

Where before it was

$ bgpctl show
Neighbor                   AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
other-peer-with-long 16581.44851          0          0     0 Never    Connect
other-peer           1658.44841          0          0     0 Never    Active
some-peer-with-long-     5678          0          0     0 Never    Active
some-peer               12345          0          0     0 Never    Connect


--
        Gregor Best

bgpctl.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Claudio Jeker
In reply to this post by Stuart Henderson-6
On Sun, Jul 27, 2014 at 03:36:06PM +0100, Stuart Henderson wrote:

> On 2014/07/27 11:15, Claudio Jeker wrote:
> > Not a big fan since this makes the bgpctl show output no longer fit 80
> > chars and so will wrap lines on default terminals. While it is OK to
> > increase the size it should be taken away from other fields in some whay.
> > An option would be to drop the OutQ since that field has only limited
> > value IMO.
>
> oh, I would miss not having OutQ, it's totally useless when things are
> working correctly, but I have had a couple of situations (one with openbgp,
> one with *cough* bay BCN "some time ago"...) where this has been helpful.
> It gives quite a clear pointer to problems transmitting to that particular
> peer without having to look in netstat output and work out which peer is
> which (some physical layer problems, MTU problems etc).

In that case I would suggest to move that information to the bgpctl show
neighbor somepeer detail view. It shows more counters and is probably
better for debugging a particular peering that has issues.

bgpctl show sum should only give a summary to get an overview.

> I think the most usable display would be to have neighbor and AS "share"
> a column, with priority to AS... i.e.
>
> somepeer                12345      16419      89603     0 09:42:06    100/8000
> somepeer-with-long-name  5678    1604940    1367282     0 13:39:28    100/8000
> otherpeer            16584484      16420      89606     0 1d00h58m Active
> otherpeer-with-long- 16584485    1604940    1367282     0 13:40:17    100/8000
>

Yes, that would be better.
--
:wq Claudio

Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Stuart Henderson-6
In reply to this post by Gregor Best
On 2014/07/27 17:24, Gregor Best wrote:

> On Sun, Jul 27, 2014 at 03:36:06PM +0100, Stuart Henderson wrote:
> > On 2014/07/27 11:15, Claudio Jeker wrote:
> > > Not a big fan since this makes the bgpctl show output no longer fit 80
> > > chars and so will wrap lines on default terminals. While it is OK to
> > > increase the size it should be taken away from other fields in some whay.
> > > An option would be to drop the OutQ since that field has only limited
> > > value IMO.
> >
> > oh, I would miss not having OutQ, it's totally useless when things are
> > working correctly, but I have had a couple of situations (one with openbgp,
> > one with *cough* bay BCN "some time ago"...) where this has been helpful.
> > It gives quite a clear pointer to problems transmitting to that particular
> > peer without having to look in netstat output and work out which peer is
> > which (some physical layer problems, MTU problems etc).
> >
> > I think the most usable display would be to have neighbor and AS "share"
> > a column, with priority to AS... i.e.
> >
> > somepeer                12345      16419      89603     0 09:42:06    100/8000
> > somepeer-with-long-name  5678    1604940    1367282     0 13:39:28    100/8000
> > otherpeer            16584484      16420      89606     0 1d00h58m Active
> > otherpeer-with-long- 16584485    1604940    1367282     0 13:40:17    100/8000
> >
>
> The attached patch does just that. Maximum line length is now 80
> characters (excluding the trailing newline), and the "Neighbor" and "AS"
> columns now take a combined maximum of 29 characters, including the
> blank separating peer name and AS. The result looks like this:
>
> $ bgpctl show
> Neighbor                   AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
> other-peer-with-l 16581.44851          0          0     0 Never    Connect
> other-peer         1658.44841          0          0     0 Never    Active
> some-peer-with-long-name 5678          0          0     0 Never    Active
> some-peer               12345          0          0     0 Never    Active
>
> Where before it was
>
> $ bgpctl show
> Neighbor                   AS    MsgRcvd    MsgSent  OutQ Up/Down  State/PrfRcvd
> other-peer-with-long 16581.44851          0          0     0 Never    Connect
> other-peer           1658.44841          0          0     0 Never    Active
> some-peer-with-long-     5678          0          0     0 Never    Active
> some-peer               12345          0          0     0 Never    Connect

Nice, I like that a lot. What do you think Claudio?


> Index: bgpctl.c
> ===================================================================
> RCS file: /mnt/media/cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.174
> diff -u -p -u -r1.174 bgpctl.c
> --- bgpctl.c 18 Mar 2014 13:47:14 -0000 1.174
> +++ bgpctl.c 27 Jul 2014 15:20:59 -0000
> @@ -517,16 +517,26 @@ show_summary_msg(struct imsg *imsg, int
>  {
>   struct peer *p;
>   char *s;
> + const char *a;
> + char *pa;
> + size_t alen;
>  
>   switch (imsg->hdr.type) {
>   case IMSG_CTL_SHOW_NEIGHBOR:
>   p = imsg->data;
>   s = fmt_peer(p->conf.descr, &p->conf.remote_addr,
>      p->conf.remote_masklen, nodescr);
> - if (strlen(s) >= 20)
> - s[20] = 0;
> - printf("%-20s %8s %10llu %10llu %5u %-8s ",
> -    s, log_as(p->conf.remote_as),
> +
> + a = log_as(p->conf.remote_as);
> + alen = strlen(a);
> +
> + if (asprintf(&pa, "%-28s", s) == -1)
> + err(1, NULL);
> + if (strlen(pa) > 28 - alen)
> + pa[28 - alen] = 0;
> +
> + printf("%s %s %10llu %10llu %5u %-8s ",
> +    pa, a,
>      p->stats.msg_rcvd_open + p->stats.msg_rcvd_notification +
>      p->stats.msg_rcvd_update + p->stats.msg_rcvd_keepalive +
>      p->stats.msg_rcvd_rrefresh,
> @@ -544,6 +554,7 @@ show_summary_msg(struct imsg *imsg, int
>   else
>   printf("%s", statenames[p->state]);
>   printf("\n");
> + free(pa);
>   free(s);
>   break;
>   case IMSG_CTL_END:

Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Gregor Best
In reply to this post by Gregor Best
Ping? Are there remaining issues with the patch or should I leave
you guys alone until the release stress is over?

--
        Gregor Best

Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Gregor Best-2
In reply to this post by Stuart Henderson-6
Ping.

--
        Gregor Best

Reply | Threaded
Open this post in threaded view
|

Re: bgpctl: enlarge columns for 4-byte ASN display

Gregor Best
In reply to this post by Stuart Henderson-6
On Sun, Jul 27, 2014 at 10:06:12PM +0100, Stuart Henderson wrote:
> [...]
> Nice, I like that a lot. What do you think Claudio?
> [...]

Ping. Are there remaining issues with the patch?

--
        Gregor Best