vfs: implement utimensat() and futimens()
70 views
Skip to first unread message
Jaspal Singh Dhillon
Jun 12, 2014, 5:59:31 AM6/12/14
to osv...@googlegroups.com
Hello,
I am trying to implement utimensat() and futimens() functions and need a bit of help
in setting the timespec arguments.
1. How do we get the current time for UTIME_NOW?
2. How do we deal with UTIME_OMIT? I could think of two ways, using vn_stat(heavy) or modifying
vn_settimes() to accept which attrs to modify ( ATIME or MTIME or both ).
Also, man page of utimes() states that it should work with NULL as argument. This does
not happen in the present implementation and the tst-utimes also does not cover it.
Thanks,
Jaspal
---
fs/vfs/main.cc | 50 +++++++++++++++++++++
fs/vfs/vfs.h | 3 ++
fs/vfs/vfs_syscalls.cc | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 171 insertions(+)
diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index 076e633..6077f13 100644
--- a/fs/vfs/main.cc
+++ b/fs/vfs/main.cc
@@ -1561,6 +1561,56 @@ int futimesat(int dirfd, const char *pathname, const struct timeval times[2])
return -1;
}
+TRACEPOINT(trace_vfs_utimensat, "\"%s\"", const char*);
+TRACEPOINT(trace_vfs_utimensat_ret, "");
+TRACEPOINT(trace_vfs_utimensat_err, "%d", int);
+
+extern "C"
+int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+
+ trace_vfs_utimensat(pathname);
+
+ error = sys_utimensat(dirfd, pathname, times, flags);
+ if (error)
+ goto out_errno;
+
+ trace_vfs_utimensat_ret();
+ return 0;
+
+ out_errno:
+ trace_vfs_utimensat_err(error);
+ errno = error;
+ return -1;
+
+}
+
+TRACEPOINT(trace_vfs_futimens, "%d", int);
+TRACEPOINT(trace_vfs_futimens_ret, "");
+TRACEPOINT(trace_vfs_futimens_err, "%d", int);
+
+extern "C"
+int futimens(int fd, const struct timespec times[2])
+{
+ int error;
+
+ trace_vfs_futimens(fd);
+
+ error = sys_futimens(fd, times);
+ if (error)
+ goto out_errno;
+
+ trace_vfs_futimens_ret();
+ return 0;
+
+ out_errno:
+ trace_vfs_futimens_err(error);
+ errno = error;
+ return -1;
+
+}
+
extern "C"
int utimes(const char *pathname, const struct timeval times[2])
{
diff --git a/fs/vfs/vfs.h b/fs/vfs/vfs.h
index 8023807..d31e2fa 100644
--- a/fs/vfs/vfs.h
+++ b/fs/vfs/vfs.h
@@ -117,6 +117,9 @@ int sys_statfs(char *path, struct statfs *buf);
int sys_truncate(char *path, off_t length);
ssize_t sys_readlink(char *path, char *buf, size_t bufsize);
int sys_utimes(char *path, const struct timeval times[2]);
+int sys_utimensat(int dirfd, const char *pathname,
+ const struct timespec times[2], int flags);
+int sys_futimens(int fd, const struct timespec times[2]);
int sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len);
int sys_mount(char *dev, char *dir, char *fsname, int flags, void *data);
diff --git a/fs/vfs/vfs_syscalls.cc b/fs/vfs/vfs_syscalls.cc
index 9c76604..e21ec24 100644
--- a/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs_syscalls.cc
@@ -1036,6 +1036,124 @@ sys_utimes(char *path, const struct timeval times[2])
return error;
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec *time)
+{
+ return (time->tv_sec >= 0) &&
+ ((time->tv_nsec >= 0 && time->tv_nsec <= 999999999) ||
+ time->tv_nsec == UTIME_NOW ||
+ time->tv_nsec == UTIME_OMIT);
+}
+
+void init_timespec(struct timespec &_times, const struct timespec ×)
+{
+
+ _times.tv_sec = times.tv_sec;
+ if (times.tv_nsec == UTIME_NOW) {
+ // TODO
+ } else if (times.tv_nsec == UTIME_OMIT) {
+ // TODO
+ } else {
+ _times.tv_nsec = times.tv_nsec;
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct dentry *dp;
+ char *absolute_path;
+ struct file *fp;
+ int length;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+
+ if (times && (!is_timespec_valid(×[0]) || !is_timespec_valid(×[1])))
+ return EINVAL;
+
+ init_timespec(timespec_times[0],times[0]);
+ init_timespec(timespec_times[1],times[1]);
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if (pathname && pathname[0] == '/') {
+ absolute_path = (char*)malloc(strlen(pathname));
+ strcpy(absolute_path, pathname);
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ length = strlen(main_task->t_cwd) + strlen(pathname) + 2;
+ absolute_path = (char*)malloc(length);
+ snprintf(absolute_path, length, "%s/%s", main_task->t_cwd, pathname);
+ } else {
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR)){
+ return ENOTDIR;
+ }
+
+ length = strlen(fp->f_dentry->d_path) + strlen(pathname) + 2;
+ absolute_path = (char*)malloc(length);
+
+ if (pathname)
+ snprintf(absolute_path, length, "%s/%s", fp->f_dentry->d_path, pathname);
+ else
+ strcpy(absolute_path, pathname);
+ fdrop(fp);
+ }
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ error = namei(absolute_path, &dp);
+ free(absolute_path);
+
+ if (error)
+ return error;
+
+ if (dp->d_mount->m_flags & MNT_RDONLY) {
+ error = EROFS;
+ } else {
+ /* FIXME: Check write permissions on file when UTIME_NOW is specified for times */
+ error = vn_settimes(dp->d_vnode, timespec_times);
+ }
+
+ drele(dp);
+ return error;
+}
+
+int
+sys_futimens(int fd, const struct timespec times[2])
+{
+ struct file *fp;
+ int error;
+ char *pathname;
+
+ error = fget(fd, &fp);
+ if (error)
+ return error;
+
+ if (!fp->f_dentry)
+ return EBADF;
+
+ pathname = (char*)malloc(strlen(fp->f_dentry->d_path));
+ strcpy(pathname, fp->f_dentry->d_path);
+ error = sys_utimensat(AT_FDCWD, pathname, times, 0);
+ free(pathname);
+ return error;
+}
+
int
sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len)
{
--
1.9.0
I am trying to implement utimensat() and futimens() functions and need a bit of help
in setting the timespec arguments.
1. How do we get the current time for UTIME_NOW?
2. How do we deal with UTIME_OMIT? I could think of two ways, using vn_stat(heavy) or modifying
vn_settimes() to accept which attrs to modify ( ATIME or MTIME or both ).
Also, man page of utimes() states that it should work with NULL as argument. This does
not happen in the present implementation and the tst-utimes also does not cover it.
Thanks,
Jaspal
---
fs/vfs/main.cc | 50 +++++++++++++++++++++
fs/vfs/vfs.h | 3 ++
fs/vfs/vfs_syscalls.cc | 118 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 171 insertions(+)
diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index 076e633..6077f13 100644
--- a/fs/vfs/main.cc
+++ b/fs/vfs/main.cc
@@ -1561,6 +1561,56 @@ int futimesat(int dirfd, const char *pathname, const struct timeval times[2])
return -1;
}
+TRACEPOINT(trace_vfs_utimensat, "\"%s\"", const char*);
+TRACEPOINT(trace_vfs_utimensat_ret, "");
+TRACEPOINT(trace_vfs_utimensat_err, "%d", int);
+
+extern "C"
+int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+
+ trace_vfs_utimensat(pathname);
+
+ error = sys_utimensat(dirfd, pathname, times, flags);
+ if (error)
+ goto out_errno;
+
+ trace_vfs_utimensat_ret();
+ return 0;
+
+ out_errno:
+ trace_vfs_utimensat_err(error);
+ errno = error;
+ return -1;
+
+}
+
+TRACEPOINT(trace_vfs_futimens, "%d", int);
+TRACEPOINT(trace_vfs_futimens_ret, "");
+TRACEPOINT(trace_vfs_futimens_err, "%d", int);
+
+extern "C"
+int futimens(int fd, const struct timespec times[2])
+{
+ int error;
+
+ trace_vfs_futimens(fd);
+
+ error = sys_futimens(fd, times);
+ if (error)
+ goto out_errno;
+
+ trace_vfs_futimens_ret();
+ return 0;
+
+ out_errno:
+ trace_vfs_futimens_err(error);
+ errno = error;
+ return -1;
+
+}
+
extern "C"
int utimes(const char *pathname, const struct timeval times[2])
{
diff --git a/fs/vfs/vfs.h b/fs/vfs/vfs.h
index 8023807..d31e2fa 100644
--- a/fs/vfs/vfs.h
+++ b/fs/vfs/vfs.h
@@ -117,6 +117,9 @@ int sys_statfs(char *path, struct statfs *buf);
int sys_truncate(char *path, off_t length);
ssize_t sys_readlink(char *path, char *buf, size_t bufsize);
int sys_utimes(char *path, const struct timeval times[2]);
+int sys_utimensat(int dirfd, const char *pathname,
+ const struct timespec times[2], int flags);
+int sys_futimens(int fd, const struct timespec times[2]);
int sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len);
int sys_mount(char *dev, char *dir, char *fsname, int flags, void *data);
diff --git a/fs/vfs/vfs_syscalls.cc b/fs/vfs/vfs_syscalls.cc
index 9c76604..e21ec24 100644
--- a/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs_syscalls.cc
@@ -1036,6 +1036,124 @@ sys_utimes(char *path, const struct timeval times[2])
return error;
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec *time)
+{
+ return (time->tv_sec >= 0) &&
+ ((time->tv_nsec >= 0 && time->tv_nsec <= 999999999) ||
+ time->tv_nsec == UTIME_NOW ||
+ time->tv_nsec == UTIME_OMIT);
+}
+
+void init_timespec(struct timespec &_times, const struct timespec ×)
+{
+
+ _times.tv_sec = times.tv_sec;
+ if (times.tv_nsec == UTIME_NOW) {
+ // TODO
+ } else if (times.tv_nsec == UTIME_OMIT) {
+ // TODO
+ } else {
+ _times.tv_nsec = times.tv_nsec;
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct dentry *dp;
+ char *absolute_path;
+ struct file *fp;
+ int length;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+
+ if (times && (!is_timespec_valid(×[0]) || !is_timespec_valid(×[1])))
+ return EINVAL;
+
+ init_timespec(timespec_times[0],times[0]);
+ init_timespec(timespec_times[1],times[1]);
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if (pathname && pathname[0] == '/') {
+ absolute_path = (char*)malloc(strlen(pathname));
+ strcpy(absolute_path, pathname);
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ length = strlen(main_task->t_cwd) + strlen(pathname) + 2;
+ absolute_path = (char*)malloc(length);
+ snprintf(absolute_path, length, "%s/%s", main_task->t_cwd, pathname);
+ } else {
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR)){
+ return ENOTDIR;
+ }
+
+ length = strlen(fp->f_dentry->d_path) + strlen(pathname) + 2;
+ absolute_path = (char*)malloc(length);
+
+ if (pathname)
+ snprintf(absolute_path, length, "%s/%s", fp->f_dentry->d_path, pathname);
+ else
+ strcpy(absolute_path, pathname);
+ fdrop(fp);
+ }
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ error = namei(absolute_path, &dp);
+ free(absolute_path);
+
+ if (error)
+ return error;
+
+ if (dp->d_mount->m_flags & MNT_RDONLY) {
+ error = EROFS;
+ } else {
+ /* FIXME: Check write permissions on file when UTIME_NOW is specified for times */
+ error = vn_settimes(dp->d_vnode, timespec_times);
+ }
+
+ drele(dp);
+ return error;
+}
+
+int
+sys_futimens(int fd, const struct timespec times[2])
+{
+ struct file *fp;
+ int error;
+ char *pathname;
+
+ error = fget(fd, &fp);
+ if (error)
+ return error;
+
+ if (!fp->f_dentry)
+ return EBADF;
+
+ pathname = (char*)malloc(strlen(fp->f_dentry->d_path));
+ strcpy(pathname, fp->f_dentry->d_path);
+ error = sys_utimensat(AT_FDCWD, pathname, times, 0);
+ free(pathname);
+ return error;
+}
+
int
sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len)
{
--
1.9.0
Nadav Har'El
Jun 12, 2014, 6:10:58 AM6/12/14
to Jaspal Singh Dhillon, Osv Dev
On Thu, Jun 12, 2014 at 12:59 PM, Jaspal Singh Dhillon <jaspal...@gmail.com> wrote:
Hello,
I am trying to implement utimensat() and futimens() functions and need a bit of help
in setting the timespec arguments.
1. How do we get the current time for UTIME_NOW?
Probably the easiest is to use the POSIX API, clock_gettime(), which returns the time already in timespec format.
Alternatively, you can use OSv's C++11-based time API, described in https://github.com/cloudius-systems/osv/wiki/Telling-the-time. You can also see an example of how to use that OSv API in our implementation of clock_gettime(), which is pretty trivial.
Alternatively, you can use OSv's C++11-based time API, described in https://github.com/cloudius-systems/osv/wiki/Telling-the-time. You can also see an example of how to use that OSv API in our implementation of clock_gettime(), which is pretty trivial.
2. How do we deal with UTIME_OMIT? I could think of two ways, using vn_stat(heavy) or modifying
vn_settimes() to accept which attrs to modify ( ATIME or MTIME or both ).
I think the best and easiest would be to modify vn_settimes() to do something like this:
vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? AT_ATIME : 0)
| ((times[1].tv_nsec == UTIME_OMIT) ? AT_MTIME : 0)
That is, unless we make sure all the VOP_SETATTR implementations already check for UTIME_OMIT? Can you please look perhaps they do?
| ((times[1].tv_nsec == UTIME_OMIT) ? AT_MTIME : 0)
That is, unless we make sure all the VOP_SETATTR implementations already check for UTIME_OMIT? Can you please look perhaps they do?
Also, man page of utimes() states that it should work with NULL as argument. This does
not happen in the present implementation and the tst-utimes also does not cover it.
Good catch - would be nice if you fix it :-) (in a separate patch)
Jaspal
Jun 12, 2014, 6:40:57 AM6/12/14
to Nadav Har'El, Osv Dev
On 06/12/2014 03:40 PM, Nadav Har'El
wrote:
On Thu, Jun 12, 2014 at 12:59 PM, Jaspal Singh Dhillon <jaspal...@gmail.com> wrote:
Hello,
I am trying to implement utimensat() and futimens() functions and need a bit of help
in setting the timespec arguments.
1. How do we get the current time for UTIME_NOW?
Probably the easiest is to use the POSIX API, clock_gettime(), which returns the time already in timespec format.
Alternatively, you can use OSv's C++11-based time API, described in https://github.com/cloudius-systems/osv/wiki/Telling-the-time. You can also see an example of how to use that OSv API in our implementation of clock_gettime(), which is pretty trivial.
2. How do we deal with UTIME_OMIT? I could think of two ways, using vn_stat(heavy) or modifying
vn_settimes() to accept which attrs to modify ( ATIME or MTIME or both ).
I think the best and easiest would be to modify vn_settimes() to do something like this:
vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? AT_ATIME : 0)
| ((times[1].tv_nsec == UTIME_OMIT) ? AT_MTIME : 0)
That is, unless we make sure all the VOP_SETATTR implementations already check for UTIME_OMIT? Can you please look perhaps they do?
I don't think that the only implementation at present for
VOP_SETATTR in zfs_vnops.c , zfs_setattr() checks for UTIME_OMIT
anywhere in its implementation because besides being defined in
stat.h, it is not used/checked anywhere!
Nadav Har'El
Jun 12, 2014, 6:45:54 AM6/12/14
to Jaspal, Osv Dev
On Thu, Jun 12, 2014 at 1:40 PM, Jaspal <jaspal...@gmail.com> wrote:
I don't think that the only implementation at present for VOP_SETATTR in zfs_vnops.c , zfs_setattr() checks for UTIME_OMIT anywhere in its implementation because besides being defined in stat.h, it is not used/checked anywhere!
vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? AT_ATIME : 0)
| ((times[1].tv_nsec == UTIME_OMIT) ? AT_MTIME : 0)
That is, unless we make sure all the VOP_SETATTR implementations already check for UTIME_OMIT? Can you please look perhaps they do?
Good, so I think it's easiest to check it in one place as I suggested above.
Jaspal Singh Dhillon
Jun 12, 2014, 8:12:52 AM6/12/14
to osv...@googlegroups.com
Add utimensat() and futimens() which can update file timestamps with
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
fs/vfs/main.cc | 50 +++++++++++++++++++
fs/vfs/vfs.h | 3 ++
fs/vfs/vfs_syscalls.cc | 127 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/vfs/vfs_vnode.c | 4 +-
4 files changed, 182 insertions(+), 2 deletions(-)
index 9c76604..79398ed 100644
--- a/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs_syscalls.cc
@@ -1036,6 +1036,133 @@ sys_utimes(char *path, const struct timeval times[2])
+{
+
+ } else if (times.tv_nsec != UTIME_OMIT) {
+ _times.tv_sec = times.tv_sec;
+ clock_gettime(CLOCK_REALTIME, ×pec_times[0]);
+ clock_gettime(CLOCK_REALTIME, ×pec_times[1]);
+ }
+ /* FIXME: Add support to follow symbolic links */
+ return EACCES;
+ if (times &&
+ (times[0].tv_nsec != UTIME_NOW || times[1].tv_nsec != UTIME_NOW) &&
+ (times[0].tv_nsec != UTIME_OMIT || times[1].tv_nsec != UTIME_OMIT) &&
+ (!(dp->d_vnode->v_mode & ~VAPPEND)))
+ return EPERM;
diff --git a/fs/vfs/vfs_vnode.c b/fs/vfs/vfs_vnode.c
index cbed3fc..1ada346 100644
--- a/fs/vfs/vfs_vnode.c
+++ b/fs/vfs/vfs_vnode.c
@@ -400,8 +400,8 @@ vn_settimes(struct vnode *vp, struct timespec times[2])
vap->va_atime = times[0];
vap->va_mtime = times[1];
- vap->va_mask = AT_ATIME | AT_MTIME;
-
+ vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? 0 : AT_ATIME)
+ | ((times[1].tv_nsec == UTIME_OMIT) ? 0 : AT_MTIME);
vn_lock(vp);
error = VOP_SETATTR(vp, vap);
vn_unlock(vp);
--
1.9.0
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
fs/vfs/main.cc | 50 +++++++++++++++++++
fs/vfs/vfs.h | 3 ++
fs/vfs/vfs_syscalls.cc | 127 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/vfs/vfs_vnode.c | 4 +-
4 files changed, 182 insertions(+), 2 deletions(-)
--- a/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs_syscalls.cc
@@ -1036,6 +1036,133 @@ sys_utimes(char *path, const struct timeval times[2])
return error;
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec *time)
+{
+ return (time->tv_sec >= 0) &&
+ ((time->tv_nsec >= 0 && time->tv_nsec <= 999999999) ||
+ time->tv_nsec == UTIME_NOW ||
+ time->tv_nsec == UTIME_OMIT);
+}
+
+void init_timespec(struct timespec &_times, const struct timespec times)
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec *time)
+{
+ return (time->tv_sec >= 0) &&
+ ((time->tv_nsec >= 0 && time->tv_nsec <= 999999999) ||
+ time->tv_nsec == UTIME_NOW ||
+ time->tv_nsec == UTIME_OMIT);
+}
+
+{
+
+ if (times.tv_nsec == UTIME_NOW) {
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else if (times.tv_nsec != UTIME_OMIT) {
+ _times.tv_sec = times.tv_sec;
+ _times.tv_nsec = times.tv_nsec;
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct dentry *dp;
+ char *absolute_path;
+ struct file *fp;
+ int length;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+
+ if (times && (!is_timespec_valid(×[0]) || !is_timespec_valid(×[1])))
+ return EINVAL;
+
+ if (times) {
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct dentry *dp;
+ char *absolute_path;
+ struct file *fp;
+ int length;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+
+ if (times && (!is_timespec_valid(×[0]) || !is_timespec_valid(×[1])))
+ return EINVAL;
+
+ init_timespec(timespec_times[0],times[0]);
+ init_timespec(timespec_times[1],times[1]);
+ } else {
+ init_timespec(timespec_times[1],times[1]);
+ clock_gettime(CLOCK_REALTIME, ×pec_times[0]);
+ clock_gettime(CLOCK_REALTIME, ×pec_times[1]);
+ }
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ error = namei(absolute_path, &dp);
+ free(absolute_path);
+
+ if (error)
+ return error;
+
+ if (dp->d_mount->m_flags & MNT_RDONLY) {
+ error = EROFS;
+ } else {
+ if (!(dp->d_vnode->v_mode & VWRITE))
+ return EINVAL;
+
+ error = namei(absolute_path, &dp);
+ free(absolute_path);
+
+ if (error)
+ return error;
+
+ if (dp->d_mount->m_flags & MNT_RDONLY) {
+ error = EROFS;
+ } else {
+ return EACCES;
+ if (times &&
+ (times[0].tv_nsec != UTIME_NOW || times[1].tv_nsec != UTIME_NOW) &&
+ (times[0].tv_nsec != UTIME_OMIT || times[1].tv_nsec != UTIME_OMIT) &&
+ (!(dp->d_vnode->v_mode & ~VAPPEND)))
+ return EPERM;
index cbed3fc..1ada346 100644
--- a/fs/vfs/vfs_vnode.c
+++ b/fs/vfs/vfs_vnode.c
@@ -400,8 +400,8 @@ vn_settimes(struct vnode *vp, struct timespec times[2])
vap->va_atime = times[0];
vap->va_mtime = times[1];
- vap->va_mask = AT_ATIME | AT_MTIME;
-
+ vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? 0 : AT_ATIME)
+ | ((times[1].tv_nsec == UTIME_OMIT) ? 0 : AT_MTIME);
vn_lock(vp);
error = VOP_SETATTR(vp, vap);
vn_unlock(vp);
--
1.9.0
Nadav Har'El
Jun 12, 2014, 8:46:19 AM6/12/14
to Jaspal Singh Dhillon, Osv Dev
Do we really have to use goto here? Would be much nicer to have something like:
if (error) {
race_vfs_utimensat_err(error);
errno = error;
return -1;
race_vfs_utimensat_err(error);
errno = error;
return -1;
} else {
trace_vfs_utimensat_ret();
trace_vfs_utimensat_ret();
return 0;
}
}
Nitpick: In C++, it's nicer to use a reference instead of pointer - const timespec &time instead of *time. The "->" become ".", and the caller can use times[0] instead of ×[0].
+}
+
+void init_timespec(struct timespec &_times, const struct timespec times)
+{
+
+ if (times.tv_nsec == UTIME_NOW) {
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else if (times.tv_nsec != UTIME_OMIT) {
+ _times.tv_sec = times.tv_sec;
+ _times.tv_nsec = times.tv_nsec;
Why the if (times.tv_nsec != UTIME_OMIT) at all, and not a straight "else"?
Even if it's UTIME_OMIT, you need to copy it, no?
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct dentry *dp;
+ char *absolute_path;
+ struct file *fp;
+ int length;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
Nitpick: in C++ (and also post-diluvian C) , you don't need to stuff all these declarations in the top of the function. You can put them closer to where you need them, preferably with the value.
+
+ if (times && (!is_timespec_valid(×[0]) || !is_timespec_valid(×[1])))
+ return EINVAL;
+
+ if (times) {
+ init_timespec(timespec_times[0],times[0]);
+ init_timespec(timespec_times[1],times[1]);
Please use a space after the commas.
+ } else {
+ clock_gettime(CLOCK_REALTIME, ×pec_times[0]);
+ clock_gettime(CLOCK_REALTIME, ×pec_times[1]);
Nitpick: (feel free to reject)
You're repeating here the clock_gettime() code, which I guess is ok, but doesn't look very pretty to me. One alternative is to make init_timespec() take a pointer, not a reference (yes, I realize how ironic this suggestion is considering my earlier comment...), and if this point is 0 also call clock_gettime. Then your call here becomes something like
init_timespec(timespec_times[0], times ? times+0 : nullptr);
init_timespec(timespec_times[0], times ? times+0 : nullptr);
+ }
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if (pathname && pathname[0] == '/') {
+ absolute_path = (char*)malloc(strlen(pathname));
+ strcpy(absolute_path, pathname);
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ length = strlen(main_task->t_cwd) + strlen(pathname) + 2;
+ absolute_path = (char*)malloc(length);
Please use unique_ptr to avoid leaking this memory. Already I see below one case you return() without freeing this.
+ snprintf(absolute_path, length, "%s/%s", main_task->t_cwd, pathname);
If main_task->t_cwd changed between the time we measured its length and copy it here, we're toast.
But to be honest, I don't understand how t_cwd locking happens at all...
(in any case, I am guessing there is a better way to do this function, without manipulating absolute paths... but maybe we can do that later...).
+ } else {
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR)){
+ return ENOTDIR;
+ }
+
+ length = strlen(fp->f_dentry->d_path) + strlen(pathname) + 2;
You need to put this in the if(pathname) below, otherwise strlen() will crash.
+ absolute_path = (char*)malloc(length);
+
+ if (pathname)
+ snprintf(absolute_path, length, "%s/%s", fp->f_dentry->d_path, pathname);
+ else
+ strcpy(absolute_path, pathname);
+ fdrop(fp);
+ }
+ /* FIXME: Add support to follow symbolic links */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ error = namei(absolute_path, &dp);
Do we have a namei() alternative taking a dir and a relative path, so you won't have to manipulate the absolute paths as above?
--
1.9.0
--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
--
Jaspal
Jun 12, 2014, 9:20:24 AM6/12/14
to Nadav Har'El, Osv Dev
I'll restructure the code but I got to say I was just trying to keep
the error handling similar to other functions such as readlink(),
ftruncate(), access()..
In fact, if you look at fs/vfs/main.cc, many functions use the goto and out_errno: is declared in 30 places!
In fact, if you look at fs/vfs/main.cc, many functions use the goto and out_errno: is declared in 30 places!
Lesson learned!
I'll keep in mind. I have been coding in C for a long time, the
practice kinda sticks.
+}
+
+void init_timespec(struct timespec &_times, const struct timespec times)
+{
+
+ if (times.tv_nsec == UTIME_NOW) {
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else if (times.tv_nsec != UTIME_OMIT) {
+ _times.tv_sec = times.tv_sec;
+ _times.tv_nsec = times.tv_nsec;
Why the if (times.tv_nsec != UTIME_OMIT) at all, and not a straight "else"?
Even if it's UTIME_OMIT, you need to copy it, no?
Nope. If it's UTIME_OMIT, the attribute is not set in vn_settimes()
and so no need to copy.
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct dentry *dp;
+ char *absolute_path;
+ struct file *fp;
+ int length;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
Nitpick: in C++ (and also post-diluvian C) , you don't need to stuff all these declarations in the top of the function. You can put them closer to where you need them, preferably with the value.
Cool.
+
+ if (times && (!is_timespec_valid(×[0]) || !is_timespec_valid(×[1])))
+ return EINVAL;
+
+ if (times) {
+ init_timespec(timespec_times[0],times[0]);
+ init_timespec(timespec_times[1],times[1]);
Please use a space after the commas.
Will do.
+ } else {
+ clock_gettime(CLOCK_REALTIME, ×pec_times[0]);
+ clock_gettime(CLOCK_REALTIME, ×pec_times[1]);
Nitpick: (feel free to reject)
You're repeating here the clock_gettime() code, which I guess is ok, but doesn't look very pretty to me. One alternative is to make init_timespec() take a pointer, not a reference (yes, I realize how ironic this suggestion is considering my earlier comment...), and if this point is 0 also call clock_gettime. Then your call here becomes something like
init_timespec(timespec_times[0], times ? times+0 : nullptr);
I was trying to cut the code but couldn't get it to compile. Thanks
for this suggestion. Will try it out.
+ }
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if (pathname && pathname[0] == '/') {
+ absolute_path = (char*)malloc(strlen(pathname));
+ strcpy(absolute_path, pathname);
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ length = strlen(main_task->t_cwd) + strlen(pathname) + 2;
+ absolute_path = (char*)malloc(length);
Please use unique_ptr to avoid leaking this memory. Already I see below one case you return() without freeing this.
Ok. If this is a .cc file, is it alright to use strings or is it
better to stick to C char[] when possible?
+ snprintf(absolute_path, length, "%s/%s", main_task->t_cwd, pathname);
If main_task->t_cwd changed between the time we measured its length and copy it here, we're toast.
One way to avoid it could be using a path[PATH_MAX] and strncpy()
but I didn't use to save memory.
But to be honest, I don't understand how t_cwd locking happens at all...
(in any case, I am guessing there is a better way to do this function, without manipulating absolute paths... but maybe we can do that later...).
+ } else {
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR)){
+ return ENOTDIR;
+ }
+
+ length = strlen(fp->f_dentry->d_path) + strlen(pathname) + 2;
You need to put this in the if(pathname) below, otherwise strlen() will crash.
My mistake.
+ absolute_path = (char*)malloc(length);
+
+ if (pathname)
+ snprintf(absolute_path, length, "%s/%s", fp->f_dentry->d_path, pathname);
+ else
+ strcpy(absolute_path, pathname);
+ fdrop(fp);
+ }
+ /* FIXME: Add support to follow symbolic links */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ error = namei(absolute_path, &dp);
Do we have a namei() alternative taking a dir and a relative path, so you won't have to manipulate the absolute paths as above?
None that I know of.
Raphael S. Carvalho
Jun 12, 2014, 9:38:14 AM6/12/14
to Nadav Har'El, Jaspal Singh Dhillon, Osv Dev
I personally don't think that gotos are that bad on certain conditions (although it's always better to avoid them), but in this case, the goto above could be, of course, avoided. I think he mostly followed our VFS layer code where gotos are often used when handling errors. Maybe, we should review the VFS code throughout, and replace code using goto with another one holding the same semantics, so that new OSv contributors working on VFS layer will not be influenced by the gotos there used.
--
Raphael S. Carvalho
Jaspal Singh Dhillon
Jun 12, 2014, 3:18:37 PM6/12/14
to osv...@googlegroups.com
Add utimensat() and futimens() which can update file timestamps with
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
Changes in v2:
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
Removed goto statements by restructing the code.
Use reference instead of pointer in is_timespec_valid()
Change prototype of init_timespec() to clean up code.
Use unique_ptr<string> ap, for absolute_path
fs/vfs/main.cc | 44 +++++++++++++++++++
fs/vfs/vfs.h | 3 ++
fs/vfs/vfs_syscalls.cc | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/vfs/vfs_vnode.c | 4 +-
4 files changed, 166 insertions(+), 2 deletions(-)
diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index 076e633..ed23aa8 100644
--- a/fs/vfs/main.cc
+++ b/fs/vfs/main.cc
@@ -1561,6 +1561,50 @@ int futimesat(int dirfd, const char *pathname, const struct timeval times[2])
return -1;
}
+TRACEPOINT(trace_vfs_utimensat, "\"%s\"", const char*);
+TRACEPOINT(trace_vfs_utimensat_ret, "");
+TRACEPOINT(trace_vfs_utimensat_err, "%d", int);
+
+extern "C"
+int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+
+ trace_vfs_utimensat(pathname);
+
+ error = sys_utimensat(dirfd, pathname, times, flags);
+ if (error) {
}
+TRACEPOINT(trace_vfs_utimensat, "\"%s\"", const char*);
+TRACEPOINT(trace_vfs_utimensat_ret, "");
+TRACEPOINT(trace_vfs_utimensat_err, "%d", int);
+
+extern "C"
+int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+
+ trace_vfs_utimensat(pathname);
+
+ error = sys_utimensat(dirfd, pathname, times, flags);
+ if (error) {
+ trace_vfs_utimensat_err(error);
+ errno = error;
+ return -1;
+ }
+
+ errno = error;
+ return -1;
+ }
+
+ trace_vfs_utimensat_ret();
+ return 0;
+}
+
+ return 0;
+}
+
+TRACEPOINT(trace_vfs_futimens, "%d", int);
+TRACEPOINT(trace_vfs_futimens_ret, "");
+TRACEPOINT(trace_vfs_futimens_err, "%d", int);
+
+extern "C"
+int futimens(int fd, const struct timespec times[2])
+{
+ int error;
+
+ trace_vfs_futimens(fd);
+
+ error = sys_futimens(fd, times);
+ if (error) {
+TRACEPOINT(trace_vfs_futimens_ret, "");
+TRACEPOINT(trace_vfs_futimens_err, "%d", int);
+
+extern "C"
+int futimens(int fd, const struct timespec times[2])
+{
+ int error;
+
+ trace_vfs_futimens(fd);
+
+ error = sys_futimens(fd, times);
+ if (error) {
+ trace_vfs_futimens_err(error);
+ errno = error;
+ return -1;
+ }
+
+ errno = error;
+ return -1;
+ }
+
+ trace_vfs_futimens_ret();
+ return 0;
+}
+
+ return 0;
+}
+
extern "C"
int utimes(const char *pathname, const struct timeval times[2])
{
diff --git a/fs/vfs/vfs.h b/fs/vfs/vfs.h
index 8023807..d31e2fa 100644
--- a/fs/vfs/vfs.h
+++ b/fs/vfs/vfs.h
@@ -117,6 +117,9 @@ int sys_statfs(char *path, struct statfs *buf);
int sys_truncate(char *path, off_t length);
ssize_t sys_readlink(char *path, char *buf, size_t bufsize);
int sys_utimes(char *path, const struct timeval times[2]);
+int sys_utimensat(int dirfd, const char *pathname,
+ const struct timespec times[2], int flags);
+int sys_futimens(int fd, const struct timespec times[2]);
int sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len);
int sys_mount(char *dev, char *dir, char *fsname, int flags, void *data);
diff --git a/fs/vfs/vfs_syscalls.cc b/fs/vfs/vfs_syscalls.cc
index 9c76604..9ac808c 100644
int utimes(const char *pathname, const struct timeval times[2])
{
diff --git a/fs/vfs/vfs.h b/fs/vfs/vfs.h
index 8023807..d31e2fa 100644
--- a/fs/vfs/vfs.h
+++ b/fs/vfs/vfs.h
@@ -117,6 +117,9 @@ int sys_statfs(char *path, struct statfs *buf);
int sys_truncate(char *path, off_t length);
ssize_t sys_readlink(char *path, char *buf, size_t bufsize);
int sys_utimes(char *path, const struct timeval times[2]);
+int sys_utimensat(int dirfd, const char *pathname,
+ const struct timespec times[2], int flags);
+int sys_futimens(int fd, const struct timespec times[2]);
int sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len);
int sys_mount(char *dev, char *dir, char *fsname, int flags, void *data);
diff --git a/fs/vfs/vfs_syscalls.cc b/fs/vfs/vfs_syscalls.cc
--- a/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs_syscalls.cc
@@ -1036,6 +1036,123 @@ sys_utimes(char *path, const struct timeval times[2])
return error;
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec &time)
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+{
+ return (time.tv_sec >= 0) &&
+ ((time.tv_nsec >= 0 && time.tv_nsec <= 999999999) ||
+ time.tv_nsec == UTIME_NOW ||
+ time.tv_nsec == UTIME_OMIT);
+}
+
+void init_timespec(struct timespec &_times, const struct timespec *times)
+{
+ if ((times != nullptr && times->tv_nsec == UTIME_NOW) || times == nullptr) {
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else if (times->tv_nsec != UTIME_OMIT) {
+ _times.tv_sec = times->tv_sec;
+ _times.tv_nsec = times->tv_nsec;
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ std::unique_ptr<std::string> ap (new std::string());
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+ extern struct task *main_task;
+ struct dentry *dp;
+
+ if (times && (!is_timespec_valid(times[0]) || !is_timespec_valid(times[1])))
+
+ return EINVAL;
+
+ init_timespec(timespec_times[0], times ? times + 0 : nullptr);
+ init_timespec(timespec_times[1], times ? times + 1 : nullptr);
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if (pathname && pathname[0] == '/') {
+ *ap = std::string(pathname);
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if (pathname && pathname[0] == '/') {
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ *ap = std::string(main_task->t_cwd) + std::string("/") + std::string(pathname);
+ if (!pathname)
+ return EFAULT;
+ } else {
+ struct file *fp;
+
+
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR))
+ return ENOTDIR;
+
+ if (pathname)
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR))
+ return ENOTDIR;
+
+ *ap = std::string(fp->f_dentry->d_path) + std::string("/") + std::string(pathname);
+ else
+ *ap = std::string(fp->f_dentry->d_path);
+
+ fdrop(fp);
+ }
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ auto absolute_path = new char[ap->length()+1];
+ return EINVAL;
+
+ strcpy(absolute_path, ap->c_str());
+ error = namei(absolute_path, &dp);
+ delete[] absolute_path;
Jaspal Singh Dhillon
Jun 12, 2014, 3:18:38 PM6/12/14
to osv...@googlegroups.com
Add tests for utimensat() and add it to build.mk
build.mk | 1 +
tests/tst-utimensat.cc | 152 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+)
create mode 100644 tests/tst-utimensat.cc
diff --git a/build.mk b/build.mk
index db44735..d063310 100644
--- a/build.mk
+++ b/build.mk
@@ -303,6 +303,7 @@ tests += tests/tst-truncate.so
tests += $(boost-tests)
tests += tests/misc-panic.so
tests += tests/tst-utimes.so
+tests += tests/tst-utimensat.so
tests += tests/tst-futimesat.so
tests += tests/misc-tcp.so
tests += tests/tst-strerror_r.so
diff --git a/tests/tst-utimensat.cc b/tests/tst-utimensat.cc
new file mode 100644
index 0000000..5294f62
--- /dev/null
+++ b/tests/tst-utimensat.cc
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2014 Jaspal Singh Dhillon
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <chrono>
+
+using namespace std::chrono;
+
+static int tests = 0, fails = 0;
+
+static void report(bool ok, const char* msg)
+{
+ ++tests;
+ fails += !ok;
+ printf("%s: %s\n", (ok ? "PASS" : "FAIL"), msg);
+}
+
+/* Convert time_t to system_clock::timepoint */
+system_clock::time_point to_timeptr(const time_t &time)
+{
+ return system_clock::from_time_t(time);
+}
+
+/* Convert timespec to system_clock::timepoint */
+system_clock::time_point to_timeptr(const timespec &time)
+{
+ return system_clock::from_time_t(time.tv_sec) + nanoseconds(time.tv_nsec);
+}
+
+/*
+ * Convert everything to system_clock::timepoint before the comparison.
+ */
+static bool compare_time(const timespec &time1, const timespec &time2)
+{
+ return to_timeptr(time1) == to_timeptr(time2);
+}
+
+static void init_timespec(timespec &time, time_t tv_sec, long tv_nsec)
+{
+ time.tv_sec = tv_sec;
+ time.tv_nsec = tv_nsec;
+}
+
+int main(int argc, char *argv[])
+{
+ const char *tmp_folder = "/tmp";
+ const char *tmp_folder_foo = "/tmp/tst_folder_foo";
+ const char *tmp_file_bar = "/tmp/tst_folder_foo/bar";
+ const char *rel_path_bar_to_foo = "bar";
+ const char *rel_path_bar_to_tmp = "tst_folder_foo/bar";
+ struct timespec times[2];
+ int ret;
+ int fd;
+ int dirfd;
+ struct stat st;
+
+ /* Create the temporary folder and file that are used in testing */
+ report(mkdir(tmp_folder_foo, 0755) == 0, "create folder foo");
+
+ fd = open(tmp_file_bar, O_CREAT|O_TRUNC|O_RDWR, 0666);
+ report(fd > 0, "create file bar");
+ write(fd, "test_bar" , 8);
+ report(close(fd) == 0, "close file bar");
+
+ /* Get fd to the foo directory */
+ dirfd = open(tmp_folder_foo, O_RDONLY);
+ report(dirfd > 0, "open folder foo");
+
+ /* Initialize atime and mtime structures */
+ init_timespec(times[0], 1234, 0); /* atime */
+ init_timespec(times[1], 0, 1234); /* mtime */
+
+ /* Test if utimensat ignores dirfd when path is absolute */
+ ret = utimensat(100, tmp_file_bar, times, 0);
+ report(ret == 0, "utimensat worked successfully using absolute path");
+
+ /* Stat the file to check if utimensat made our time changes
+ * to the inode */
+ report(stat(tmp_file_bar, &st) == 0, "stat the file");
+
+ /* Check atime changes */
+ report(compare_time(st.st_atim, times[0]), "check atime changes");
+
+ /* Check mtime changes */
+ report(compare_time(st.st_mtim, times[1]), "check mtime changes");
+
+ /* Change directory to /tmp and check if utimensat can work with AT_FDCWD */
+ report(chdir(tmp_folder) == 0, "change directory to tmp");
+ ret = utimensat(AT_FDCWD, rel_path_bar_to_tmp, times, 0);
+ report(ret == 0, "utimensat worked successfully with AT_FDCWD");
+
+ /* Use dirfd and relative path of bar to check utimensat */
+ ret = utimensat(dirfd, rel_path_bar_to_foo, times, 0);
+ report(ret == 0, "utimensat works with dirfd and relative path");
+
+ /* Force utimensat to fail using invalid dirfd */
+ ret = utimensat(100, rel_path_bar_to_foo, times, 0);
+ report(ret == -1 && errno == EBADF, "utimensat fails with invalid dirfd");
+
+ /* Check utimensat when times == NULL */
+ ret = utimensat(dirfd, rel_path_bar_to_foo, NULL, 0);
+ report(ret == 0, "utimensat works with times == NULL");
+
+ /* Force utimensat to fail when dirfd was AT_FDCWD and pathname is NULL */
+ ret = utimensat(AT_FDCWD, NULL, times, 0);
+ report(ret == -1 && errno == EFAULT, "utimensat fails when dirfd is AT_FDCWD and pathname is NULL");
+
+ /* Force utimensat to fail with invalid flags */
+ ret = utimensat(dirfd, rel_path_bar_to_tmp, times, 23);
+ report(ret == -1 && errno == EINVAL, "utimensat fails with invalid flags");
+
+ /* Force utimensat to fail with invalid values in times */
+ init_timespec(times[0], -1, 100); /* change atime */
+ ret = utimensat(dirfd, rel_path_bar_to_tmp, times, 0);
+ report(ret == -1 && errno == EINVAL, "utimensat fails with invalid value in times");
+
+ init_timespec(times[0], 1234, 100);
+
+ /* Force utimensat to fail with non-existent path */
+ ret = utimensat(dirfd, "this/does/not/exist", times, 0);
+ report(ret == -1 && errno == ENOENT, "utimensat fails with non-existent path");
+
+ /* Force utimensat to fail with empty pathname */
+ ret = utimensat(dirfd, "", times, 0);
+ report(ret == -1 && errno == ENOENT, "utimensat fails with empty pathname");
+
+ /* Force utimensat with invalid dirfd and relative pathname */
+ report(close(dirfd) == 0, "closing dirfd");
+ dirfd = open(tmp_file_bar, O_RDONLY);
+ report(dirfd > 0, "open file bar with dirfd");
+ ret = utimensat(dirfd, rel_path_bar_to_foo, times, 0);
+ report(ret == -1 && errno == ENOTDIR, "utimensat fails when dirfd points to file and relative pathname is given");
+
+ /* Clean up temporary files */
+ report(unlink(tmp_file_bar) == 0, "remove the file bar");
+ report(rmdir(tmp_folder_foo) == 0, "remove the folder foo");
+
+ /* Report results. */
+ printf("SUMMARY: %d tests, %d failures\n", tests, fails);
+ return 0;
+}
--
1.9.0
build.mk | 1 +
tests/tst-utimensat.cc | 152 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+)
create mode 100644 tests/tst-utimensat.cc
diff --git a/build.mk b/build.mk
index db44735..d063310 100644
--- a/build.mk
+++ b/build.mk
@@ -303,6 +303,7 @@ tests += tests/tst-truncate.so
tests += $(boost-tests)
tests += tests/misc-panic.so
tests += tests/tst-utimes.so
+tests += tests/tst-utimensat.so
tests += tests/tst-futimesat.so
tests += tests/misc-tcp.so
tests += tests/tst-strerror_r.so
diff --git a/tests/tst-utimensat.cc b/tests/tst-utimensat.cc
new file mode 100644
index 0000000..5294f62
--- /dev/null
+++ b/tests/tst-utimensat.cc
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2014 Jaspal Singh Dhillon
+ *
+ * This work is open source software, licensed under the terms of the
+ * BSD license as described in the LICENSE file in the top-level directory.
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <chrono>
+
+using namespace std::chrono;
+
+static int tests = 0, fails = 0;
+
+static void report(bool ok, const char* msg)
+{
+ ++tests;
+ fails += !ok;
+ printf("%s: %s\n", (ok ? "PASS" : "FAIL"), msg);
+}
+
+/* Convert time_t to system_clock::timepoint */
+system_clock::time_point to_timeptr(const time_t &time)
+{
+ return system_clock::from_time_t(time);
+}
+
+/* Convert timespec to system_clock::timepoint */
+system_clock::time_point to_timeptr(const timespec &time)
+{
+ return system_clock::from_time_t(time.tv_sec) + nanoseconds(time.tv_nsec);
+}
+
+/*
+ * Convert everything to system_clock::timepoint before the comparison.
+ */
+static bool compare_time(const timespec &time1, const timespec &time2)
+{
+ return to_timeptr(time1) == to_timeptr(time2);
+}
+
+static void init_timespec(timespec &time, time_t tv_sec, long tv_nsec)
+{
+ time.tv_sec = tv_sec;
+ time.tv_nsec = tv_nsec;
+}
+
+int main(int argc, char *argv[])
+{
+ const char *tmp_folder = "/tmp";
+ const char *tmp_folder_foo = "/tmp/tst_folder_foo";
+ const char *tmp_file_bar = "/tmp/tst_folder_foo/bar";
+ const char *rel_path_bar_to_foo = "bar";
+ const char *rel_path_bar_to_tmp = "tst_folder_foo/bar";
+ struct timespec times[2];
+ int ret;
+ int fd;
+ int dirfd;
+ struct stat st;
+
+ /* Create the temporary folder and file that are used in testing */
+ report(mkdir(tmp_folder_foo, 0755) == 0, "create folder foo");
+
+ fd = open(tmp_file_bar, O_CREAT|O_TRUNC|O_RDWR, 0666);
+ report(fd > 0, "create file bar");
+ write(fd, "test_bar" , 8);
+ report(close(fd) == 0, "close file bar");
+
+ /* Get fd to the foo directory */
+ dirfd = open(tmp_folder_foo, O_RDONLY);
+ report(dirfd > 0, "open folder foo");
+
+ /* Initialize atime and mtime structures */
+ init_timespec(times[0], 1234, 0); /* atime */
+ init_timespec(times[1], 0, 1234); /* mtime */
+
+ /* Test if utimensat ignores dirfd when path is absolute */
+ ret = utimensat(100, tmp_file_bar, times, 0);
+ report(ret == 0, "utimensat worked successfully using absolute path");
+
+ /* Stat the file to check if utimensat made our time changes
+ * to the inode */
+ report(stat(tmp_file_bar, &st) == 0, "stat the file");
+
+ /* Check atime changes */
+ report(compare_time(st.st_atim, times[0]), "check atime changes");
+
+ /* Check mtime changes */
+ report(compare_time(st.st_mtim, times[1]), "check mtime changes");
+
+ /* Change directory to /tmp and check if utimensat can work with AT_FDCWD */
+ report(chdir(tmp_folder) == 0, "change directory to tmp");
+ ret = utimensat(AT_FDCWD, rel_path_bar_to_tmp, times, 0);
+ report(ret == 0, "utimensat worked successfully with AT_FDCWD");
+
+ /* Use dirfd and relative path of bar to check utimensat */
+ ret = utimensat(dirfd, rel_path_bar_to_foo, times, 0);
+ report(ret == 0, "utimensat works with dirfd and relative path");
+
+ /* Force utimensat to fail using invalid dirfd */
+ ret = utimensat(100, rel_path_bar_to_foo, times, 0);
+ report(ret == -1 && errno == EBADF, "utimensat fails with invalid dirfd");
+
+ /* Check utimensat when times == NULL */
+ ret = utimensat(dirfd, rel_path_bar_to_foo, NULL, 0);
+ report(ret == 0, "utimensat works with times == NULL");
+
+ /* Force utimensat to fail when dirfd was AT_FDCWD and pathname is NULL */
+ ret = utimensat(AT_FDCWD, NULL, times, 0);
+ report(ret == -1 && errno == EFAULT, "utimensat fails when dirfd is AT_FDCWD and pathname is NULL");
+
+ /* Force utimensat to fail with invalid flags */
+ ret = utimensat(dirfd, rel_path_bar_to_tmp, times, 23);
+ report(ret == -1 && errno == EINVAL, "utimensat fails with invalid flags");
+
+ /* Force utimensat to fail with invalid values in times */
+ init_timespec(times[0], -1, 100); /* change atime */
+ ret = utimensat(dirfd, rel_path_bar_to_tmp, times, 0);
+ report(ret == -1 && errno == EINVAL, "utimensat fails with invalid value in times");
+
+ init_timespec(times[0], 1234, 100);
+
+ /* Force utimensat to fail with non-existent path */
+ ret = utimensat(dirfd, "this/does/not/exist", times, 0);
+ report(ret == -1 && errno == ENOENT, "utimensat fails with non-existent path");
+
+ /* Force utimensat to fail with empty pathname */
+ ret = utimensat(dirfd, "", times, 0);
+ report(ret == -1 && errno == ENOENT, "utimensat fails with empty pathname");
+
+ /* Force utimensat with invalid dirfd and relative pathname */
+ report(close(dirfd) == 0, "closing dirfd");
+ dirfd = open(tmp_file_bar, O_RDONLY);
+ report(dirfd > 0, "open file bar with dirfd");
+ ret = utimensat(dirfd, rel_path_bar_to_foo, times, 0);
+ report(ret == -1 && errno == ENOTDIR, "utimensat fails when dirfd points to file and relative pathname is given");
+
+ /* Clean up temporary files */
+ report(unlink(tmp_file_bar) == 0, "remove the file bar");
+ report(rmdir(tmp_folder_foo) == 0, "remove the folder foo");
+
+ /* Report results. */
+ printf("SUMMARY: %d tests, %d failures\n", tests, fails);
+ return 0;
+}
--
1.9.0
Nadav Har'El
Jun 12, 2014, 3:55:00 PM6/12/14
to Jaspal Singh Dhillon, Osv Dev
Hi, thanks. Some serious issues, plus some minor comments, below.
This is where C++'s ability to not declare variables on the top of the function shines. You can do here
int error = sys_utimensat(dirfd, pathname, times, flags);
int error = sys_utimensat(dirfd, pathname, times, flags);
and not split this line into two as above.
You can even do
auto error = sys_utimensat(dirfd, pathname, times, flags);
auto error = sys_utimensat(dirfd, pathname, times, flags);
if you want.
+ if (error) {
+ trace_vfs_utimensat_err(error);
+ errno = error;
+ return -1;
+ }
+
+ trace_vfs_utimensat_ret();
+ return 0;
Nicer, clear, and shorter, without that ugly goto :-)
The above condition is equivalent to just
if (times == nullptr || times->tv_nsec == UTIME_NOW)
unless I'm mistaken.
unless I'm mistaken.
{
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else if (times->tv_nsec != UTIME_OMIT)
+ _times.tv_sec = times->tv_sec;
+ _times.tv_nsec = times->tv_nsec;
I still don't understand why you need to check nsec != UTIME_OMIT here. What harm can happen if you copy the timespec even in the UTIME_OMIT case? See below more about this issue.
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ std::unique_ptr<std::string> ap (new std::string());
Unless I'm misunderstanding something, you don't need all this complexity - a simple
std::string ap;
will do, and work just as well, and all the code below will be less ugly (e.g.,
instead of *ap = std::string(pathname), you can just do ap = pathname;
For
*ap = std::string(main_task->t_cwd) + std::string("/") + std::string(pathname);
you may still need to do
ap = std::string(main_task->t_cwd) + "/" + pathname;
(I think the first std::string is needed because otherwise, C++ will not even know std::strings are involved.
BTW, all of this string processing is likely to be less efficient than the explicit strlen & malloc code you had earlier, but I doubt this matters for this function, and I agree this code is nicer)
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+ struct dentry *dp;
+
+ if (times && (!is_timespec_valid(times[0]) || !is_timespec_valid(times[1])))
+ return EINVAL;
+
+ init_timespec(timespec_times[0], times ? times + 0 : nullptr);
+ init_timespec(timespec_times[1], times ? times + 1 : nullptr);
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
Nitpick: It makes sense (though obviously not important) to do this sort of parameter checking before the init_timespec calls.
+
+ if (pathname && pathname[0] == '/') {
+ *ap = std::string(pathname);
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ *ap = std::string(main_task->t_cwd) + std::string("/") + std::string(pathname);
+ } else {
+ struct file *fp;
+
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
Oh no, you are returning here (and other places below) without fdrop(fp)...
Either remember to fdrop(fp) in all the missing places, or better yet, start using the better, C++-RAII-style, "fileref", which will mean you'll never have to worry about fdrop again.
++
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR))
+ return ENOTDIR;
+
+ if (pathname)
+ *ap = std::string(fp->f_dentry->d_path) + std::string("/") + std::string(pathname);
+ else
+ *ap = std::string(fp->f_dentry->d_path);
+ fdrop(fp);
+ }
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ auto absolute_path = new char[ap->length()+1];
+ strcpy(absolute_path, ap->c_str());
Why not just use ap->c_str directly - why do you need to copy it into absolute_path? ap->c_str() is already a nice, clean, C string, you can use....
Again, I don't understand why you are copying d_path instead of just using it directly? What am I missing?
+ error = sys_utimensat(AT_FDCWD, pathname, times, 0);
+ free(pathname);
+ return error;
+}
+
int
sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len)
{
diff --git a/fs/vfs/vfs_vnode.c b/fs/vfs/vfs_vnode.c
index cbed3fc..98445f4 100644
--- a/fs/vfs/vfs_vnode.c
+++ b/fs/vfs/vfs_vnode.c
@@ -400,8 +400,8 @@ vn_settimes(struct vnode *vp, struct timespec times[2])
vap->va_atime = times[0];
vap->va_mtime = times[1];
- vap->va_mask = AT_ATIME | AT_MTIME;
-
+ vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? 0 : AT_ATIME)
+ | ((times[1].tv_nsec == UTIME_OMIT) ? 0 : AT_MTIME);
vn_lock(vp);
error = VOP_SETATTR(vp, vap);
vn_unlock(vp);
--
1.9.0
--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Jaspal
Jun 12, 2014, 4:32:03 PM6/12/14
to Nadav Har'El, Osv Dev
Right, will simplify it. I must have missed it :-/
{
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else if (times->tv_nsec != UTIME_OMIT)
+ _times.tv_sec = times->tv_sec;
+ _times.tv_nsec = times->tv_nsec;
I still don't understand why you need to check nsec != UTIME_OMIT here. What harm can happen if you copy the timespec even in the UTIME_OMIT case? See below more about this issue.
No harm. I'll put the simpler 'else'.
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ std::unique_ptr<std::string> ap (new std::string());
Unless I'm misunderstanding something, you don't need all this complexity - a simple
std::string ap;
will do, and work just as well, and all the code below will be less ugly (e.g.,
instead of *ap = std::string(pathname), you can just do ap = pathname;
For
*ap = std::string(main_task->t_cwd) + std::string("/") + std::string(pathname);
you may still need to do
ap = std::string(main_task->t_cwd) + "/" + pathname;
(I think the first std::string is needed because otherwise, C++ will not even know std::strings are involved.
Ok.
BTW, all of this string processing is likely to be less efficient than the explicit strlen & malloc code you had earlier, but I doubt this matters for this function, and I agree this code is nicer)
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+ struct dentry *dp;
+
+ if (times && (!is_timespec_valid(times[0]) || !is_timespec_valid(times[1])))
+ return EINVAL;
+
+ init_timespec(timespec_times[0], times ? times + 0 : nullptr);
+ init_timespec(timespec_times[1], times ? times + 1 : nullptr);
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
Nitpick: It makes sense (though obviously not important) to do this sort of parameter checking before the init_timespec calls.
Will move it up in the function.
+
+ if (pathname && pathname[0] == '/') {
+ *ap = std::string(pathname);
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ *ap = std::string(main_task->t_cwd) + std::string("/") + std::string(pathname);
+ } else {
+ struct file *fp;
+
+ error = fget(dirfd, &fp);
+ if (error)
+ return error;
+
+ if(!fp->f_dentry)
+ return EBADF;
Oh no, you are returning here (and other places below) without fdrop(fp)...
Either remember to fdrop(fp) in all the missing places, or better yet, start using the better, C++-RAII-style, "fileref", which will mean you'll never have to worry about fdrop again.
I'll fix it.
++
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR))
+ return ENOTDIR;
+
+ if (pathname)
+ *ap = std::string(fp->f_dentry->d_path) + std::string("/") + std::string(pathname);
+ else
+ *ap = std::string(fp->f_dentry->d_path);
+ fdrop(fp);
+ }
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ auto absolute_path = new char[ap->length()+1];
+ strcpy(absolute_path, ap->c_str());
Why not just use ap->c_str directly - why do you need to copy it into absolute_path? ap->c_str() is already a nice, clean, C string, you can use....
I did try to use it directly. Turns out, namei expects char*, not
const char* which c_str() provides. It had to be either this change
or change of function header ( add const ) to namei() ,
vfs_findroot() and so on.
Same as above.
Nadav Har'El
Jun 12, 2014, 5:25:28 PM6/12/14
to Jaspal, Osv Dev
On Thu, Jun 12, 2014 at 11:32 PM, Jaspal <jaspal...@gmail.com> wrote:
+ auto absolute_path = new char[ap->length()+1];I did try to use it directly. Turns out, namei expects char*, not const char* which c_str() provides. It had to be either this change or change of function header ( add const ) to namei() , vfs_findroot() and so on.+ strcpy(absolute_path, ap->c_str());
Why not just use ap->c_str directly - why do you need to copy it into absolute_path? ap->c_str() is already a nice, clean, C string, you can use....
Hi,
I think the *right* way would be to, indeed, fix namei() and the rest of the functions downstream to take const char *. Of course, this shouldn't be done as part of this patch.
Who's keeping tabs, but C has had the "const" keyword for 25 years now (!), and if I remember correctly, C++ has had it all along (30 years ago). For a few more years there might have been some excuse to not use "const" correctly, but really, a long long time has passed since. The age of const-correctness (http://en.wikipedia.org/wiki/Const_correctness) has long ago arrived. We shouldn't have code in our shiny new OSv which looks like it has been written in the 80's ;-)
Who's keeping tabs, but C has had the "const" keyword for 25 years now (!), and if I remember correctly, C++ has had it all along (30 years ago). For a few more years there might have been some excuse to not use "const" correctly, but really, a long long time has passed since. The age of const-correctness (http://en.wikipedia.org/wiki/Const_correctness) has long ago arrived. We shouldn't have code in our shiny new OSv which looks like it has been written in the 80's ;-)
Instead of needlessly copying the array, another option you have (before namei() is fixed) is to use const_cast (http://en.cppreference.com/w/cpp/language/const_cast), which is a magic trick to pretend a const pointer isn't const. You should obviously do this only after verifying namei() doesn't write to the array, and add a big fat comment explaining this abomination. But I think that even if you keep the copy you have now, you need a big fat comment explaining why it's there.
Another option is to send a patch to fix the const of namei() and friends, and only then resubmit this patch without ugly workarounds. I think it should be relatively easy, just add const to half a dozen functions? The compiler will tell you exactly where it's missing :-) If this is not too difficult, I think this is the best option.
Thanks!
Nadav.
Jaspal
Jun 12, 2014, 5:59:30 PM6/12/14
to Nadav Har'El, Osv Dev
Done. Just sent the patch!
I agree this is the right option.
I agree this is the right option.
Thanks!
Nadav.
Jaspal Singh Dhillon
Jun 12, 2014, 6:25:24 PM6/12/14
to osv...@googlegroups.com
Add utimensat() and futimens() which can update file timestamps with
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
Please apply this patch after applying the patch for namei() and friends.
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
Changes in v3:
Use string instead of unique_ptr and simplify code.
Use fileref to avoid missing a call to fdrop() on return
Don't use an extra array to hold the absolute path. Instead use c_str()
fs/vfs/main.cc | 40 +++++++++++++++++
fs/vfs/vfs.h | 3 ++
fs/vfs/vfs_syscalls.cc | 114 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/vfs/vfs_vnode.c | 4 +-
4 files changed, 159 insertions(+), 2 deletions(-)
diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
index 076e633..59926f5 100644
--- a/fs/vfs/main.cc
+++ b/fs/vfs/main.cc
@@ -1561,6 +1561,46 @@ int futimesat(int dirfd, const char *pathname, const struct timeval times[2])
return -1;
}
+TRACEPOINT(trace_vfs_utimensat, "\"%s\"", const char*);
+TRACEPOINT(trace_vfs_utimensat_ret, "");
+TRACEPOINT(trace_vfs_utimensat_err, "%d", int);
+
+extern "C"
+int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ trace_vfs_utimensat(pathname);
}
+TRACEPOINT(trace_vfs_utimensat, "\"%s\"", const char*);
+TRACEPOINT(trace_vfs_utimensat_ret, "");
+TRACEPOINT(trace_vfs_utimensat_err, "%d", int);
+
+extern "C"
+int utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+
+ auto error = sys_utimensat(dirfd, pathname, times, flags);
+ if (error) {
+ trace_vfs_utimensat_err(error);
+ errno = error;
+ return -1;
+ }
+
+ trace_vfs_utimensat_ret();
+ return 0;
+ trace_vfs_utimensat_err(error);
+ errno = error;
+ return -1;
+ }
+
+ trace_vfs_utimensat_ret();
+ return 0;
+}
+
+TRACEPOINT(trace_vfs_futimens, "%d", int);
+TRACEPOINT(trace_vfs_futimens_ret, "");
+TRACEPOINT(trace_vfs_futimens_err, "%d", int);
+
+extern "C"
+int futimens(int fd, const struct timespec times[2])
+{
+ trace_vfs_futimens(fd);
+
+TRACEPOINT(trace_vfs_futimens, "%d", int);
+TRACEPOINT(trace_vfs_futimens_ret, "");
+TRACEPOINT(trace_vfs_futimens_err, "%d", int);
+
+extern "C"
+int futimens(int fd, const struct timespec times[2])
+{
+
+ auto error = sys_futimens(fd, times);
--- a/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs_syscalls.cc
@@ -1036,6 +1036,120 @@ sys_utimes(char *path, const struct timeval times[2])
return error;
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec &time)
+{
+ return (time.tv_sec >= 0) &&
+ ((time.tv_nsec >= 0 && time.tv_nsec <= 999999999) ||
+ time.tv_nsec == UTIME_NOW ||
+ time.tv_nsec == UTIME_OMIT);
+}
+
+void init_timespec(struct timespec &_times, const struct timespec *times)
+{
+ if (times == nullptr || times->tv_nsec == UTIME_NOW) {
}
+/*
+ * Check the validity of members of a struct timespec
+ */
+static bool is_timespec_valid(const struct timespec &time)
+{
+ return (time.tv_sec >= 0) &&
+ ((time.tv_nsec >= 0 && time.tv_nsec <= 999999999) ||
+ time.tv_nsec == UTIME_NOW ||
+ time.tv_nsec == UTIME_OMIT);
+}
+
+void init_timespec(struct timespec &_times, const struct timespec *times)
+{
+ clock_gettime(CLOCK_REALTIME, &_times);
+ } else {
+ _times.tv_sec = times->tv_sec;
+ _times.tv_nsec = times->tv_nsec;
+ _times.tv_nsec = times->tv_nsec;
+ }
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ std::string ap;
+ return;
+}
+
+int
+sys_utimensat(int dirfd, const char *pathname, const struct timespec times[2], int flags)
+{
+ int error;
+ struct timespec timespec_times[2];
+ extern struct task *main_task;
+ struct dentry *dp;
+
+ extern struct task *main_task;
+ struct dentry *dp;
+
+ /* utimensat should return ENOENT when pathname is empty */
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+
+ if(pathname && pathname[0] == 0)
+ return ENOENT;
+ if (flags && !(flags & AT_SYMLINK_NOFOLLOW))
+ return EINVAL;
+
+ return EINVAL;
+
+ if (times && (!is_timespec_valid(times[0]) || !is_timespec_valid(times[1])))
+ return EINVAL;
+
+ init_timespec(timespec_times[0], times ? times + 0 : nullptr);
+ init_timespec(timespec_times[1], times ? times + 1 : nullptr);
+
+ return EINVAL;
+
+ init_timespec(timespec_times[0], times ? times + 0 : nullptr);
+ init_timespec(timespec_times[1], times ? times + 1 : nullptr);
+
+ if (pathname && pathname[0] == '/') {
+ ap = pathname;
+ } else if (dirfd == AT_FDCWD) {
+ if (!pathname)
+ return EFAULT;
+ ap = std::string(main_task->t_cwd) + "/" + pathname;
+ if (!pathname)
+ return EFAULT;
+ } else {
+ struct file *fp;
+ fileref f(fileref_from_fd(dirfd));
+ struct file *fp;
+
+ if (!f)
+ return EBADF;
+
+ fp = f.get();
+
+ if(!fp->f_dentry)
+ return EBADF;
+ if(!fp->f_dentry)
+ return EBADF;
+
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR))
+ return ENOTDIR;
+
+ if (pathname)
+ ap = std::string(fp->f_dentry->d_path) + "/" + pathname;
+ if (!(fp->f_dentry->d_vnode->v_type & VDIR))
+ return ENOTDIR;
+
+ if (pathname)
+ else
+ ap = fp->f_dentry->d_path;
+ }
+
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+
+ error = namei(ap.c_str(), &dp);
+ /* FIXME: Add support for AT_SYMLINK_NOFOLLOW */
+
+ if (!f)
+ return EBADF;
+
+ fp = f.get();
+
+ if (!fp->f_dentry)
+ return EBADF;
+
+ std::string pathname = fp->f_dentry->d_path;
+ if (!fp->f_dentry)
+ return EBADF;
+
+ auto error = sys_utimensat(AT_FDCWD, pathname.c_str(), times, 0);
Jaspal Singh Dhillon
Jun 12, 2014, 6:25:26 PM6/12/14
to osv...@googlegroups.com
build.mk | 1 +
tests/tst-utimensat.cc | 152 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+)
create mode 100644 tests/tst-utimensat.cc
diff --git a/build.mk b/build.mk
index ba36e90..c1a9bfc 100644
tests/tst-utimensat.cc | 152 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 153 insertions(+)
create mode 100644 tests/tst-utimensat.cc
diff --git a/build.mk b/build.mk
--- a/build.mk
+++ b/build.mk
@@ -303,6 +303,7 @@ tests += tests/tst-truncate.so
tests += $(boost-tests)
tests += tests/misc-panic.so
tests += tests/tst-utimes.so
+tests += tests/tst-utimensat.so
tests += tests/tst-futimesat.so
tests += tests/misc-tcp.so
tests += tests/tst-strerror_r.so
diff --git a/tests/tst-utimensat.cc b/tests/tst-utimensat.cc
new file mode 100644
index 0000000..ee4307f
+++ b/build.mk
@@ -303,6 +303,7 @@ tests += tests/tst-truncate.so
tests += $(boost-tests)
tests += tests/misc-panic.so
tests += tests/tst-utimes.so
+tests += tests/tst-utimensat.so
tests += tests/tst-futimesat.so
tests += tests/misc-tcp.so
tests += tests/tst-strerror_r.so
diff --git a/tests/tst-utimensat.cc b/tests/tst-utimensat.cc
new file mode 100644
+{
Nadav Har'El
Jun 15, 2014, 6:17:00 AM6/15/14
to Jaspal Singh Dhillon, Osv Dev
This version looks good to me. Thanks.
--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Raphael S. Carvalho
Jun 15, 2014, 7:31:27 AM6/15/14
to Jaspal Singh Dhillon, osv-dev
Just an issue with the coding style: Note that brackets are used even on branches with a single statement, i.e.
the conditional statement above needs bracket.
++ std::string pathname = fp->f_dentry->d_path;
+ auto error = sys_utimensat(AT_FDCWD, pathname.c_str(), times, 0);
+ return error;
+}
+
int
sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len)
{
diff --git a/fs/vfs/vfs_vnode.c b/fs/vfs/vfs_vnode.c
index cbed3fc..98445f4 100644
--- a/fs/vfs/vfs_vnode.c
+++ b/fs/vfs/vfs_vnode.c
@@ -400,8 +400,8 @@ vn_settimes(struct vnode *vp, struct timespec times[2])
vap->va_atime = times[0];
vap->va_mtime = times[1];
- vap->va_mask = AT_ATIME | AT_MTIME;
-
+ vap->va_mask = ((times[0].tv_nsec == UTIME_OMIT) ? 0 : AT_ATIME)
+ | ((times[1].tv_nsec == UTIME_OMIT) ? 0 : AT_MTIME);
vn_lock(vp);
error = VOP_SETATTR(vp, vap);
vn_unlock(vp);
--
1.9.0
--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
--
Raphael S. Carvalho
Jaspal
Jun 15, 2014, 7:35:40 AM6/15/14
to Raphael S. Carvalho, osv-dev
Ok, I'll keep it in mind from next time.
Thanks.
Thanks.
Commit Bot
Jun 15, 2014, 8:37:59 AM6/15/14
to osv...@googlegroups.com
From: Jaspal Singh Dhillon <jaspal...@gmail.com>
vfs: implement utimensat() and futimens()
Add utimensat() and futimens() which can update file timestamps with
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
Signed-off-by: Avi Kivity <a...@cloudius-systems.com>
---
diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
vfs: implement utimensat() and futimens()
Add utimensat() and futimens() which can update file timestamps with
nanosecond precision.
Implementation to handle AT_SYMLINK_NOFOLLOW flag is enabled by default
as namei currently does not follow symbolic links.
Signed-off-by: Jaspal Singh Dhillon <jaspal...@gmail.com>
---
diff --git a/fs/vfs/main.cc b/fs/vfs/main.cc
--- a/fs/vfs/vfs.h
+++ b/fs/vfs/vfs.h
@@ -117,6 +117,9 @@ int sys_statfs(char *path, struct statfs *buf);
int sys_truncate(char *path, off_t length);
ssize_t sys_readlink(char *path, char *buf, size_t bufsize);
int sys_utimes(char *path, const struct timeval times[2]);
+int sys_utimensat(int dirfd, const char *pathname,
+ const struct timespec times[2], int flags);
+int sys_futimens(int fd, const struct timespec times[2]);
int sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len);
int sys_mount(char *dev, char *dir, char *fsname, int flags, void *data);
diff --git a/fs/vfs/vfs_syscalls.cc b/fs/vfs/vfs_syscalls.cc
+++ b/fs/vfs/vfs.h
@@ -117,6 +117,9 @@ int sys_statfs(char *path, struct statfs *buf);
int sys_truncate(char *path, off_t length);
ssize_t sys_readlink(char *path, char *buf, size_t bufsize);
int sys_utimes(char *path, const struct timeval times[2]);
+int sys_utimensat(int dirfd, const char *pathname,
+ const struct timespec times[2], int flags);
+int sys_futimens(int fd, const struct timespec times[2]);
int sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len);
int sys_mount(char *dev, char *dir, char *fsname, int flags, void *data);
diff --git a/fs/vfs/vfs_syscalls.cc b/fs/vfs/vfs_syscalls.cc
+
+ std::string pathname = fp->f_dentry->d_path;
+ auto error = sys_utimensat(AT_FDCWD, pathname.c_str(), times, 0);
+ return error;
+}
+
int
sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len)
{
diff --git a/fs/vfs/vfs_vnode.c b/fs/vfs/vfs_vnode.c
+ std::string pathname = fp->f_dentry->d_path;
+ auto error = sys_utimensat(AT_FDCWD, pathname.c_str(), times, 0);
+ return error;
+}
+
int
sys_fallocate(struct file *fp, int mode, loff_t offset, loff_t len)
{
diff --git a/fs/vfs/vfs_vnode.c b/fs/vfs/vfs_vnode.c
Reply all
Reply to author
Forward
0 new messages