-
Notifications
You must be signed in to change notification settings - Fork 3.1k
df: Correct BLOCKSIZE with -k #1906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for taking the time to contribute to FreeBSD! |
|
@bsdimp, @concussious can you review this ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bin/df/df.c
Outdated
| /* | ||
| * POSIX specifically discusses the behavior of | ||
| * both -k and -P. It states that the blocksize should | ||
| * be set to 1024. Thus, if this occurs, simply break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the last sentence Thus, if this occurs, simply break rather than clobbering the old blocksize. should be removed.
| if (kflag) { | ||
| setenv("BLOCKSIZE", "1024", 1); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this } needs to be indented using 3 horizontal tabulation characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should i make these changes and squash it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok final :
- Remove this one :
Thus, if this occurs, simply break ather than clobbering the old blocksize. - and this } needs to be indented using 3 horizontal tabulation characters.
|
@jlduran can u review this ?? I think I did 3 horizontal tab indent this part by mistake is it ok ? |
bin/df/df.c
Outdated
| */ | ||
| if (kflag) { | ||
| setenv("BLOCKSIZE", "1024", 1); | ||
| if (kflag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes in indentation are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there anything can I do to fix this back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I usually do is copy over the original file, open it split-screen and copy it back.
No, those changes should be reverted. |
|
I did this :
|
|
@jlduran is it ok now ? |
|
So, this is changing the behavior to match POSIX? By 1k do you mean 1024 or 1000? |
by 1k I mean 1024 |
|
Then please use ISO IEC designation, that's KiB, not K. |
but what I found: |
|
I guess this is fine by me, if @concussious or @bsdimp have further suggestions, they have the last word. |
|
@jlduran ok I do have to squash into 1 commit ?? can you help me out with the title of commit ? |
This patch resolves inconsistent behavior between the -k option and other related flags, including -P. Previously, using -k resulted in output displayed in 1024-byte blocks, which did not align with the behavior of similar options such as -m and -g, where output is shown in 1M-blocks and 1G-blocks respectively. The updated implementation ensures that -k now correctly displays sizes in 1K-blocks. In addition, the patch incorporates the POSIX requirement that when both -k -P are specified, the block size must be explicitly forced to 1024-blocks. Together, these changes make the behavior of -k consistent, predictable, and compliant with the standard. Signed-off-by: Ankush Mondal <[email protected]>
I'm terrible at commit titles, something along the lines of:
|
Yes, and we should use 1024. The international standard for referring to 1024 that is called KiB. K or k is ambiguous because macOS and storage manufacturers refer to k as 1000. When scaled to modern infrastructure, measured in PiB, it's a 12.5% difference, hundreds of terabytes. We use KiB widely in the tree already: I even fixed this in our upstream already: So, if we're changing the display text, the current proposed change is a regression. 1024 is specific, KiB is specific, but K or k is just not specific. |
Actually, do you have a link? |
i see 'k' is kind of ambiguous |
https://unix.stackexchange.com/questions/179274/what-does-1k-blocks-column-mean-in-the-output-of-df |
This kind of stuff is very low level, very old, and has a lot of dependencies. When were talking about this, you must go to the actual spec, not stack exchange or Gemini. Here is the spec: https://pubs.opengroup.org/onlinepubs/9699919799/ It is very specific about output, but does not mention how to refer to it. Edit: the most recent version: https://pubs.opengroup.org/onlinepubs/9799919799/ |
|
I spoke with some of the other committers, and we couldn't get a consensus to change it. Thank you for your submission Ankush! |
|
I beg to differ. I'd planned on landing this |
This patch resolves inconsistent behavior between the -k option and other related flags, including -P. Previously, using -k resulted in output displayed in 1024-byte blocks, which did not align with the behavior of similar options such as -m and -g, where output is shown in 1M-blocks and 1G-blocks respectively.
The updated implementation ensures that -k now correctly displays sizes in 1K-blocks. In addition, the patch incorporates the POSIX requirement that when both -k -P are specified, the block size must be explicitly forced to 1024-blocks.
Together, these changes make the behavior of -k consistent, predictable, and compliant with the standard.
[BUG: 290710]