Re: git: 7b35b4d19630 - main - sockstat: add libxo support

CC'ing Damin Rido.  I haven't had time to look at all of Phil's
observations yet, but I probably will this weekend.

On Fri, Aug 1, 2025 at 11:11 AM Phil Shafer <phil@juniper.net> wrote:

> This looks good.  Some issues below:
>
> On 30 Jul 2025, at 16:27, Alan Somers wrote:
> > @@ -1345,65 +1380,104 @@ display_sock(struct sock *s, struct col_widths
> *cw, char *buf, size_t bufsize)
> > ...
> >> +                       } else if (laddr->address.ss_len == 0 &&
> > +                               faddr->conn == 0 && is_text_style) {
> > +                               xo_emit(" {:/%-*.*s}", cw->local_addr,
> > +                                       cw->local_addr, "(not
> connected)");
> > +                       } else if (is_text_style) {
> > +                               xo_emit(" {:/%-*.*s}", cw->local_addr,
> > +                                       cw->local_addr, "??");
> > +                       }
>
> These calls are missing a field name; you should have seen warning for
> this, something like:
>
>   foo: missing field name: %-*.*s
>   <missing-field-name>??      </missing-field-name>
>
> You'll need something like "{:local-address/%*.*s}".
>
> Please be sure to test with "--libxo:W", which will report many issues.
> Also there's "xolint" for checking source code.
>

I can't trigger any warnings, with any combination of options.  Probably I
don't have the same type of socket that you do.  What kind of socket is
triggering warnings for you?


>
> Longer term, I'd like to make some sort of clang plugin to report
> formatting issues at build time.
>
> > @@ -1436,47 +1510,52 @@ display_sock(struct sock *s, struct col_widths
> *cw, char *buf, size_t bufsize)
> >                                     s->proto == IPPROTO_TCP) {
> >                                         switch (s->proto) {
> >                                         case IPPROTO_SCTP:
> > -                                               printf(" %-*s",
> cw->conn_state,
> > -
>  sctp_conn_state(s->state));
> > +                                               xo_emit("
> {:path-state/%-*s}",
> > +                                                       cw->path_state,
> > +                                                       sctp_path_state(
> > +
>  faddr->state));
> >                                                 break;
>
> Is the change from conn_state to path_state intentional?
>
>
> > +       if (xo_get_style(NULL) == XO_STYLE_TEXT) {
> > +               cw = (struct col_widths) {
> > ...
>
> Here and elsewhere: when you check for XO_STYLE_TEXT for formatting,
> you'll also care about XO_STYLE_HTML and other future styles.  I have a
> function in libxo that's currently private/static:
>
>         /*
>          * Indicate if the style is an "encoding" one as opposed to a
> "display" one.
>          */
>         static int
>         xo_style_is_encoding (xo_handle_t *xop)
>         {
>             if (xo_style(xop) == XO_STYLE_JSON
>                 || xo_style(xop) == XO_STYLE_XML
>                 || xo_style(xop) == XO_STYLE_SDPARAMS
>                 || xo_style(xop) == XO_STYLE_ENCODER)
>                 return TRUE;
>             return FALSE;
>         }
>
> I can make that public, allowing you to say "if
> (!xo_style_is_encoding(NULL)) { ... }", but for now, you'll need to
> explicitly test for XO_STYLE_HTML.
>
> > +     xo_error(
> > +"usage: sockstat [--libxo] [-46ACcfIiLlnqSsUuvw] [-j jid] [-p ports]\n"
> > +"                [-P protocols]\n");
> > +     exit(1);
>
> I don't have a real convention for this, but maybe "[--libxo ...]" would
> convey that there's content with the option?
>
> Thanks,
>  Phil
>