Skip to content

Fix utime() syscall #16460

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 10, 2022
Merged

Fix utime() syscall #16460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ef5bd8c
Fix utime() syscall
tiran Mar 9, 2022
b37e3a5
Update test cases
tiran Mar 9, 2022
e83521f
Change check for times == NULL
tiran Mar 9, 2022
8c4fb68
Tolerate difference of +/- 5 seconds
tiran Mar 9, 2022
79c506b
Declare vars once
tiran Mar 9, 2022
d5921a9
Apply factor 1000 inside noderawfs layer
tiran Mar 9, 2022
154a2f3
Add debug code to test
tiran Mar 10, 2022
af928b0
Handle utimes(path, NULL) in wasmfs
tiran Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/library_noderawfs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ mergeInto(LibraryManager.library, {
}
fs.ftruncateSync.apply(void 0, arguments);
},
utime: function() { fs.utimesSync.apply(void 0, arguments); },
utime: function(path, atime, mtime) { fs.utimesSync(path, atime/1000, mtime/1000); },
open: function(path, flags, mode, suggestFD) {
if (typeof flags == "string") {
flags = VFS.modeStringToFlags(flags)
Expand Down
19 changes: 12 additions & 7 deletions src/library_syscall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,13 +1046,18 @@ var SyscallsLibrary = {
assert(flags === 0);
#endif
path = SYSCALLS.calculateAt(dirfd, path, true);
var seconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_sec, 'i32') }}};
var nanoseconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_nsec, 'i32') }}};
var atime = (seconds*1000) + (nanoseconds/(1000*1000));
times += {{{ C_STRUCTS.timespec.__size__ }}};
seconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_sec, 'i32') }}};
nanoseconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_nsec, 'i32') }}};
var mtime = (seconds*1000) + (nanoseconds/(1000*1000));
if (!times) {
var atime = Date.now();
var mtime = atime;
Copy link
Collaborator

@sbc100 sbc100 Mar 9, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@tiran tiran Mar 9, 2022

Choose a reason for hiding this comment

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

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

Copy link

@syrusakbary syrusakbary Mar 9, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sbc100 sbc100 Mar 9, 2022

Choose a reason for hiding this comment

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

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.

Copy link

@syrusakbary syrusakbary Mar 10, 2022

Choose a reason for hiding this comment

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

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

} else {
var seconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_sec, 'i32') }}};
var nanoseconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_nsec, 'i32') }}};
atime = (seconds*1000) + (nanoseconds/(1000*1000));
times += {{{ C_STRUCTS.timespec.__size__ }}};
seconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_sec, 'i32') }}};
nanoseconds = {{{ makeGetValue('times', C_STRUCTS.timespec.tv_nsec, 'i32') }}};
mtime = (seconds*1000) + (nanoseconds/(1000*1000));
}
FS.utime(path, atime, mtime);
return 0;
},
Expand Down
11 changes: 8 additions & 3 deletions system/lib/wasmfs/syscalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,10 +1006,15 @@ long __syscall_utimensat(int dirFD,

// TODO: Set tv_nsec (nanoseconds) as well.
// TODO: Handle tv_nsec being UTIME_NOW or UTIME_OMIT.
// TODO: Handle NULL times.
// TODO: Check for write access to the file (see man page for specifics).
auto aSeconds = times[0].tv_sec;
auto mSeconds = times[1].tv_sec;
time_t aSeconds, mSeconds;
if (times == NULL) {
aSeconds = time(NULL);
mSeconds = aSeconds;
} else {
aSeconds = times[0].tv_sec;
mSeconds = times[1].tv_sec;
Copy link
Collaborator

@sbc100 sbc100 Mar 10, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tiran tiran Mar 10, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@sbc100 sbc100 Mar 10, 2022

Choose a reason for hiding this comment

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

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

}

auto locked = parsed.getFile()->locked();
locked.setATime(aSeconds);
Expand Down
27 changes: 23 additions & 4 deletions tests/utime/test_utime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>
#include <utime.h>
#include <sys/stat.h>
Expand All @@ -32,20 +33,38 @@ void test() {
// will fail
struct utimbuf t = {1000000000, 1000000000};

utime("writeable", &t);
errno = 0;
Copy link
Collaborator

@sbc100 sbc100 Mar 9, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tiran tiran Mar 9, 2022

Choose a reason for hiding this comment

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

I have no clue ...

Copy link
Collaborator

@sbc100 sbc100 Mar 9, 2022

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

@tiran tiran Mar 10, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@sbc100 sbc100 Mar 10, 2022

Choose a reason for hiding this comment

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

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

int rv = utime("writeable", &t);
assert(rv == 0);
assert(!errno);
memset(&s, 0, sizeof s);
stat("writeable", &s);
rv = stat("writeable", &s);
assert(rv == 0);
assert(s.st_atime == t.actime);
assert(s.st_mtime == t.modtime);

// NULL sets atime and mtime to current time.
long now = time(NULL);
rv = utime("writeable", NULL);
assert(rv == 0);
memset(&s, 0, sizeof s);
stat("writeable", &s);
assert(s.st_atime == s.st_mtime);
long diff = s.st_atime - now;
if (abs(diff) > 5) {
fprintf(stderr, "st_atime: %li, now: %li, diff: %li\n ", s.st_atime, now, diff);
assert(abs(diff) <= 5);
}

// write permissions aren't checked when setting node
// attributes unless the user uid isn't the owner (so
// therefor, this should work fine)
utime("unwriteable", &t);
rv = utime("unwriteable", &t);
assert(rv == 0);
assert(!errno);
memset(&s, 0, sizeof s);
stat("unwriteable", &s);
rv = stat("unwriteable", &s);
assert(rv == 0);
assert(s.st_atime == t.actime);
assert(s.st_mtime == t.modtime);

Expand Down