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!
