-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
android 64-bit compatibility adaptions #5821
Changes from 4 commits
885683a
7d1981a
b61c20c
c86084f
97bfa59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -642,7 +642,7 @@ impl FsMeta for StatFs { | |
not(target_arch = "s390x"), | ||
target_pointer_width = "64" | ||
))] | ||
return self.f_bsize; | ||
return self.f_bsize; // i64 -> i64 - OK | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure those comments are needed, the Rust compiler is here to tell us if this is ok or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it there to help the reviewer/developer that reads this code to understand what happens. There are a lot of #[cfg(..)] around there and its not directly visible what the main point in the three cases is. But I'll remove it if you insist on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just expressing a preference, I'll let the coreutils maintainers weight on this.
cre4ture marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[cfg(all( | ||
not(target_env = "musl"), | ||
not(target_os = "freebsd"), | ||
|
@@ -655,7 +655,7 @@ impl FsMeta for StatFs { | |
not(target_pointer_width = "64") | ||
) | ||
))] | ||
return self.f_bsize.into(); | ||
return self.f_bsize.into(); // i32 or u32 -> i64 - OK | ||
cre4ture marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[cfg(any( | ||
target_env = "musl", | ||
target_os = "freebsd", | ||
|
@@ -664,7 +664,7 @@ impl FsMeta for StatFs { | |
target_os = "redox", | ||
all(target_os = "android", target_pointer_width = "64"), | ||
))] | ||
return self.f_bsize.try_into().unwrap(); | ||
return self.f_bsize.try_into().unwrap(); // u64 -> i64 - might fail if number is too high | ||
cre4ture marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
fn total_blocks(&self) -> u64 { | ||
#[cfg(target_pointer_width = "64")] | ||
|
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.
Do we really have sometimes a number of inodes greater than
u64::MAX
?If this is the case, is it ok to compute a wrong value by saturating the addition?
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.
this peace of code does an accumulation. As far as I understood the code, this accumulation is purely needed to compute an optional finalising row called "total" in the output. This "total" row shall display the sum of each column. Thus, also the inodes are accumulated.
On my phones system, there are apparently several filesystems that reports "inodes" as exactly
u64::MAX
. Summing that up always causes an int 64 overflow. But is it relevant? I guess not really as the number is anyway insanely high. If someone finds a usecase where this is relevant, we can still extend the integer type tou128
.Alternatively. one could automatically filter the filesystem with
u64::MAX
because the number might anyway not be valid.EDIT: One addition could be that we don't print the value in the total row when it is at u64::MAX because the sum is invalid. That might be the easiest and also correct behavior as long as we don't extent to u128.
EDIT2: The feature to compute the "total" row is apparently not available in the df implementation on my phone (
toybox 0.8.4-android
)Output of pre-installed (toybox, NOT GNU) of df on my phone:
for completion the output of uutils:
filesystem types:
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.
This looks awfully similar to a -1 cast to
uint64_t
…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.
Could you retry with
--total
as well? That would be interesting to see how GNU coreutils perform.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 did some editing to give more detail. So please read again. The df-variant that runs on my phone is "toybox" and not GNU. It doesn't support the "--total" argument.
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 created a small image for erofs on my ubuntu PC. Then you can see the GNU output:
its placing some dashes "-" and EDIT: its having a silent integer-overlfow for the IFree. I checked it, the real sum without "erofs" would be "21093966" and not "21093963" ... thats 3 off.
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.
by the way, the inodes for erofs are not really relevant in general as it is a read only filesystem
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.
My test on Ubuntu prooves that the issue with the integeroverflow is actually also visible on non-android systems if they use erofs. So we could actually split this change into an own pull-request.