FreeBSD Mail Archives

Date:      Sat, 2 Aug 2025 11:29:49 +0530
From:      Damin Rido <daminrido139@gmail.com>
To:        Alan Somers <asomers@freebsd.org>
Cc:        Phil Shafer <phil@juniper.net>, 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:  <CAODom%2BaViZ3L4ES05NUeFaXwns3=-gfy6GZB-dtGgPuVC1K2sQ@mail.gmail.com>
In-Reply-To: <CAOtMX2jZYtY-X8Xs1aT_R2SrkR3QZXCSCUO-y_JWhKS_-j0kcQ@mail.gmail.com>
References:  <202507302027.56UKRhb5011315@gitrepo.freebsd.org> <8F928557-328D-46E9-BB53-BDE216693BFC@juniper.net> <CAOtMX2jZYtY-X8Xs1aT_R2SrkR3QZXCSCUO-y_JWhKS_-j0kcQ@mail.gmail.com>

--000000000000de5d04063b5b9587
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Thank you for your valuable feedback.

> @@ -1345,65 +1380,104 @@ display_sock(struct sock *s, struct col_widths
>> *cw, char *buf, size_t bufsize)
>> > ...
>> >> +                       } else if (laddr->address.ss_len =3D=3D 0 &&
>> > +                               faddr->conn =3D=3D 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 deliberately omitted field names in some xo_emit calls when the output
style was XO_STYLE_TEXT, since I assumed field names wouldn't matter for
human-readable formats. I also couldn=E2=80=99t trigger any warnings with
--libxo:W. However, to stay consistent with xolint expectations and ensure
correctness across all styles, I=E2=80=99ll add proper field names in my ne=
xt
commit.


>
>> > @@ -1436,47 +1510,52 @@ display_sock(struct sock *s, struct col_widths
>> *cw, char *buf, size_t bufsize)
>> >                                     s->proto =3D=3D 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_stat=
e(
>> > +
>>  faddr->state));
>> >                                                 break;
>>
>> Is the change from conn_state to path_state intentional?
>>
>
The change from conn-state to path-state was a mistake. Thanks for catching
that. I=E2=80=99ll revert it in my next commit.


>
>> > +       if (xo_get_style(NULL) =3D=3D XO_STYLE_TEXT) {
>> > +               cw =3D (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) =3D=3D XO_STYLE_JSON
>>                 || xo_style(xop) =3D=3D XO_STYLE_XML
>>                 || xo_style(xop) =3D=3D XO_STYLE_SDPARAMS
>>                 || xo_style(xop) =3D=3D 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.
>>
>>
I initially assumed that only TEXT is intended for human-readable output,
and that XML, JSON, and HTML are all machine-readable formats. That=E2=80=
=99s why I
treated TEXT separately in my formatting logic. Also, I use space-padding
specifically for XO_STYLE_TEXT to produce a neat, structured, table-like
output, which I assumed wouldn=E2=80=99t be necessary for HTML or other for=
mats.
Could you please clarify why HTML should be handled similarly to TEXT in
this case?


> > +     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?
>>
>>
Sure, that makes sense. I=E2=80=99ll update it to use "[--libxo ...]" to in=
dicate
that there are options. Thanks for the suggestion!

--000000000000de5d04063b5b9587
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Thank you for your valuable feedback.<div dir=3D"ltr"><br>=
</div><div class=3D"gmail_quote gmail_quote_container"><blockquote class=3D=
"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(2=
04,204,204);padding-left:1ex"><div dir=3D"ltr"><div class=3D"gmail_quote"><=
blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l=
eft:1px solid rgb(204,204,204);padding-left:1ex">
&gt; @@ -1345,65 +1380,104 @@ display_sock(struct sock *s, struct col_width=
s *cw, char *buf, size_t bufsize)<br>
&gt; ...<br>
&gt;&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0} else if (laddr-&gt;address.ss_len =3D=3D 0 &amp;&amp;=
<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0faddr-&gt;conn =3D=3D 0 &amp;&amp=
; is_text_style) {<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0xo_emit(&quot; {:/%-*.*s}&quot;, =
cw-&gt;local_addr,<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cw-&g=
t;local_addr, &quot;(not connected)&quot;);<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0} else if (is_text_style) {<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0xo_emit(&quot; {:/%-*.*s}&quot;, =
cw-&gt;local_addr,<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cw-&g=
t;local_addr, &quot;??&quot;);<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0}<br>
<br>
These calls are missing a field name; you should have seen warning for this=
, something like:<br>
<br>
=C2=A0 foo: missing field name: %-*.*s<br>
=C2=A0 &lt;missing-field-name&gt;??=C2=A0 =C2=A0 =C2=A0 &lt;/missing-field-=
name&gt;<br>
<br>
You&#39;ll need something like &quot;{:local-address/%*.*s}&quot;.<br>
<br>
Please be sure to test with &quot;--libxo:W&quot;, which will report many i=
ssues.=C2=A0 Also there&#39;s &quot;xolint&quot; for checking source code.<=
br></blockquote></div></div></blockquote><div><br></div><div>I deliberately=
 omitted field names in some xo_emit calls when the output style was XO_STY=
