Page MenuHomeFreeBSD

Change w(1) to compute FROM (host) field size dynamically
Needs RevisionPublic

Authored by karels on Sun, Aug 11, 12:23 AM.

Details

Reviewers
marcel
markm
cem
Summary

It's nice to be able to display a full IPv6 host address if
needed, but it's also nice to display more than 3 characters of a command
line. Compute the needed size for the FROM column in an earlier pass,
and determine the maximum, then print what fits for the command.

Test Plan

tested on amd64

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25790
Build 24362: arc lint + arc unit

Event Timeline

karels created this revision.Sun, Aug 11, 12:23 AM
cem added a comment.Sun, Aug 11, 3:59 AM

Isn't changing the output of w a POLA violation?

markm accepted this revision.Sun, Aug 11, 9:38 AM

Very small complaints only.

usr.bin/w/w.c
99–100

Whitespace nit.

347

I find this mix of preprocessor code and "real" code annoying to eyeball-parse. Is there a way to make it a bit less jarring?

This revision is now accepted and ready to land.Sun, Aug 11, 9:38 AM

@cem: Not if it is an improvement on another change, and makes better use of the existing space.

usr.bin/w/w.c
99–100

I think this is an HTML rendering artifact; it's right in the code.

347

Yes, and it was wrong (fromwidth wasn't computed with -h). I'm moving it, but it makes it look like fromwidth is used before set (but it is not).

karels updated this revision to Diff 60649.Sun, Aug 11, 1:55 PM

minor corrections

This revision now requires review to proceed.Sun, Aug 11, 1:55 PM
markm accepted this revision.Sun, Aug 11, 6:33 PM
This revision is now accepted and ready to land.Sun, Aug 11, 6:33 PM
cem requested changes to this revision.Sun, Aug 11, 7:09 PM

@cem: Not if it is an improvement on another change, and makes better use of the existing space.

Hm, that is certainly not the standard we've used in the past.

This revision now requires changes to proceed.Sun, Aug 11, 7:09 PM

@cem: phabricator says "cem requested changes to this revision.", but I see no changes requested.

cem added a comment.Sun, Aug 11, 8:13 PM

@cem: phabricator says "cem requested changes to this revision.", but I see no changes requested.

Sorry that this was unclear. There is a limitation in phabricator to the kinds of negative feedback metadata one can provide. It's "request changes" or nothing. So I use "request changes" here to explicitly mark "not approved."

In this case, the concern is the change breaks common interactive uses and POLA of an ancient BSD command to work around user error. It adds even more magic to how column widths are computed when there should probably be less.

One less magical option would be to do something like Linux' procps-ng w and have environment variables to control the width of at least a subset of columns. w.1 on Linux documents PROCPS_USERLEN and PROCPS_FROMLEN environment variable controls. Something similar in this vein would be to make the desired magical behavior conditional on a flag, such as -m for Magical.

A high-level option would be to add support for the environment variable COLUMNS or flag(s) -ww to w rather than magical inference from TTY parameters. This would allow users stuck on 80-column terminals to view the full command with a scrolling pager, like the less we've shipped since May 2000.

usr.bin/w/w.c
99–100

Obligatory "fix the POLA" change-request comment in case it isn't clear from the "requests changes" annotation.

322–323

I agree with markm that macros that act on local variables should take them as explicit parameters rather than implicit references by name. E.g.,

#define WUSED(fromwidth) (W_DISPUSERSIZE + (fromwidth) + ... )
353–408

As long as we're moving this code, this is a fantastic time to create a subroutine instead of just pasting it bodily into the other function. This is exactly the kind and scale of code where a subroutine with specific intent helps improve clarity.

Sorry that this was unclear. There is a limitation in phabricator to the kinds of negative feedback metadata one can provide. It's "request changes" or nothing.

Maybe there is a reason for that.

In this case, the concern is the change breaks common interactive uses and POLA of an ancient BSD command to work around user error. It adds even more magic to how column widths are computed when there should probably be less.

I see no breakage or POLA here. I don't know what possible user error you are talking about; the error of creating a fixed-width field for something that rarely happens? There is no magic to computing the required width of a column, and BSD programs of this sort have been doing this for at least 30 years (see ls and column, which derives from it).

One less magical option would be to do something like Linux' procps-ng w and have environment variables to control the width of at least a subset of columns. w.1 on Linux documents PROCPS_USERLEN and PROCPS_FROMLEN environment variable controls. Something similar in this vein would be to make the desired magical behavior conditional on a flag, such as -m for Magical.

I won't bother to check how this Linux program works. Environment variables can't compute the required width of a column. Do we set PS_WIDTH_PID, PS_WIDTH_PPID, PS_WIDTH_UID, etc? Of course not.

A high-level option would be to add support for the environment variable COLUMNS or flag(s) -ww to w rather than magical inference from TTY parameters. This would allow users stuck on 80-column terminals to view the full command with a scrolling pager, like the less we've shipped since May 2000.

I don't know where this comes from. I did not change the overall line length, and I don't see that using TTY screen size is any more magical now than when added 30 years ago.

usr.bin/w/w.c
322–323

Mark didn't say that, but OK.

353–408

I don't agree. The code goes within an existing loop to set up an entry, and I see no point to turning it into a subroutine.

karels updated this revision to Diff 60660.Mon, Aug 12, 1:04 AM

change macro to take parameter

cem added 1 blocking reviewer(s): cem.Mon, Aug 12, 3:09 AM
cem requested changes to this revision.

Well, sarcasm and snide dismissal isn’t persuasive or refutive. Still Nack.

This revision now requires changes to proceed.Mon, Aug 12, 3:09 AM

Let me go back to the beginning in an attempt to unravel this. Let's start with Conrad's question:

"Isn't changing the output of w a POLA violation?"

Conrad: Please provide a concrete example of how you think this change violates POLA and why?

Conrad: it has now been more than a week since Marcel asked a question. If you can't or won't explain why you oppose this change, I think you need to drop your block on the review.