Date: Fri, 1 Aug 2025 21:02:24 -0600 From: Alan Somers <asomers@freebsd.org> To: Phil Shafer <phil@juniper.net> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org, Damin Rido <rido@freebsd.org> Subject: Re: git: 7b35b4d19630 - main - sockstat: add libxo support Message-ID: <CAOtMX2jZYtY-X8Xs1aT_R2SrkR3QZXCSCUO-y_JWhKS_-j0kcQ@mail.gmail.com> In-Reply-To: <8F928557-328D-46E9-BB53-BDE216693BFC@juniper.net> References: <202507302027.56UKRhb5011315@gitrepo.freebsd.org> <8F928557-328D-46E9-BB53-BDE216693BFC@juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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
>
[-- Attachment #2 --]
<div dir="ltr"><div>CC'ing Damin Rido. I haven't had time to look at all of Phil's observations yet, but I probably will this weekend.</div><div><br></div><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Fri, Aug 1, 2025 at 11:11 AM Phil Shafer <<a href="mailto:phil@juniper.net">phil@juniper.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This looks good. Some issues below:<br>
<br>
On 30 Jul 2025, at 16:27, Alan Somers wrote:<br>
> @@ -1345,65 +1380,104 @@ display_sock(struct sock *s, struct col_widths *cw, char *buf, size_t bufsize)<br>
> ...<br>
>> + } else if (laddr->address.ss_len == 0 &&<br>
> + faddr->conn == 0 && is_text_style) {<br>
> + xo_emit(" {:/%-*.*s}", cw->local_addr,<br>
> + cw->local_addr, "(not connected)");<br>
> + } else if (is_text_style) {<br>
> + xo_emit(" {:/%-*.*s}", cw->local_addr,<br>
> + cw->local_addr, "??");<br>
> + }<br>
<br>
These calls are missing a field name; you should have seen warning for this, something like:<br>
<br>
foo: missing field name: %-*.*s<br>
<missing-field-name>?? </missing-field-name><br>
<br>
You'll need something like "{:local-address/%*.*s}".<br>
<br>
Please be sure to test with "--libxo:W", which will report many issues. Also there's "xolint" for checking source code.<br></blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Longer term, I'd like to make some sort of clang plugin to report formatting issues at build time.<br>
<br>
> @@ -1436,47 +1510,52 @@ display_sock(struct sock *s, struct col_widths *cw, char *buf, size_t bufsize)<br>
> s->proto == IPPROTO_TCP) {<br>
> switch (s->proto) {<br>
> case IPPROTO_SCTP:<br>
> - printf(" %-*s", cw->conn_state,<br>
> - sctp_conn_state(s->state));<br>
> + xo_emit(" {:path-state/%-*s}",<br>
> + cw->path_state,<br>
> + sctp_path_state(<br>
> + faddr->state));<br>
> break;<br>
<br>
Is the change from conn_state to path_state intentional?<br>
<br>
<br>
> + if (xo_get_style(NULL) == XO_STYLE_TEXT) {<br>
> + cw = (struct col_widths) {<br>
> ...<br>
<br>
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:<br>
<br>
/*<br>
* Indicate if the style is an "encoding" one as opposed to a "display" one.<br>
*/<br>
static int<br>
xo_style_is_encoding (xo_handle_t *xop)<br>
{<br>
if (xo_style(xop) == XO_STYLE_JSON<br>
|| xo_style(xop) == XO_STYLE_XML<br>
|| xo_style(xop) == XO_STYLE_SDPARAMS<br>
|| xo_style(xop) == XO_STYLE_ENCODER)<br>
return TRUE;<br>
return FALSE;<br>
}<br>
<br>
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.<br>
<br>
> + xo_error(<br>
> +"usage: sockstat [--libxo] [-46ACcfIiLlnqSsUuvw] [-j jid] [-p ports]\n"<br>
> +" [-P protocols]\n");<br>
> + exit(1);<br>
<br>
I don't have a real convention for this, but maybe "[--libxo ...]" would convey that there's content with the option?<br>
<br>
Thanks,<br>
Phil<br>
</blockquote></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2jZYtY-X8Xs1aT_R2SrkR3QZXCSCUO-y_JWhKS_-j0kcQ>