Re: git: 94567c8641e2 - main - netstat: Resolve duplicate "dropped-packets" key from libxo output

On Sun, Sep 22, 2024 at 11:29 AM Mark Johnston <markj@freebsd.org> wrote:

> On Fri, Sep 20, 2024 at 03:58:10PM +0000, Warner Losh wrote:
> > The branch main has been updated by imp:
> >
> > URL:
> https://cgit.FreeBSD.org/src/commit/?id=94567c8641e235763b5b2926416d89d36654cde1
> >
> > commit 94567c8641e235763b5b2926416d89d36654cde1
> > Author:     Bram <bram@cbbg.nl>
> > AuthorDate: 2024-07-23 08:57:42 +0000
> > Commit:     Warner Losh <imp@FreeBSD.org>
> > CommitDate: 2024-09-20 15:06:26 +0000
> >
> >     netstat: Resolve duplicate "dropped-packets" key from libxo output
> >
> >     The current libxo output uses the "dropped-packets" key for both
> incoming and
> >     outgoing packets. This commit distinguishes between the two by
> adding an "in"
> >     and "out" suffix. The original "dropped-packets" key is kept for
> backwards
> >     compatibility for now.
> >
> >     PR: 244589
> >     Reviewed by: imp, zlei
> >     Pull Request: https://github.com/freebsd/freebsd-src/pull/1331
> > ---
> >  usr.bin/netstat/if.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/usr.bin/netstat/if.c b/usr.bin/netstat/if.c
> > index 172ea5324ccb..f0af785bce04 100644
> > --- a/usr.bin/netstat/if.c
> > +++ b/usr.bin/netstat/if.c
> > @@ -501,8 +501,11 @@ intpr(void (*pfunc)(char *), int af)
> >                   IFA_STAT(ipackets), link|network, 1);
> >               show_stat("lu", nerr_len, "received-errors",
> IFA_STAT(ierrors),
> >                   link, 1);
> > +             /* Below is kept for backwards compatibility. Will be
> removed in the future. */
> >               show_stat("lu", nerr_len, "dropped-packets",
> IFA_STAT(iqdrops),
> >                   link, 1);
> > +             show_stat("lu", nerr_len, "dropped-packets-in",
> IFA_STAT(iqdrops),
> > +                 link, 1);
>
> This breaks plain netstat output, causing a bunch of network tests to
> fail.  For instance, "netstat -Wf link -I re0" now prints an extra
> column.
>
> I think we should not bother with backwards compat unless libxo is
> emitting structured output.  This can be implemented using the "e"
> format modifier described in the xo_format modified, but I'm not sure
> how to plumb it through show_stat().
>
> Let's revert until this is fixed?
>

Works for me. I hadn't noticed before the push. Thanks for bringing it up.

Warner


> >               if (bflag)
> >                       show_stat("lu", nbyte_len, "received-bytes",
> >                           IFA_STAT(ibytes), link|network, 0);
> > @@ -516,7 +519,7 @@ intpr(void (*pfunc)(char *), int af)
> >               show_stat("NRSlu", nerr_len, "collisions",
> IFA_STAT(collisions),
> >                   link, 1);
> >               if (dflag)
> > -                     show_stat("LSlu", nerr_len, "dropped-packets",
> > +                     show_stat("LSlu", nerr_len, "dropped-packets-out",
> >                           IFA_STAT(oqdrops), link, 1);
> >               xo_emit("\n");
> >
> > @@ -705,8 +708,11 @@ loop:
> >           new->ift_ip - old->ift_ip, 1, 1);
> >       show_stat("lu", 5, "received-errors",
> >           new->ift_ie - old->ift_ie, 1, 1);
> > +     /* Below is kept for backwards compatibility. Will be removed in
> the future. */
> >       show_stat("lu", 5, "dropped-packets",
> >           new->ift_id - old->ift_id, 1, 1);
> > +     show_stat("lu", 5, "dropped-packets-in",
> > +         new->ift_id - old->ift_id, 1, 1);
> >       show_stat("lu", 10, "received-bytes",
> >           new->ift_ib - old->ift_ib, 1, 0);
> >       show_stat("lu", 10, "sent-packets",
> > @@ -718,7 +724,7 @@ loop:
> >       show_stat("NRSlu", 5, "collisions",
> >           new->ift_co - old->ift_co, 1, 1);
> >       if (dflag)
> > -             show_stat("LSlu", 5, "dropped-packets",
> > +             show_stat("LSlu", 5, "dropped-packets-out",
> >                   new->ift_od - old->ift_od, 1, 1);
> >       xo_close_instance("stats");
> >       xo_emit("\n");
>