Skip to content

Removed cast to integer in getSize#5744

Merged
MorrisJobke merged 2 commits into
nextcloud:masterfrom
1manprojects:patch-1
Jul 18, 2017
Merged

Removed cast to integer in getSize#5744
MorrisJobke merged 2 commits into
nextcloud:masterfrom
1manprojects:patch-1

Conversation

@1manprojects

Copy link
Copy Markdown
Contributor

Fixes - Wrong or no sizes of files/folders #5031 for 32-bit systems a direct cast to integer causes problems.

Fixes - Wrong or no sizes of files/folders nextcloud#5031
@mention-bot

Copy link
Copy Markdown

@1manprojects, thanks for your PR! By analyzing the history of the files in this pull request, we identified @icewind1991, @nickvergessen and @DeepDiver1975 to be potential reviewers.

@derkostka

Copy link
Copy Markdown
Contributor

Works ! Please consider for 12.0.1 @MorrisJobke
👍

@MorrisJobke

Copy link
Copy Markdown
Member

Was added in #4001 🙈

cc @nickvergessen @rullzer @icewind1991

@icewind1991 icewind1991 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is causing problems on 32bit systems it should be changed to 0 + instead of just removing it

@1manprojects

Copy link
Copy Markdown
Contributor Author

How would a change to 0+ help in this situation. PHP will automacily change a integer value on overflow to a float unless it is casted. Casting the filesize directly to int would always couse problems on certain files if they are large enough.

@icewind1991

Copy link
Copy Markdown
Member

doing a 0 + will ensure that the value is cast to a numeric value if it is a string (which is, from what I can tell, the reason the cast is there in the first place) without running into int size issues

@codecov

codecov Bot commented Jul 17, 2017

Copy link
Copy Markdown

Codecov Report

Merging #5744 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #5744      +/-   ##
============================================
- Coverage     53.86%   53.85%   -0.01%     
  Complexity    22755    22755              
============================================
  Files          1405     1405              
  Lines         86711    86711              
  Branches       1328     1328              
============================================
- Hits          46705    46699       -6     
- Misses        40006    40012       +6
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/FileInfo.php 91.86% <100%> (ø) 60 <0> (ø) ⬇️
core/js/js.js 61.27% <0%> (-0.56%) 0% <0%> (ø)
apps/files_external/lib/Lib/Storage/SMB.php 50.94% <0%> (-0.38%) 116% <0%> (ø)

@1manprojects

Copy link
Copy Markdown
Contributor Author

Thanks for the info , @icewind1991. I changed the commit so it will be cast to a numeric value with 0 +.

@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish backport-request bug enhancement labels Jul 18, 2017
@rullzer rullzer added this to the Nextcloud 13 milestone Jul 18, 2017
@MorrisJobke MorrisJobke merged commit 5accbf5 into nextcloud:master Jul 18, 2017
@MorrisJobke

Copy link
Copy Markdown
Member

@1manprojects Could I ask you to open another PR against stable12 for the backport? Thanks

@MorrisJobke

Copy link
Copy Markdown
Member

And congratulations to your first contribution to Nextcloud 🎉

@alexhass

alexhass commented Jul 23, 2017

Copy link
Copy Markdown

Please backport to 12.0.1. Why is this 32bit only? As I know I have a Debian 9 with 64bit and see the issue.

@1manprojects

Copy link
Copy Markdown
Contributor Author

Well technically this could also affect 64-bit system if a foldersize of filesize is large then the 64-bit integer range however this is a very large number.

@alexhass

Copy link
Copy Markdown

I can only say that I have e.g 550 files of 10-20mb each in a folder and the sum counter on upper folder overview is zero. There is no file thst is 2gb or larger in the folder. All files have just max 25mb. In a sum it is nearly 7.2gb in these folders. I also have orher folders with smaller files and less mb and they are also 0mb in nextcloud web interface. I hope i do not need to wait until 13.0 to see this issue resolved.

@derkostka

Copy link
Copy Markdown
Contributor

It must be a different issue then, you would need billions of files of that size to reach the 64 Bit, today, this is impossible.

Are you sure that you have a 64 Bit Php installation?

@alexhass

alexhass commented Jul 24, 2017

Copy link
Copy Markdown

Not sure... how can I find out? The OS is 64bit... Debian 9. I suspect every package is x64 that is embedded, but I could be wrong. Let me know how to find out, please.

System | Linux 3.16.0-042stab123.1 #1 SMP Wed Mar 22 15:21:30 MSK 2017 x86_64

@nickvergessen

Copy link
Copy Markdown
Member

Just run:

php -r "echo PHP_INT_MAX;"

If the result is 9223372036854775807 its 64, if it is 4294967296 you have 32 bits

@alexhass

Copy link
Copy Markdown

Ok, 64bit php as expected... should I debug something on DB level to help you identify the root cause of the 0 counters? I have made a brand new install with 12.0. No upgrade... I'm have only 1 week expierence with nextcloud itself.

@alexhass

Copy link
Copy Markdown

I tested sudo -u www-data php occ files:scan and now the counters are no longer 0. However this does not explain how this could have happened. I guess I do not need a cron task that runs files:scan, isn't it? It should be done automatically on upload... All my files have been uploaded via drag and drop to a brand new v12 installation. And I hope HTTP2 is not a possible reason for this...

@derkostka

Copy link
Copy Markdown
Contributor

Good. Now you've done several tests. I think if there is a problem then it is not related to this change. Http2 is unrelated, as you expect, and the files scan is not needed to be run within cron.

I'd ask you to open a new ticket and attach a detailed log, if that is possible. Maybe we can find the root cause that way.

derkostka added a commit to derkostka/server that referenced this pull request Jul 29, 2017
Fixes - Wrong or no sizes of files/folders nextcloud#5031 for 32-bit systems a direct cast to integer causes problems.
Backport from nextcloud#5744

Signed-off-by: Sebastian Kostka <sebastian.kostka@gmail.com>
@derkostka

Copy link
Copy Markdown
Contributor

Created a PR (#5925) for 12.0.1, please review. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants