Page MenuHomeFreeBSD

Add syscall to set more than 2 times
Needs ReviewPublic

Authored by walker.aj325_gmail.com on Jul 8 2020, 5:55 PM.

Details

Reviewers
freqlabs
brooks
Summary

When a filesystem is shared with Samba, SMB clients may request to change the timestamps on files. This often happens when a user uses the "robocopy" windows utility to synchronize a directory on a windows computer to a FreeBSD samba server. Due to some quirks of how robocopy works, it first sends a request to set the modify time to a date in 1980 and then after that succeeds, the client sends a second request to set the btime, mtime, and atime to the correct values to preserve what is on the original file.

Unfortunately, the current mechanism for modifying birthtime does not allow us to go forward in time. Moreover, Windows clients often request to set a btime that is more recent than the mtime, and if we don't honor this unfortunate abuse of timestamps, many backup utilities will keep trying to re-sync timestamps with us.

Proposal is to add a new syscall that allows setting btime, mtime, and atime simultaneously.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

walker.aj325_gmail.com requested review of this revision.Jul 8 2020, 5:55 PM

Add context to diff

brooks added a comment.Jul 8 2020, 6:59 PM

Some general comments in no particular order:

  • pathseg strikes me as over engineering. While trivial to implement the value is always the same.
  • I'm a bit skeptical of the overall interface in particular the cnt argument. It does match setutimes but that interface is odd (and there doesn't appear to be any in-tree consumer of the numtimes > 2).
  • A manpage would be required for commit.
  • Please either don't include generated files or update to a version of FreeBSD that adds @generated to them so they don't show up in the diff.
sys/compat/freebsd32/freebsd32_misc.c
1779
		error = copyin(uap->times, ts32, sizeof(ts32) * uap->cnt);

You also need to check that uap->cnt <= MAX_UTIMES

sys/kern/syscalls.master
3248

_In_reads_(cnt) or cnt shouldn't exist.

sys/kern/vfs_syscalls.c
3075

blank line here

3084

The copying below need to be in an else block of you'll unconditionally dereference NULL.

3111

I seen no path where this is true, likewise the path below.

Emerging style would encourage bool here.

Some general comments in no particular order:

  • pathseg strikes me as over engineering. While trivial to implement the value is always the same.
  • I'm a bit skeptical of the overall interface in particular the cnt argument. It does match setutimes but that interface is odd (and there doesn't appear to be any in-tree consumer of the numtimes > 2).
  • A manpage would be required for commit.
  • Please either don't include generated files or update to a version of FreeBSD that adds @generated to them so they don't show up in the diff.

Sorry it's taken bit of time to respond. Thanks for the feedback! I'll make the changes soon. The main place where I'm planning to set btime, atime, and mtime simultaneously is in Samba's VFS. This is so that can provide proper behavior for NT times without having to resort to storing them in xattrs. I saw some old discussion in mailing lists about possibly adding new timespecs (for instance last archived time). I thought it might be a good idea to be future-proofed regarding this if the issue is raised again and the decision is made to add extra timespecs.