GNU bug report logs -
#29961
[PATCH] mv: document the missing atomicity of 'mv -n'
Previous Next
Reported by: Kamil Dudka <kdudka <at> redhat.com>
Date: Wed, 3 Jan 2018 15:03:02 UTC
Severity: normal
Tags: patch
Done: Paul Eggert <eggert <at> cs.ucla.edu>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29961 in the body.
You can then email your comments to 29961 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 03 Jan 2018 15:03:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Kamil Dudka <kdudka <at> redhat.com>
:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org
.
(Wed, 03 Jan 2018 15:03:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
* src/mv.c (usage): Add note about the missing atomicity of 'mv -n'.
* doc/coreutils.texi (mv invocation): Likewise.
---
doc/coreutils.texi | 3 +++
src/mv.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3fa083085..e7ca6a737 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9660,6 +9660,9 @@ If the response is not affirmative, the file is skipped.
Do not overwrite an existing file.
@mvOptsIfn
This option is mutually exclusive with @option{-b} or @option{--backup} option.
+Note that the operation is not atomic. If @var{dest} appears after
+@command{mv} has checked its non-existence, it can be overwritten despite
+the @option{-n} option is used.
@item -u
@itemx --update
diff --git a/src/mv.c b/src/mv.c
index a8df730a7..a08e75649 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -306,6 +306,8 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
-i, --interactive prompt before overwrite\n\
-n, --no-clobber do not overwrite an existing file\n\
If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
+NOTE: The operation is not atomic. If DEST appears after mv has checked its\n\
+non-existence, it can be overwritten despite the -n option is used.\n\
"), stdout);
fputs (_("\
--strip-trailing-slashes remove any trailing slashes from each SOURCE\n\
--
2.13.6
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 03 Jan 2018 15:09:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On 03/01/18 15:01, Kamil Dudka wrote:
> * src/mv.c (usage): Add note about the missing atomicity of 'mv -n'.
> * doc/coreutils.texi (mv invocation): Likewise.
> ---
> doc/coreutils.texi | 3 +++
> src/mv.c | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 3fa083085..e7ca6a737 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -9660,6 +9660,9 @@ If the response is not affirmative, the file is skipped.
> Do not overwrite an existing file.
> @mvOptsIfn
> This option is mutually exclusive with @option{-b} or @option{--backup} option.
> +Note that the operation is not atomic. If @var{dest} appears after
> +@command{mv} has checked its non-existence, it can be overwritten despite
> +the @option{-n} option is used.
>
> @item -u
> @itemx --update
> diff --git a/src/mv.c b/src/mv.c
> index a8df730a7..a08e75649 100644
> --- a/src/mv.c
> +++ b/src/mv.c
> @@ -306,6 +306,8 @@ Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.\n\
> -i, --interactive prompt before overwrite\n\
> -n, --no-clobber do not overwrite an existing file\n\
> If you specify more than one of -i, -f, -n, only the final one takes effect.\n\
> +NOTE: The operation is not atomic. If DEST appears after mv has checked its\n\
> +non-existence, it can be overwritten despite the -n option is used.\n\
> "), stdout);
> fputs (_("\
> --strip-trailing-slashes remove any trailing slashes from each SOURCE\n\
Eep, Seems like we should use RENAME_NOREPLACE in this case,
rather than document the caveat?
This is already used in shred.
cheers,
Pádraig
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 03 Jan 2018 16:27:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On Wednesday, January 3, 2018 4:08:51 PM CET Pádraig Brady wrote:
> Eep, Seems like we should use RENAME_NOREPLACE in this case,
> rather than document the caveat?
Thanks for the suggestion! I will give it a try...
Kamil
> This is already used in shred.
>
> cheers,
> Pádraig
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 03 Jan 2018 16:28:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 03 Jan 2018 18:25:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On Wed, Jan 3, 2018 at 8:27 AM, Kamil Dudka <kdudka <at> redhat.com> wrote:
> On Wednesday, January 3, 2018 4:08:51 PM CET Pádraig Brady wrote:
>> Eep, Seems like we should use RENAME_NOREPLACE in this case,
>> rather than document the caveat?
Good catch/fix. Thanks to both of you.
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Thu, 04 Jan 2018 09:01:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 29961 <at> debbugs.gnu.org (full text, mbox):
... if it is created by another process after mv has checked its
non-existence.
* src/copy.c (copy_internal): Use renameat2 (..., RENAME_NOREPLACE)
if called by mv -n. If it fails with EEXIST in that case, pretend
successful rename as if the existing destination file was detected
by the preceding lstat call.
* NEWS: Mention the fix.
Fixes https://bugs.gnu.org/29961
---
NEWS | 3 +++
src/copy.c | 17 ++++++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index b89254f7e..e463ce6db 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ GNU coreutils NEWS -*- outline -*-
df no longer hangs when given a fifo argument.
[bug introduced in coreutils-7.3]
+ mv -n no longer overwrites the destination if it is created by another
+ process after mv has checked its non-existence.
+
ptx -S no longer infloops for a pattern which returns zero-length matches.
[the bug dates back to the initial implementation]
diff --git a/src/copy.c b/src/copy.c
index 2a804945e..be4e357a8 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -53,6 +53,7 @@
#include "ignore-value.h"
#include "ioblksize.h"
#include "quote.h"
+#include "renameat2.h"
#include "root-uid.h"
#include "same.h"
#include "savedir.h"
@@ -2319,7 +2320,12 @@ copy_internal (char const *src_name, char const *dst_name,
if (x->move_mode)
{
- if (rename (src_name, dst_name) == 0)
+ int flags = 0;
+ if (x->interactive == I_ALWAYS_NO)
+ /* do not replace DST_NAME if it was created since our last check */
+ flags = RENAME_NOREPLACE;
+
+ if (renameat2 (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags) == 0)
{
if (x->verbose)
{
@@ -2351,6 +2357,15 @@ copy_internal (char const *src_name, char const *dst_name,
return true;
}
+ if ((flags & RENAME_NOREPLACE) && (errno == EEXIST))
+ {
+ /* Pretend the rename succeeded, so the caller (mv)
+ doesn't end up removing the source file. */
+ if (rename_succeeded)
+ *rename_succeeded = true;
+ return true;
+ }
+
/* FIXME: someday, consider what to do when moving a directory into
itself but when source and destination are on different devices. */
--
2.13.6
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Thu, 04 Jan 2018 09:50:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 29961 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Kamil Dudka wrote:
> - if (rename (src_name, dst_name) == 0)
> + int flags = 0;
> + if (x->interactive == I_ALWAYS_NO)
> + /* do not replace DST_NAME if it was created since our last check */
> + flags = RENAME_NOREPLACE;
By then it's too late, as multiple decisions have been made on the basis of
stat/lstat calls that are still subject to races. It's better to try the
renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing code
only if renameat2 fails with errno != EEXIST.
Plus, there are some other problems when combining -u and -n.
How about the attached patch instead?
[0001-mv-improve-n-atomicity.patch (text/x-patch, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Thu, 04 Jan 2018 11:02:01 GMT)
Full text and
rfc822 format available.
Message #26 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
> Kamil Dudka wrote:
> > - if (rename (src_name, dst_name) == 0)
> > + int flags = 0;
> > + if (x->interactive == I_ALWAYS_NO)
> > + /* do not replace DST_NAME if it was created since our last check
> > */ + flags = RENAME_NOREPLACE;
>
> By then it's too late, as multiple decisions have been made on the basis of
> stat/lstat calls that are still subject to races.
Do you mean in the case of mv -n? Which decisions exactly?
> It's better to try the
> renameat2 with RENAME_NOREPLACE first thing, and fall back on the existing
> code only if renameat2 fails with errno != EEXIST.
>
> Plus, there are some other problems when combining -u and -n.
Sounds like a corner case. Please consider writing a separate patch for that.
> How about the attached patch instead?
I had difficulties trying to evaluate the patch. It does not compile
with gcc-7.2.1:
src/copy.c: In function 'copy_internal':
src/copy.c:2340:17: error: 'earlier_file' may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (! create_hard_link (earlier_file, dst_name, true, x->verbose,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
dereference))
~~~~~~~~~~~~
src/copy.c:2050:14: error: 'return_now' may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (return_now)
^
So I added initializers to those two variables but then I saw multiple
test-cases failing because of the patch:
FAIL: tests/mv/hard-link-1
FAIL: tests/mv/mv-special-1
FAIL: tests/mv/part-fail
FAIL: tests/mv/part-hardlink
FAIL: tests/mv/part-rename
FAIL: tests/mv/part-symlink
Kamil
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Fri, 05 Jan 2018 01:02:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 29961 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 01/04/2018 03:01 AM, Kamil Dudka wrote:
> On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
>> Kamil Dudka wrote:
>>> - if (rename (src_name, dst_name) == 0)
>>> + int flags = 0;
>>> + if (x->interactive == I_ALWAYS_NO)
>>> + /* do not replace DST_NAME if it was created since our last check
>>> */ + flags = RENAME_NOREPLACE;
>> By then it's too late, as multiple decisions have been made on the basis of
>> stat/lstat calls that are still subject to races.
> Do you mean in the case of mv -n? Which decisions exactly?
Mostly mv -n, but I suspect problems also for mv without -n. It's all
the decisions that depend on the result of lstat of dst_name, before
abandon_move decides whether to skip the rename. With the patch you
proposed, mv -n could call lstat and get a failure (with errno ==
ENOENT), then (after another process creates the file) call renameat2
with RENAME_NOREPLACE and after this fails (with errno == EEXIST) report
an error. mv -n should silently succeed in that case.
> Sounds like a corner case. Please consider writing a separate patch
> for that.
OK, that's pretty straightforward so I installed it. Please see the
first attached patch.
> I had difficulties trying to evaluate the patch. It does not compile
That's what I get for sending an untested patch. Sorry. I fixed the bugs
you mentioned and tested the result. Please see the second attached
patch, which I have not installed.
There is an interesting behavior change with this second patch.
Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the
same file". With the patch, 'mv -n a a' silently succeeds. The coreutils
documentation allows both behaviors. I doubt whether anyone cares, and
doing it the new way avoids some syscalls so I left it that way.
[0001-mv-n-overrides-u.patch (text/x-patch, attachment)]
[0002-mv-improve-n-atomicity.patch (text/x-patch, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Fri, 05 Jan 2018 11:56:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On Friday, January 5, 2018 2:00:52 AM CET Paul Eggert wrote:
> On 01/04/2018 03:01 AM, Kamil Dudka wrote:
> > On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
> >> Kamil Dudka wrote:
> >>> - if (rename (src_name, dst_name) == 0)
> >>> + int flags = 0;
> >>> + if (x->interactive == I_ALWAYS_NO)
> >>> + /* do not replace DST_NAME if it was created since our last
> >>> check
> >>> */ + flags = RENAME_NOREPLACE;
> >>
> >> By then it's too late, as multiple decisions have been made on the basis
> >> of
> >> stat/lstat calls that are still subject to races.
> >
> > Do you mean in the case of mv -n? Which decisions exactly?
>
> Mostly mv -n, but I suspect problems also for mv without -n.
My patch changes nothing but the 'mv -n' behavior. It could hardly break
(or even change behavior of) anything else. Your patch reworks the logic
of copy_internal(), which itself is very fragile, as you learned from the
first version of your patch.
> It's all
> the decisions that depend on the result of lstat of dst_name, before
> abandon_move decides whether to skip the rename.
I am only fixing the case where the destination file is created after the
lstat() call. In that case, the only result is ENOENT, which is harmless.
> With the patch you
> proposed, mv -n could call lstat and get a failure (with errno ==
> ENOENT), then (after another process creates the file) call renameat2
> with RENAME_NOREPLACE and after this fails (with errno == EEXIST)
Exactly. That is the only case that my patch intended to fix.
> report an error.
Nope. It will silently succeed with my patch. Did you try it?
> mv -n should silently succeed in that case.
I agree.
> > Sounds like a corner case. Please consider writing a separate patch
> > for that.
>
> OK, that's pretty straightforward so I installed it. Please see the
> first attached patch.
>
> > I had difficulties trying to evaluate the patch. It does not compile
>
> That's what I get for sending an untested patch. Sorry. I fixed the bugs
> you mentioned and tested the result. Please see the second attached
> patch, which I have not installed.
Thanks for the fixup!
> There is an interesting behavior change with this second patch.
> Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the
> same file". With the patch, 'mv -n a a' silently succeeds. The coreutils
> documentation allows both behaviors. I doubt whether anyone cares, and
> doing it the new way avoids some syscalls so I left it that way.
If you decide to apply your patch anyway, please document this change
at least in the commit message. Still my concern is that your patch also
changes the behavior of 'mv' without '-n', which is neither tested, nor
documented.
Kamil
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Fri, 05 Jan 2018 15:30:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On 05/01/18 11:56, Kamil Dudka wrote:
> On Friday, January 5, 2018 2:00:52 AM CET Paul Eggert wrote:
>> On 01/04/2018 03:01 AM, Kamil Dudka wrote:
>>> On Thursday, January 4, 2018 10:48:56 AM CET Paul Eggert wrote:
>>>> Kamil Dudka wrote:
>>>>> - if (rename (src_name, dst_name) == 0)
>>>>> + int flags = 0;
>>>>> + if (x->interactive == I_ALWAYS_NO)
>>>>> + /* do not replace DST_NAME if it was created since our last
>>>>> check
>>>>> */ + flags = RENAME_NOREPLACE;
>>>>
>>>> By then it's too late, as multiple decisions have been made on the basis
>>>> of
>>>> stat/lstat calls that are still subject to races.
>>>
>>> Do you mean in the case of mv -n? Which decisions exactly?
>>
>> Mostly mv -n, but I suspect problems also for mv without -n.
>
> My patch changes nothing but the 'mv -n' behavior. It could hardly break
> (or even change behavior of) anything else. Your patch reworks the logic
> of copy_internal(), which itself is very fragile, as you learned from the
> first version of your patch.
>
>> It's all
>> the decisions that depend on the result of lstat of dst_name, before
>> abandon_move decides whether to skip the rename.
>
> I am only fixing the case where the destination file is created after the
> lstat() call. In that case, the only result is ENOENT, which is harmless.
>
>> With the patch you
>> proposed, mv -n could call lstat and get a failure (with errno ==
>> ENOENT), then (after another process creates the file) call renameat2
>> with RENAME_NOREPLACE and after this fails (with errno == EEXIST)
>
> Exactly. That is the only case that my patch intended to fix.
>
>> report an error.
>
> Nope. It will silently succeed with my patch. Did you try it?
>
>> mv -n should silently succeed in that case.
>
> I agree.
>
>>> Sounds like a corner case. Please consider writing a separate patch
>>> for that.
>>
>> OK, that's pretty straightforward so I installed it. Please see the
>> first attached patch.
>>
>>> I had difficulties trying to evaluate the patch. It does not compile
>>
>> That's what I get for sending an untested patch. Sorry. I fixed the bugs
>> you mentioned and tested the result. Please see the second attached
>> patch, which I have not installed.
>
> Thanks for the fixup!
>
>> There is an interesting behavior change with this second patch.
>> Currently, 'mv -n a a' fails with a diagnostic "mv: 'a' and 'a' are the
>> same file". With the patch, 'mv -n a a' silently succeeds. The coreutils
>> documentation allows both behaviors. I doubt whether anyone cares, and
>> doing it the new way avoids some syscalls so I left it that way.
>
> If you decide to apply your patch anyway, please document this change
> at least in the commit message. Still my concern is that your patch also
> changes the behavior of 'mv' without '-n', which is neither tested, nor
> documented.
Thanks to both of you.
The approaches can be summarized as:
Orig:
---------------------------------
stat() => ENOENT
<file created externally>
rename() may clobber file
Kamil's:
---------------------------------
stat() => ENOENT
<file created externally>
renameat() doesn't clobber file if -n
exit early if -n && errno==EEXIST
Paul's:
---------------------------------
renameat2() => EEXIST
-n => exit early
if (renameat2_failed)
unless EEXIST && -n
stat()
if (renameat2_failed)
rename()
I think both patches ensure rename() doesn't clobber when -n is specified.
Paul's is more encompassing for the non -n case.
For example if a directory was _created_ externally
after the stat() in Kamil's logic above,
it could be erroneously clobbered.
Paul's also avoids a stat() in the common case
where the initial renameat2() succeeds.
Both versions are still susceptible to erroneous clobbering
if the destination file was externally _replaced_
by a directory for example, after the stat().
Though that is less likely.
Paul's fix looks good to apply here,
since it's more encompassing.
cheers,
Pádraig
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Fri, 05 Jan 2018 16:20:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
> Thanks to both of you.
> The approaches can be summarized as:
>
> Orig:
> ---------------------------------
> stat() => ENOENT
> <file created externally>
> rename() may clobber file
>
> Kamil's:
> ---------------------------------
> stat() => ENOENT
> <file created externally>
> renameat() doesn't clobber file if -n
> exit early if -n && errno==EEXIST
>
> Paul's:
> ---------------------------------
> renameat2() => EEXIST
> -n => exit early
> if (renameat2_failed)
> unless EEXIST && -n
> stat()
> if (renameat2_failed)
> rename()
>
> I think both patches ensure rename() doesn't clobber when -n is specified.
Thanks for the summary!
> Paul's is more encompassing for the non -n case.
> For example if a directory was _created_ externally
> after the stat() in Kamil's logic above,
> it could be erroneously clobbered.
Do you mean without -n?
I am getting the following error message in that case:
mv: cannot move 'a' to 'b': Is a directory
... which is consistent with the original behavior.
> Paul's also avoids a stat() in the common case
> where the initial renameat2() succeeds.
At the cost of _not_ avoiding the renameat2() call in the most common case.
I think both the solutions are equivalent performance-wise.
> Both versions are still susceptible to erroneous clobbering
> if the destination file was externally _replaced_
> by a directory for example, after the stat().
> Though that is less likely.
>
> Paul's fix looks good to apply here,
> since it's more encompassing.
No problem with that. Anyway, I will go with my conservative (or even the
documentation-only) patch for RHEL-7, which was the trigger of this effort,
because Paul's patch comes with changes of behavior observable beyond the
reported scenario.
Kamil
> cheers,
> Pádraig
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Fri, 05 Jan 2018 23:00:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 29961 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 01/05/2018 08:19 AM, Kamil Dudka wrote:
> I am only fixing the case where the destination file is created after the
> lstat() call. In that case, the only result is ENOENT, which is harmless.
>
Ah, you're right. Sorry, I misread your patch. It should work.
> On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
>
>> Paul's also avoids a stat() in the common case
>> where the initial renameat2() succeeds.
> At the cost of _not_ avoiding the renameat2() call in the most common case.
I expect that the most common case is 'mv A B' where B does not already
exist. This is the case that avoids the stat of B.
Come to think of it, we don't need to stat A either, when the initial
renameat2 succeeds. Attached is a revised proposed patchset to do that.
The first is the same as before; the second causes 'mv A B' to issue
just a renameat2 syscall (with no calls to stat) in the common case
where A exists and B does not.
> I will go with my conservative (or even the
> documentation-only) patch for RHEL-7, which was the trigger of this effort,
> because Paul's patch comes with changes of behavior observable beyond the
Sounds good.
[0001-mv-improve-n-atomicity.txt (text/plain, attachment)]
[0002-mv-fewer-syscalls-for-mv-a-b.txt (text/plain, attachment)]
[0003-mv-clarify-mv-n-A-A-change.txt (text/plain, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Sat, 06 Jan 2018 09:23:01 GMT)
Full text and
rfc822 format available.
Message #44 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On 01/05/2018 11:59 PM, Paul Eggert wrote:
> Attached is a revised proposed patchset to do that.
I didn't have a look at the details, but - as we're changing the
behavior for some cases - it would be good to also add some tests.
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Sat, 06 Jan 2018 10:07:02 GMT)
Full text and
rfc822 format available.
Message #47 received at 29961 <at> debbugs.gnu.org (full text, mbox):
On 05/01/18 22:59, Paul Eggert wrote:
> On 01/05/2018 08:19 AM, Kamil Dudka wrote:
>
>> I am only fixing the case where the destination file is created after the
>> lstat() call. In that case, the only result is ENOENT, which is harmless.
>>
>
> Ah, you're right. Sorry, I misread your patch. It should work.
>
>> On Friday, January 5, 2018 4:29:55 PM CET Pádraig Brady wrote:
>>
>>> Paul's also avoids a stat() in the common case
>>> where the initial renameat2() succeeds.
>> At the cost of _not_ avoiding the renameat2() call in the most common case.
>
> I expect that the most common case is 'mv A B' where B does not already
> exist. This is the case that avoids the stat of B.
>
> Come to think of it, we don't need to stat A either, when the initial
> renameat2 succeeds. Attached is a revised proposed patchset to do that.
> The first is the same as before; the second causes 'mv A B' to issue
> just a renameat2 syscall (with no calls to stat) in the common case
> where A exists and B does not.
This second patch looks good also.
thanks,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Sat, 06 Jan 2018 20:11:01 GMT)
Full text and
rfc822 format available.
Message #50 received at 29961 <at> debbugs.gnu.org (full text, mbox):
Bernhard Voelker wrote:
> as we're changing the
> behavior for some cases - it would be good to also add some tests
That would make sense if the edge-case behavior was documented and the change is
something that users could rely on. But the behavior is not documented, and I'm
not sure we want to promise that the behavior won't change back. (Maybe I'm
being too cautious?)
Reply sent
to
Paul Eggert <eggert <at> cs.ucla.edu>
:
You have taken responsibility.
(Wed, 10 Jan 2018 08:54:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Kamil Dudka <kdudka <at> redhat.com>
:
bug acknowledged by developer.
(Wed, 10 Jan 2018 08:54:02 GMT)
Full text and
rfc822 format available.
Message #55 received at 29961-done <at> debbugs.gnu.org (full text, mbox):
No further comment, so I installed the proposed patches and I'm closing this bug
report.
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 10 Jan 2018 10:48:01 GMT)
Full text and
rfc822 format available.
Message #58 received at submit <at> debbugs.gnu.org (full text, mbox):
On Wednesday, January 10, 2018 9:53:07 AM CET Paul Eggert wrote:
> No further comment, so I installed the proposed patches and I'm closing this
> bug report.
Sorry. There were just too many changes in your patches for me to review
them quickly enough (and most of the changes were unrelated to my initial
submission anyway).
Thank you for fixing it!
Kamil
Information forwarded
to
bug-coreutils <at> gnu.org
:
bug#29961
; Package
coreutils
.
(Wed, 10 Jan 2018 10:48:02 GMT)
Full text and
rfc822 format available.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org
.
(Wed, 07 Feb 2018 12:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 6 years and 52 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.