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>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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 == 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 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’t trigger any warnings with
--libxo:W. However, to stay consistent with xolint expectations and ensure
correctness across all styles, I’ll add proper field names in my next
commit.
>
>> > @@ -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?
>>
>
The change from conn-state to path-state was a mistake. Thanks for catching
that. I’ll revert it in my next commit.
>
>> > + 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.
>>
>>
I initially assumed that only TEXT is intended for human-readable output,
and that XML, JSON, and HTML are all machine-readable formats. That’s 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’t be necessary for HTML or other formats.
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’ll update it to use "[--libxo ...]" to indicate
that there are options. Thanks for the suggestion!
[-- Attachment #2 --]
<div dir="ltr">Thank you for your valuable feedback.<div dir="ltr"><br></div><div class="gmail_quote gmail_quote_container"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> @@ -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></div></blockquote><div><br></div><div>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’t trigger any warnings with --libxo:W. However, to stay consistent with xolint expectations and ensure correctness across all styles, I’ll add proper field names in my next commit.</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"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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></blockquote></div></div></blockquote><div><br></div><div>The change from conn-state to path-state was a mistake. Thanks for catching that. I’ll revert it in my next commit.</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"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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></blockquote></div></div></blockquote><div><br></div><div>I initially assumed that only TEXT is intended for human-readable output, and that XML, JSON, and HTML are all machine-readable formats. That’s 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’t be necessary for HTML or other formats. Could you please clarify why HTML should be handled similarly to TEXT in this case?</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"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + 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></blockquote></div></div></blockquote><div><br>Sure, that makes sense. I’ll update it to use "[--libxo ...]" to indicate that there are options. Thanks for the suggestion!</div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAODom%2BaViZ3L4ES05NUeFaXwns3=-gfy6GZB-dtGgPuVC1K2sQ>