Re: renameat2
Hi Ken,
On Aug 18 18:24, Ken Brown wrote:
> Hi Corinna,
>
> On 8/18/2017 11:15 AM, Corinna Vinschen wrote:
> > On Aug 18 09:21, Ken Brown wrote:
> > But there's light. NtSetInformationFile(FileRenameInformation) already
> > supports RENAME_NOREPLACE :)
> >
> > Have a look at line 2494 (prior to your patch):
> >
> > pfri->ReplaceIfExists = TRUE;
> >
> > if you replace this with something like
> >
> > pfri->ReplaceIfExists = !(flags & RENAME_NOREPLACE);
> >
> > it should give us the atomic behaviour of renameat2 on Linux.
> >
> > Another upside is, the status code returned is STATUS_OBJECT_NAME_COLLISION,
> > which translates to Win32 error ERROR_ALREADY_EXISTS, which in turn is
> > already converted to EEXIST by Cygwin, so there's nothing more to do :)
> >
> > What do you think?
>
> Thanks for the improvements! A revised patch is attached. As you'll see, I
> still found a few places where I thought I needed to explicitly set the
> errno to EEXIST. Let me know if any of these could be avoided.
No, you're right. Just one thing:
> @@ -2410,6 +2433,11 @@ rename (const char *oldpath, const char *newpath)
>unlink_nt returns with STATUS_DIRECTORY_NOT_EMPTY. */
>if (dstpc->isdir ())
> {
> + if (noreplace)
> + {
> + set_errno (EEXIST);
> + __leave;
> + }
> status = unlink_nt (*dstpc);
> if (!NT_SUCCESS (status))
> {
> @@ -2423,6 +2451,11 @@ rename (const char *oldpath, const char *newpath)
>due to a mangled suffix. */
>else if (!removepc && dstpc->has_attribute (FILE_ATTRIBUTE_READONLY))
> {
> + if (noreplace)
> + {
> + set_errno (EEXIST);
> + __leave;
> + }
> status = NtOpenFile (&nfh, FILE_WRITE_ATTRIBUTES,
> dstpc->get_object_attr (attr, sec_none_nih),
> &io, FILE_SHARE_VALID_FLAGS,
Both of the above cases are just border cases of one and the same
problem, the destination file already exists.
In retrospect your original patch was more concise:
+ /* Should we replace an existing file? */
+ if ((removepc || dstpc->exists ()) && (flags & RENAME_NOREPLACE))
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
The atomicity considerations are not affected by this test anyway, but
it would avoid starting and stopping a transaction on NTFS for no good
reason.
Maybe it's better to revert to this test and drop the other two again?
> @@ -2491,11 +2524,15 @@ rename (const char *oldpath, const char *newpath)
> __leave;
> }
>pfri = (PFILE_RENAME_INFORMATION) tp.w_get ();
> - pfri->ReplaceIfExists = TRUE;
> + pfri->ReplaceIfExists = !noreplace;
>pfri->RootDirectory = NULL;
>pfri->FileNameLength = dstpc->get_nt_native_path ()->Length;
>memcpy (&pfri->FileName, dstpc->get_nt_native_path ()->Buffer,
> pfri->FileNameLength);
> + /* If dstpc points to an existing file and RENAME_NOREPLACE has
> + been specified, then we should get NT error
> + STATUS_OBJECT_NAME_COLLISION ==> Win32 error
> + ERROR_ALREADY_EXISTS ==> Cygwin error EEXIST. */
>status = NtSetInformationFile (fh, &io, pfri,
>sizeof *pfri + pfri->FileNameLength,
>FileRenameInformation);
> @@ -2509,6 +2546,11 @@ rename (const char *oldpath, const char *newpath)
>if (status == STATUS_ACCESS_DENIED && dstpc->exists ()
> && !dstpc->isdir ())
> {
> + if (noremove)
> + {
> + set_errno (EEXIST);
> + __leave;
> + }
Oh, right, that's a good catch.
The patch is ok as is, just let me know what you think of the above
minor tweak (and send the revised patch if you agree).
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
Re: renameat2
Hi Corinna,
On 8/19/2017 5:57 AM, Corinna Vinschen wrote:
Hi Ken,
On Aug 18 18:24, Ken Brown wrote:
Thanks for the improvements! A revised patch is attached. As you'll see, I
still found a few places where I thought I needed to explicitly set the
errno to EEXIST. Let me know if any of these could be avoided.
No, you're right. Just one thing:
@@ -2410,6 +2433,11 @@ rename (const char *oldpath, const char *newpath)
unlink_nt returns with STATUS_DIRECTORY_NOT_EMPTY. */
if (dstpc->isdir ())
{
+ if (noreplace)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
status = unlink_nt (*dstpc);
if (!NT_SUCCESS (status))
{
@@ -2423,6 +2451,11 @@ rename (const char *oldpath, const char *newpath)
due to a mangled suffix. */
else if (!removepc && dstpc->has_attribute (FILE_ATTRIBUTE_READONLY))
{
+ if (noreplace)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
status = NtOpenFile (&nfh, FILE_WRITE_ATTRIBUTES,
dstpc->get_object_attr (attr, sec_none_nih),
&io, FILE_SHARE_VALID_FLAGS,
Both of the above cases are just border cases of one and the same
problem, the destination file already exists.
In retrospect your original patch was more concise:
+ /* Should we replace an existing file? */
+ if ((removepc || dstpc->exists ()) && (flags & RENAME_NOREPLACE))
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
The atomicity considerations are not affected by this test anyway, but
it would avoid starting and stopping a transaction on NTFS for no good
reason.
Maybe it's better to revert to this test and drop the other two again?
@@ -2491,11 +2524,15 @@ rename (const char *oldpath, const char *newpath)
__leave;
}
pfri = (PFILE_RENAME_INFORMATION) tp.w_get ();
- pfri->ReplaceIfExists = TRUE;
+ pfri->ReplaceIfExists = !noreplace;
pfri->RootDirectory = NULL;
pfri->FileNameLength = dstpc->get_nt_native_path ()->Length;
memcpy (&pfri->FileName, dstpc->get_nt_native_path ()->Buffer,
pfri->FileNameLength);
+ /* If dstpc points to an existing file and RENAME_NOREPLACE has
+been specified, then we should get NT error
+STATUS_OBJECT_NAME_COLLISION ==> Win32 error
+ERROR_ALREADY_EXISTS ==> Cygwin error EEXIST. */
status = NtSetInformationFile (fh, &io, pfri,
sizeof *pfri + pfri->FileNameLength,
FileRenameInformation);
@@ -2509,6 +2546,11 @@ rename (const char *oldpath, const char *newpath)
if (status == STATUS_ACCESS_DENIED && dstpc->exists ()
&& !dstpc->isdir ())
{
+ if (noremove)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
Oh, right, that's a good catch.
The patch is ok as is, just let me know what you think of the above
minor tweak (and send the revised patch if you agree).
Yes, I agree. But can't I also drop the third test (where you said
"good catch") for the same reason? I've done that in the attached. If
I'm wrong and I still need that third test, let me know and I'll put it
back.
Thanks.
Ken
From bbff16b727172a459a9a052b0b948641c82d80eb Mon Sep 17 00:00:00 2001
From: Ken Brown
Date: Thu, 17 Aug 2017 09:12:15 -0400
Subject: [PATCH] cygwin: Implement renameat2
Define the RENAME_NOREPLACE flag in as defined on Linux
in . The other RENAME_* flags defined on Linux are not
supported.
---
newlib/libc/include/stdio.h| 3 +++
winsup/cygwin/common.din | 1 +
winsup/cygwin/include/cygwin/fs.h | 6 +
winsup/cygwin/include/cygwin/version.h | 3 ++-
winsup/cygwin/syscalls.cc | 48 +-
5 files changed, 54 insertions(+), 7 deletions(-)
diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h
index 5d8cb1092..331a1cf07 100644
--- a/newlib/libc/include/stdio.h
+++ b/newlib/libc/include/stdio.h
@@ -384,6 +384,9 @@ int _EXFUN(vdprintf, (int, const char *__restrict, __VALIST)
#endif
#if __ATFILE_VISIBLE
int_EXFUN(renameat, (int, const char *, int, const char *));
+# ifdef __CYGWIN__
+int_EXFUN(renameat2, (int, const char *, int, const char *, unsigned int));
+# endif
#endif
/*
diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din
index 8da432b8a..ca6ff3cf9 100644
--- a/winsup/cygwin/common.din
+++ b/winsup/cygwin/common.din
@@ -1168,6 +1168,7 @@ remquof NOSIGFE
remquol NOSIGFE
rename SIGFE
renameat SIGFE
+renameat2 SIGFE
res_close = __res_close SIGFE
res_init = __res_init SIGFE
res_mkquery = __res_mkquery SIGFE
diff --git a/winsup/cygwin/include/cygwin/fs.h
b/winsup/cygwin/include/cygwin/fs.h
index f606ffc39..48b0cca45 100644
--- a/winsup/cygwin/include/cygwin/fs.h
+++
Re: renameat2
On Aug 19 10:29, Ken Brown wrote: > Hi Corinna, > > On 8/19/2017 5:57 AM, Corinna Vinschen wrote: > > Hi Ken, > > > > On Aug 18 18:24, Ken Brown wrote: > > The patch is ok as is, just let me know what you think of the above > > minor tweak (and send the revised patch if you agree). > > Yes, I agree. But can't I also drop the third test (where you said "good > catch") for the same reason? I've done that in the attached. If I'm wrong > and I still need that third test, let me know and I'll put it back. Nope, you're right. Same rules apply for the third test. Patch pushed. Doc changes coming? :) Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: renameat2
On Aug 19 18:28, Corinna Vinschen wrote: > On Aug 19 10:29, Ken Brown wrote: > > Hi Corinna, > > > > On 8/19/2017 5:57 AM, Corinna Vinschen wrote: > > > Hi Ken, > > > > > > On Aug 18 18:24, Ken Brown wrote: > > > The patch is ok as is, just let me know what you think of the above > > > minor tweak (and send the revised patch if you agree). > > > > Yes, I agree. But can't I also drop the third test (where you said "good > > catch") for the same reason? I've done that in the attached. If I'm wrong > > and I still need that third test, let me know and I'll put it back. > > Nope, you're right. Same rules apply for the third test. Patch pushed. > Doc changes coming? :) Oh, one more thing. This is a question to Yaakov, too. diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h index 5d8cb1092..331a1cf07 100644 --- a/newlib/libc/include/stdio.h +++ b/newlib/libc/include/stdio.h @@ -384,6 +384,9 @@ int _EXFUN(vdprintf, (int, const char *__restrict, __VALIST) #endif #if __ATFILE_VISIBLE int_EXFUN(renameat, (int, const char *, int, const char *)); +# ifdef __CYGWIN__ +int_EXFUN(renameat2, (int, const char *, int, const char *, unsigned int)); +# endif #endif Does it makes sense to guard the renameat2 prototype more extensively to cater for standards junkies? __MISC_VISIBLE, perhaps? Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: renameat2
On 8/19/2017 12:37 PM, Corinna Vinschen wrote: > Oh, one more thing. This is a question to Yaakov, too. > > diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h > index 5d8cb1092..331a1cf07 100644 > --- a/newlib/libc/include/stdio.h > +++ b/newlib/libc/include/stdio.h > @@ -384,6 +384,9 @@ int _EXFUN(vdprintf, (int, const char *__restrict, > __VALIST) > #endif > #if __ATFILE_VISIBLE > int_EXFUN(renameat, (int, const char *, int, const char *)); > +# ifdef __CYGWIN__ > +int_EXFUN(renameat2, (int, const char *, int, const char *, unsigned > int)); > +# endif > #endif > > Does it makes sense to guard the renameat2 prototype more extensively > to cater for standards junkies? __MISC_VISIBLE, perhaps? I'll defer to Yaakov. But here's a related question. Is renameat currently guarded properly? The Linux man page says: Feature Test Macro Requirements for glibc (see feature_test_macros(7)): renameat(): Since glibc 2.10: _POSIX_C_SOURCE >= 200809L Before glibc 2.10: _ATFILE_SOURCE This suggests that we should do something like the following: diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h index 331a1cf07..9eb0964f2 100644 --- a/newlib/libc/include/stdio.h +++ b/newlib/libc/include/stdio.h @@ -381,8 +381,6 @@ FILE * _EXFUN(open_memstream, (char **, size_t *)); int_EXFUN(vdprintf, (int, const char *__restrict, __VALIST) _ATTRIBUTE ((__format__ (__printf__, 2, 0; # endif -#endif -#if __ATFILE_VISIBLE int_EXFUN(renameat, (int, const char *, int, const char *)); # ifdef __CYGWIN__ int_EXFUN(renameat2, (int, const char *, int, const char *, unsigned int)); Ken
Re: renameat2
On 8/19/2017 12:28 PM, Corinna Vinschen wrote: Doc changes coming? :) Attached. Ken From 0704541f1d29e0d9aa0af6e549f8ca0114a44a7c Mon Sep 17 00:00:00 2001 From: Ken Brown Date: Sat, 19 Aug 2017 13:15:04 -0400 Subject: [PATCH] Document renameat2 --- winsup/cygwin/release/2.9.0 | 2 ++ winsup/doc/new-features.xml | 4 winsup/doc/posix.xml| 4 3 files changed, 10 insertions(+) diff --git a/winsup/cygwin/release/2.9.0 b/winsup/cygwin/release/2.9.0 index 421d6f24f..ac4c64949 100644 --- a/winsup/cygwin/release/2.9.0 +++ b/winsup/cygwin/release/2.9.0 @@ -6,6 +6,8 @@ What's new: - New APIs: pthread_mutex_timedwait, pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock. +- New API: renameat2. + What changed: - diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml index 23673d1e0..0aa857730 100644 --- a/winsup/doc/new-features.xml +++ b/winsup/doc/new-features.xml @@ -17,6 +17,10 @@ New APIs: pthread_mutex_timedwait, pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock. + +New API: renameat2. + + Improved implementation of. diff --git a/winsup/doc/posix.xml b/winsup/doc/posix.xml index a2fffeebf..6e96272b7 100644 --- a/winsup/doc/posix.xml +++ b/winsup/doc/posix.xml @@ -1356,6 +1356,7 @@ also IEEE Std 1003.1-2008 (POSIX.1-2008). ptsname_r putwc_unlocked putwchar_unlocked +renameat2 (see chapter "Implementation Notes") qsort_r(see chapter "Implementation Notes") quotactl rawmemchr @@ -1671,6 +1672,9 @@ group quotas, no inode quotas, no time constraints. qsort_r is available in both BSD and GNU flavors, depending on whether _BSD_SOURCE or _GNU_SOURCE is defined when compiling. +The Linux-specific function renameat2 only +supports the RENAME_NOREPLACE flag. + basename is available in both POSIX and GNU flavors, depending on whether libgen.h is included or not. -- 2.14.1
Re: renameat2
On Aug 19 13:13, Ken Brown wrote: > On 8/19/2017 12:37 PM, Corinna Vinschen wrote: > > Oh, one more thing. This is a question to Yaakov, too. > > > > diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h > > index 5d8cb1092..331a1cf07 100644 > > --- a/newlib/libc/include/stdio.h > > +++ b/newlib/libc/include/stdio.h > > @@ -384,6 +384,9 @@ int _EXFUN(vdprintf, (int, const char *__restrict, > > __VALIST) > > #endif > > #if __ATFILE_VISIBLE > > int_EXFUN(renameat, (int, const char *, int, const char *)); > > +# ifdef __CYGWIN__ > > +int_EXFUN(renameat2, (int, const char *, int, const char *, unsigned > > int)); > > +# endif > > #endif > > > > Does it makes sense to guard the renameat2 prototype more extensively > > to cater for standards junkies? __MISC_VISIBLE, perhaps? > > I'll defer to Yaakov. But here's a related question. Is renameat > currently guarded properly? The Linux man page says: > > Feature Test Macro Requirements for glibc (see feature_test_macros(7)): > >renameat(): >Since glibc 2.10: >_POSIX_C_SOURCE >= 200809L >Before glibc 2.10: >_ATFILE_SOURCE > > This suggests that we should do something like the following: > > diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h > index 331a1cf07..9eb0964f2 100644 > --- a/newlib/libc/include/stdio.h > +++ b/newlib/libc/include/stdio.h > @@ -381,8 +381,6 @@ FILE * _EXFUN(open_memstream, (char **, size_t *)); > int_EXFUN(vdprintf, (int, const char *__restrict, __VALIST) > _ATTRIBUTE ((__format__ (__printf__, 2, 0; > # endif > -#endif > -#if __ATFILE_VISIBLE > int_EXFUN(renameat, (int, const char *, int, const char *)); > # ifdef __CYGWIN__ > int_EXFUN(renameat2, (int, const char *, int, const char *, unsigned > int)); No, that's correct. See sys/features.h: * __ATFILE_VISIBLE * "at" functions; enabled by default, with _ATFILE_SOURCE, * _POSIX_C_SOURCE >= 200809L, or _XOPEN_SOURCE >= 700. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
Re: renameat2
On Aug 19 13:24, Ken Brown wrote: > On 8/19/2017 12:28 PM, Corinna Vinschen wrote: > > Doc changes coming? :) > > Attached. Pushed. I also generated new developer snapshots. Thanks, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat signature.asc Description: PGP signature
