Handle BSD case for 32 bit filemtime and install warning #28759

phil-davis · 2017-08-22T03:23:51Z

Description

Handle the BSD (and Mac) case in lib/private/Files/Storage/Local.php filemtime
That was performing a Linux-style stat command only.

Make a generic runningOn convenience function and refactor existing code that did special strpos() stuff to detect the OS.

Remove some legacy tests for "win" that were still in getFileSizeViaExec

Rearrange install OS error checks in Setup.php so that the error (really a warning) will be displayed for any OS that is not linux-type.

Related Issue

#28758
#28760

Motivation and Context

BSD stat command has different options to Linux

How Has This Been Tested?

Copied function runningOnBSD() onto my pfSense FreeBSD system and tried calling it manually.
It returns true for that, should also work for "OpenBSD" or other variants.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

phil-davis · 2017-08-22T03:26:14Z

Note: The filemtime() 32-bit check was introduced by PR #25428 for issue #18971 but that code only had a stat command for Linux.

DeepDiver1975 · 2017-08-22T05:51:36Z

Nice:+1:

DeepDiver1975 · 2017-08-22T10:09:31Z

lib/private/Files/Storage/Local.php

@@ -180,7 +180,12 @@ public function filemtime($path) {
 			return false;
 		}
 		if (PHP_INT_SIZE === 4) {
-			return (int) exec ('stat -c %Y '. escapeshellarg ($fullPath));
+			if (\OC_Util::runningOnLinux()) {
+				return (int) exec ('stat -c %Y '. escapeshellarg ($fullPath));


Unrelated to this change ...

Isn't it stupid to call stat to get values higher then 32 bit int and then case that to a 32bit int?

A good question! That "rounds" off the answer at max int.

Not quite as stupid as it looks. The problem is that for files of SIZE > 2GB on 32-bit systems, even filemtime() fails:
http://php.net/manual/en/function.filemtime.php#68814

The file SIZE might be too big for a 32-bit signed int, but mtime will fit OK until 2038.

Since filemtime() returns an int, I guess whoever wrote this also returned an int. But it would not matter to just remove the (int) cast.

phil-davis · 2017-08-22T13:33:40Z

Rebased - CI should be passing on latest master.

phil-davis · 2017-08-22T15:00:45Z

@DeepDiver1975 @mmattel I refactored this based on comments in the other related PR #28761 and included the little bit of Setup.php code from there. The refactoring meant that I had to touch Setup.php anyway.
Please re-review.

mmattel · 2017-08-22T16:01:38Z

Code looks good, not tested 👍
Jenkins: docker: Error response from daemon: grpc: the connection is unavailable...

phil-davis · 2017-08-23T01:17:18Z

Rebased again and Jenkins still dies early. I guess there will be no CI until someone is available to look at it.

DeepDiver1975 · 2017-08-23T09:33:59Z

lib/private/legacy/util.php

 	 */
-	public static function runningOnMac() {
-		return (strtoupper(substr(PHP_OS, 0, 6)) === 'DARWIN');
+	public static function runningOn($osType) {


a little bit less code ;-)

$osType = strtolower($osType) == 'mac' ? 'darwin' : strtolower($osType); return (strtolower(substr(PHP_OS, 0, strlen($osType))) === $osType);

bsd case is different, because for that it is not necessarily at the start of the string. On my pfSense:

$x = PHP_OS; var_dump($x); string(7) "FreeBSD"

so it does not all fit nicely into a single check (although they could all do strpos() to just check if PHP_OS contains 'linux', 'darwin' or 'bsd' somewhere)

good point - didnt look that close - sorry

PVince81 · 2017-08-23T09:43:19Z

lib/private/legacy/util.php

+		switch($osType) {
+			case 'linux':
+				return (strtolower(substr(PHP_OS, 0, 5)) === 'linux');
+				break;


no need to break if you return directly

phil-davis · 2017-08-23T16:01:44Z

I refactored it - maybe it is more obscure now?

But it is "flexible" - by default it checks for the parameter matching the start of PHP_OS. It can be passed 'lin' (matching a Linux...) or 'darw' (matching a 'darwin...') or 'free' (matching 'FreeBSD').
And it knows about the special cases of 'mac' => 'darwin' and 'bsd' can come anywhere in PHP_OS.

But also, I can't imagine using the "flexibility" any time soon.

mmattel · 2017-08-24T06:41:31Z

👍

phil-davis · 2017-08-24T08:58:46Z

@PVince81 or @DeepDiver1975 do either of you want to give this a final approval and press merge?
Or change or revert back something here?

phil-davis · 2017-08-24T12:07:07Z

Backport stable10 #28790
since this makes the setup warning more generic, applying to all attempts at installing on a non-linux-style platform.

lock · 2019-08-02T17:03:41Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

This was referenced Aug 22, 2017

CLI Upgrade: stat: illegal option -- c #28758

Closed

Give install warning on all OS other than Linux #28761

Closed

phil-davis self-assigned this Aug 22, 2017

phil-davis added the 3 - To Review label Aug 22, 2017

phil-davis added this to the development milestone Aug 22, 2017

phil-davis requested a review from DeepDiver1975 August 22, 2017 10:03

DeepDiver1975 requested changes Aug 22, 2017

View reviewed changes

phil-davis force-pushed the stat-c-os-dependent branch from 725e114 to f7fc09c Compare August 22, 2017 13:33

DeepDiver1975 approved these changes Aug 22, 2017

View reviewed changes

phil-davis force-pushed the stat-c-os-dependent branch from f7fc09c to e6aaf3c Compare August 22, 2017 14:53

phil-davis changed the title ~~Handle BSD case for 32 bit filemtime~~ Handle BSD case for 32 bit filemtime and install warning Aug 22, 2017

phil-davis force-pushed the stat-c-os-dependent branch from e6aaf3c to e0d6c0f Compare August 22, 2017 14:59

BSD case for 32 bit filemtime and install warning

ab0ec67

phil-davis force-pushed the stat-c-os-dependent branch from e0d6c0f to ab0ec67 Compare August 23, 2017 01:12

DeepDiver1975 reviewed Aug 23, 2017

View reviewed changes

PVince81 reviewed Aug 23, 2017

View reviewed changes

refactor runningOn()

982b54f

PVince81 merged commit df60e3a into master Aug 24, 2017

PVince81 deleted the stat-c-os-dependent branch August 24, 2017 11:37

phil-davis mentioned this pull request Aug 24, 2017

[stable10] Handle BSD case for 32 bit filemtime and install warning #28790

Merged

phil-davis mentioned this pull request Aug 24, 2017

No warning given when installing on BSD #28760

Closed

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle BSD case for 32 bit filemtime and install warning #28759

Handle BSD case for 32 bit filemtime and install warning #28759

phil-davis commented Aug 22, 2017 •

edited

Loading

phil-davis commented Aug 22, 2017

DeepDiver1975 commented Aug 22, 2017

DeepDiver1975 Aug 22, 2017

phil-davis Aug 22, 2017 •

edited

Loading

phil-davis Aug 22, 2017 •

edited

Loading

phil-davis commented Aug 22, 2017

phil-davis commented Aug 22, 2017 •

edited

Loading

mmattel commented Aug 22, 2017

phil-davis commented Aug 23, 2017

DeepDiver1975 Aug 23, 2017

phil-davis Aug 23, 2017 •

edited

Loading

DeepDiver1975 Aug 23, 2017

PVince81 Aug 23, 2017

phil-davis commented Aug 23, 2017

mmattel commented Aug 24, 2017

phil-davis commented Aug 24, 2017

phil-davis commented Aug 24, 2017

lock bot commented Aug 2, 2019

Handle BSD case for 32 bit filemtime and install warning #28759

Handle BSD case for 32 bit filemtime and install warning #28759

Conversation

phil-davis commented Aug 22, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

phil-davis commented Aug 22, 2017

DeepDiver1975 commented Aug 22, 2017

DeepDiver1975 Aug 22, 2017

Choose a reason for hiding this comment

phil-davis Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

phil-davis Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

phil-davis commented Aug 22, 2017

phil-davis commented Aug 22, 2017 • edited Loading

mmattel commented Aug 22, 2017

phil-davis commented Aug 23, 2017

DeepDiver1975 Aug 23, 2017

Choose a reason for hiding this comment

phil-davis Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

DeepDiver1975 Aug 23, 2017

Choose a reason for hiding this comment

PVince81 Aug 23, 2017

Choose a reason for hiding this comment

phil-davis commented Aug 23, 2017

mmattel commented Aug 24, 2017

phil-davis commented Aug 24, 2017

phil-davis commented Aug 24, 2017

lock bot commented Aug 2, 2019

phil-davis commented Aug 22, 2017 •

edited

Loading

phil-davis Aug 22, 2017 •

edited

Loading

phil-davis Aug 22, 2017 •

edited

Loading

phil-davis commented Aug 22, 2017 •

edited

Loading

phil-davis Aug 23, 2017 •

edited

Loading