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

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!