Fix utime() syscall #16460

tiran · 2022-03-09T18:47:42Z

Fix off by factor 1000: FS.utime() expects mtime and atime in seconds.

times == NULL is now correctly handled. utimes(path, NULL)
updates atime and mtime to current time.

https://nodejs.org/api/fs.html#fsutimespath-atime-mtime-callback

Fixes: #16458
Signed-off-by: Christian Heimes christian@python.org

Fix off by factor 1000: ``FS.utime()`` expects mtime and atime in seconds. ``times == NULL`` is now correctly handled. ``utimes(path, NULL)`` updates atime and mtime to current time. https://nodejs.org/api/fs.html#fsutimespath-atime-mtime-callback Fixes: emscripten-core#16458 Signed-off-by: Christian Heimes <christian@python.org>

sbc100

Thanks!

I'm curious to see if any tests fail.. if they don't it means we lack coverage.

src/library_syscall.js

tiran · 2022-03-09T19:49:21Z

Thanks!

I'm curious to see if any tests fail.. if they don't it means we lack coverage.

I compiled test_utime.c manually and patched the JS code. The test failed for me because errno was not reset to 0. I wonder if the test is actually executed...

The test file now resets errno, checks return values, and tests for utime(path, NULL)

sbc100 · 2022-03-09T20:23:36Z

tests/utime/test_utime.c

@@ -32,20 +33,35 @@ void test() {
  // will fail
  struct utimbuf t = {1000000000, 1000000000};

-  utime("writeable", &t);
+  errno = 0;


Hmm .. if this line is needed.. how was the test passing before?

I have no clue ...

What test are you running that fails if you remove this line?

./tests/runner core2.test_utime works fine for on HEAD.

I executed the test manually with $ emcc -s NODERAWFS -o test_utime.js test_utime.c && node test_utime.js. The test failed with error message RuntimeError: Aborted(Assertion failed: !errno, at: test_utime.c,37,test). My guess is that some function earlier left errno != 0.

From man errno(3):

The value in errno is significant only when the return value of the call indicated an error (i.e., -1 from most system calls; -1 or NULL from most library functions); a function that succeeds is allowed to change errno. The value of errno is never set to zero by any system call or library function.

In other words, if you want to use errno for error checking, then you have to reset errno to 0 before a function call and check it immediately after the function call.

Yes, I'm only confused about how that test was passing before.

It sounds like the tests passes because its not run with NODERAWFS today? i.e. ./tests/runner core2.test_utime does not set NODERAWFS. Perhaps we should annotate that test with also_with_noderawfs

sbc100 · 2022-03-09T20:25:59Z

src/library_syscall.js

-    var mtime = (seconds*1000) + (nanoseconds/(1000*1000));
+    if (!times) {
+      var atime = Date.now()/1000;
+      var mtime = atime;


Seems a little odd to declare var mtime and var atime in both branches of the if/else. Maybe just declare them in one? (I'm not a JS expert but IIRC you just need one var decl for a give variable it it will be hoisted to function level).

I don't know any JS at all. :)

mtime and atime should be declared on level up (var mtime, atime;).
Otherwise the FS.utime(path, atime, mtime) will not be able to access the variables on the scope.

Is that true? I thought var declaration were hoisted to the function level in JS. This seems to work just fine:

function foo() { if (0) { var bar = 10; } else { bar = 7; } console.error(bar); }

It looks odd but its saves a few bytes in code size.

You are completely right! I got a bit confused with var and let :P

sbc100

lgtm % comments

tiran · 2022-03-09T21:29:13Z

Tests are failing.

The noderawfs layer defines the utime function as function() { fs.utimesSync.apply(void 0, arguments); }. According to https://nodejs.org/api/fs.html#fsutimessyncpath-atime-mtime and https://nodejs.org/api/fs.html#fsutimespath-atime-mtime-callback the utimesSync function accepts the time arguments as seconds, not milliseconds:

The atime and mtime arguments follow these rules:
Values can be either numbers representing Unix epoch time in seconds, Dates, or a numeric string like '123456789.0'.

Perhaps the values must be adjusted in the noderawfs layer instead?

tiran · 2022-03-10T14:04:38Z

Tests are no passing. I had to implement the same fix in wasmfs layer.

sbc100

lgtm! We can try to figure out why the missing errno = 0 was ok in the past but not any more as a followup.

sbc100 · 2022-03-10T16:29:12Z

system/lib/wasmfs/syscalls.cpp

+    mSeconds = aSeconds;
+  } else {
+    aSeconds = times[0].tv_sec;
+    mSeconds = times[1].tv_sec;


I'm curious how / why this was needed? Do we run test_utime under WASMFS? Or did you try running manually with this flag?

This was needed to implement utime(path, NULL) for wasmfs. The PR fixes utime(path, times) on noderawfs and implements utime(path, NULL) for all fs layers.

test_utime was failing on test_core.wasmfs, https://app.circleci.com/pipelines/github/emscripten-core/emscripten/19754/workflows/57871d6d-c5f8-4f0c-83ae-2566038f9724/jobs/513149

At thanks.. i had forgotten that we can run the whole of the core tests suite in wasmfs mode

sbc100 reviewed Mar 9, 2022

View reviewed changes

src/library_syscall.js Outdated Show resolved Hide resolved

tiran added 2 commits March 9, 2022 20:44

Update test cases

b37e3a5

Change check for times == NULL

e83521f

Tolerate difference of +/- 5 seconds

8c4fb68

tiran marked this pull request as ready for review March 9, 2022 19:55

sbc100 reviewed Mar 9, 2022

View reviewed changes

sbc100 approved these changes Mar 9, 2022

View reviewed changes

Declare vars once

79c506b

tiran added 3 commits March 9, 2022 22:38

Apply factor 1000 inside noderawfs layer

d5921a9

Add debug code to test

154a2f3

Handle utimes(path, NULL) in wasmfs

af928b0

tiran requested a review from sbc100 March 10, 2022 14:04

sbc100 approved these changes Mar 10, 2022

View reviewed changes

sbc100 reviewed Mar 10, 2022

View reviewed changes

sbc100 merged commit a7b10da into emscripten-core:main Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix utime() syscall #16460

Fix utime() syscall #16460

tiran commented Mar 9, 2022

sbc100 left a comment

tiran commented Mar 9, 2022

sbc100 Mar 9, 2022

tiran Mar 9, 2022

sbc100 Mar 9, 2022

tiran Mar 10, 2022

sbc100 Mar 10, 2022

sbc100 Mar 9, 2022

tiran Mar 9, 2022

syrusakbary Mar 9, 2022

sbc100 Mar 9, 2022

syrusakbary Mar 10, 2022

sbc100 left a comment

tiran commented Mar 9, 2022

tiran commented Mar 10, 2022

sbc100 left a comment

sbc100 Mar 10, 2022

tiran Mar 10, 2022

sbc100 Mar 10, 2022

Fix utime() syscall #16460

Fix utime() syscall #16460

Conversation

tiran commented Mar 9, 2022

sbc100 left a comment

Choose a reason for hiding this comment

tiran commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

tiran commented Mar 9, 2022

tiran commented Mar 10, 2022

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment