Conversation
syscall.c: Add fchmodat(), if it exists on the system, as a last-resort attempt to change the Unix access permissions for a symlink (on those systems that support it). This is done just prior to finally abandoning such an attempt and after any other available options for this, such as lchmod() or setattrlist(). This option is required to preserve symlink access permissions when making copies using rsync on OpenBSD. The patch itself is not OS-specific. testsuite: Add a test, symlink-unix-perms_test.py, to testsuite to check on the success of setting symlink access changes using rsync. The test is skipped on those systems that do not allow symlink access permission changes. The test checks at runtime whether or not the system permits symlink access changes. As with the patch, this test is not OS-specific.
|
Make the test deterministic, use a normal Python shebang and catch only the expected unsupported-operation errors. A programming error or unexpected filesystem failure must fail the test rather than silently skip it. Please also add a configure-time compile check for the four-argument |
|
Thanks for looking at this. It's really appreciated. Apologies for the goofs in the test script. I should (and do) know better. A repair is in the works. I originally had a configure check for the existence of fchmodat in the change. I removed it under the (clearly misguided) impression that it was unnecessary given that fchmodat (with AT_SYMLINK_NOFOLLOW) is already in use by rsync (in syscall.c). I can (will) put the check for fchmodat back in but (at the moment) I have no real idea how to check for specifically a 4 argument versus something different. If you know of a pointer, it would really help. My knowledge of autoconf is somewhat limited. Again, thanks for looking at this. |
|
I think I may have found what I'm looking for vis-a-vis argument count checking for fchmodat via autoconf. |
|
Ah. Actually. No need to wrestle with Autoconf here. My request for another configure check was wrong. We already call the four-argument The implementation itself looks appropriately narrow. The remaining work is in the test: use |
|
OK. Sounds good. Thanks I'm almost done with the test. I'm just doing a bit of minor cleanup and sending it through its paces. It should be ready shortly. |
|
Much appreciated. |
84aee7d to
f773141
Compare
…andards. 1) Set the proper python shebang 2) Remove the use of random test access values in favour of fixed values. 3) Skip the test only on "not implemented" or "not supported" exceptions. All other unexpected errors will fail the test. 4) Test using symlinks to a specifically created directory or file as opposed to the '.' directory. The latter could potentially lead to cyclical paths. Some code tweaking has also been done for readability.
|
I just upload a new copy of the test program. I think it addresses all of the concerns: shebang updated, use of random values removed, more precise checking of exception conditions. I also did a little bit of tweaking to the code for readability and, perhaps, a little more robustness. This should not have introduced any similar issues. I re-ran all of my original testing and the results were all as I would expect. Please take a look. Many thanks! |
steadytao
left a comment
steadytao
left a comment
There was a problem hiding this comment.
Mostly okay now. Just the comment and removing the unused random import 🤨
| if target == 'directory': | ||
| isdir = True | ||
| else: | ||
| isdir = False | ||
|
|
||
| os.symlink(f'{target}', FROMDIR / f'symlink-{n}', target_is_directory=f'{isdir}') |
There was a problem hiding this comment.
target is never the literal "directory" -- then converting isdir to "False" makes it truthy anyway. Compare target with testdirectory and pass the boolean directly.
2 participants
Many (perhaps all) BSD Unix systems allow users to set/modify the Unix access permissions on a symlink (as opposed to its target). OpenBSD does permit this but requires the use of fchmodat() to do so. Rsync does not provide the use of fchmodat() in this situation so it does not preserve symlink permissions when copying in OpenBSD.
The patch in the PR modifies syscall.c to provide the use of fchmodat(), on those systems that have it, as a last resort effort to setting a symlink permission prior to (ultimately) giving up on the attempt. This patch is not specific to any operating system; it only relies on the availability of fchmodat();
The PR also includes a testsuite test in that checks so see if symlink Unix access permissions are preserved on those systems that permit their change. The test is skipped on systems, such as (most) Linux variants, that don't allow changes to symlink Unix access permissions. The test, too, is not OS-specific. It tests at runtime to see if symlink access can be changed on the system and behaves accordingly.
I've run the testsuite with this patch applied on OpenBSD (7.9), FreeBSD (15.1), MacOS (Sequoia), ArchLinux (7.2.7-arch1-1), Debian (13), and Ubuntu (26.04.1 LTS). The results were all as expected. I've also run a complete system copy with Rsync on OpenBSD and Debian. On comparison of the results, the original and the copy were the same. The comparison made use of values obtained via the Unix lstat() system routine as well as MD5 checksums of file contents.
My apologies if I've submitted incorrectly. This is my first attempt at a PR.
Thanks for your attention.