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 <p...@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
>
