Page MenuHomeFreeBSD

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

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

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

Very small complaints only.

usr.bin/w/w.c
99 ↗(On Diff #60641)

Whitespace nit.

337 ↗(On Diff #60641)

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.Aug 11 2019, 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 ↗(On Diff #60641)

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

337 ↗(On Diff #60641)

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.Aug 11 2019, 1:55 PM

minor corrections

This revision now requires review to proceed.Aug 11 2019, 1:55 PM
markm accepted this revision.Aug 11 2019, 6:33 PM
This revision is now accepted and ready to land.Aug 11 2019, 6:33 PM
cem requested changes to this revision.Aug 11 2019, 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.Aug 11 2019, 7:09 PM

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

cem added a comment.Aug 11 2019, 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 ↗(On Diff #60649)

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

322–323 ↗(On Diff #60649)

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 ↗(On Diff #60649)

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 ↗(On Diff #60649)

Mark didn't say that, but OK.

353–408 ↗(On Diff #60649)

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.Aug 12 2019, 1:04 AM

change macro to take parameter

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

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

This revision now requires changes to proceed.Aug 12 2019, 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.

marcel removed a reviewer: cem.Aug 21 2019, 7:11 PM
This revision now requires review to proceed.Aug 21 2019, 7:11 PM
marcel accepted this revision.Aug 21 2019, 7:12 PM

Resolving stalemate.

This revision is now accepted and ready to land.Aug 21 2019, 7:12 PM
This revision was automatically updated to reflect the committed changes.