LE_TEXT, since I assumed field names wouldn&#39;t matter for human-readable=
 formats. I also couldn=E2=80=99t trigger any warnings with --libxo:W. Howe=
ver, to stay consistent with xolint expectations and ensure correctness acr=
oss all styles, I=E2=80=99ll add proper field names in my next commit.</div=
><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px=
 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div di=
r=3D"ltr"><div class=3D"gmail_quote"><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex"><br>
&gt; @@ -1436,47 +1510,52 @@ display_sock(struct sock *s, struct col_widths=
 *cw, char *buf, size_t bufsize)<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s-&gt;proto =
=3D=3D IPPROTO_TCP) {<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0switch (s-&gt;proto) {<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0case IPPROTO_SCTP:<br>
&gt; -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0printf(&quot; %-*s&quot;, cw-&gt;conn_state,<br>
&gt; -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sctp_conn_state(s-&gt;state));<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0xo_emit(&quot; {:path-state/%-*s}&quot;,<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cw-&gt;path_state,<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sctp_path_state(<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0faddr-&gt;state));<br>
&gt;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;<br>
<br>
Is the change from conn_state to path_state intentional?<br></blockquote></=
div></div></blockquote><div><br></div><div>The change from conn-state to pa=
th-state was a mistake. Thanks for catching that. I=E2=80=99ll revert it in=
 my next commit.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padd=
ing-left:1ex"><div dir=3D"ltr"><div class=3D"gmail_quote"><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex">
<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (xo_get_style(NULL) =3D=3D XO_STYLE_TEX=
T) {<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cw =3D (struct=
 col_widths) {<br>
&gt; ...<br>
<br>
Here and elsewhere: when you check for XO_STYLE_TEXT for formatting, you&#3=
9;ll also care about XO_STYLE_HTML and other future styles.=C2=A0 I have a =
function in libxo that&#39;s currently private/static:<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /*<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* Indicate if the style is an &quot;encod=
ing&quot; one as opposed to a &quot;display&quot; one.<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 static int<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 xo_style_is_encoding (xo_handle_t *xop)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 {<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (xo_style(xop) =3D=3D XO_STYLE=
_JSON<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 || xo_style(xop) =
=3D=3D XO_STYLE_XML<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 || xo_style(xop) =
=3D=3D XO_STYLE_SDPARAMS<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 || xo_style(xop) =
=3D=3D XO_STYLE_ENCODER)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return TRUE;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return FALSE;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }<br>
<br>
I can make that public, allowing you to say &quot;if (!xo_style_is_encoding=
(NULL)) { ... }&quot;, but for now, you&#39;ll need to explicitly test for =
XO_STYLE_HTML.<br>
<br></blockquote></div></div></blockquote><div><br></div><div>I initially a=
ssumed that only TEXT is intended for human-readable output, and that XML, =
JSON, and HTML are all machine-readable formats. That=E2=80=99s why I treat=
ed TEXT separately in my formatting logic. Also, I use space-padding specif=
ically for XO_STYLE_TEXT to produce a neat, structured, table-like output, =
which I assumed wouldn=E2=80=99t be necessary for HTML or other formats. Co=
uld you please clarify why HTML should be handled similarly to TEXT in this=
 case?</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"mar=
gin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1=
ex"><div dir=3D"ltr"><div class=3D"gmail_quote"><blockquote class=3D"gmail_=
quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,=
204);padding-left:1ex">
&gt; +=C2=A0 =C2=A0 =C2=A0xo_error(<br>
&gt; +&quot;usage: sockstat [--libxo] [-46ACcfIiLlnqSsUuvw] [-j jid] [-p po=
rts]\n&quot;<br>
&gt; +&quot;=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 [-P pro=
tocols]\n&quot;);<br>
&gt; +=C2=A0 =C2=A0 =C2=A0exit(1);<br>
<br>I don&#39;t have a real convention for this, but maybe &quot;[--libxo .=
..]&quot; would convey that there&#39;s content with the option?<br><br></b=
lockquote></div></div></blockquote><div><br>Sure, that makes sense. I=E2=80=
=99ll update it to use &quot;[--libxo ...]&quot; to indicate that there=C2=
=A0are options. Thanks for the suggestion!</div></div></div>

--000000000000de5d04063b5b9587--

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAODom%2BaViZ3L4ES05NUeFaXwns3=-gfy6GZB-dtGgPuVC1K2sQ>

Header And Logo

Peripheral Links

Site Navigation

Header And Logo

Peripheral Links

Search

Site Navigation