Patchwork [2,of,2] util: move checknlink away from the dependency of a hard-coded filename

login
register
mail settings
Submitter Tony Tung
Date Aug. 19, 2016, 8:58 p.m.
Message ID <4aaeb57d5f4497ef35b4.1471640320@andromeda.local>
Download mbox | patch
Permalink /patch/16362/
State Changes Requested
Headers show

Comments

Tony Tung - Aug. 19, 2016, 8:58 p.m.
# HG changeset patch
# User Tony Tung <tonytung@merly.org>
# Date 1471639931 25200
#      Fri Aug 19 13:52:11 2016 -0700
# Node ID 4aaeb57d5f4497ef35b48e23b80e43b914afd819
# Parent  2efc5a05d80a6d4253767f2ce0c2fb062ba83cb6
util: move checknlink away from the dependency of a hard-coded filename

This somewhat insulates us against bugs in checknlink when a stale file
left behind could result in checknlink always returning False.
Yuya Nishihara - Aug. 23, 2016, 3:28 p.m.
On Fri, 19 Aug 2016 13:58:40 -0700, ttung@fb.com wrote:
> # HG changeset patch
> # User Tony Tung <tonytung@merly.org>
> # Date 1471639931 25200
> #      Fri Aug 19 13:52:11 2016 -0700
> # Node ID 4aaeb57d5f4497ef35b48e23b80e43b914afd819
> # Parent  2efc5a05d80a6d4253767f2ce0c2fb062ba83cb6
> util: move checknlink away from the dependency of a hard-coded filename

> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1305,34 +1305,33 @@
>  def checknlink(testfile):
>      '''check whether hardlink count reporting works properly'''
>  
> +    dirpath, filename = os.path.split(testfile)
> +
>      # testfile may be open, so we need a separate file for checking to
>      # work around issue2543 (or testfile may get lost on Samba shares)
> -    f1 = testfile + ".hgtmp1"
> -    if os.path.lexists(f1):
> -        return False
> +    f1handle, f1path = tempfile.mkstemp(prefix=filename, dir=dirpath)
> +    os.close(f1handle)
> +
> +    f2handle, f2path = tempfile.mkstemp(prefix=filename, dir=dirpath)
> +    os.close(f2handle)
> +    f2handle = None

As you've mentioned in the previous patch, we should delete f1/f2path on error.
Other than that, this series looks good to me.

> +    # there's a small race condition that another file can jam itself in between
> +    # the time we remove f2 and the time we create the hard link.  in the
> +    # unlikely scenario that happens, we'll treat it as nlink being unreliable.
>      try:
[...]
> +        os.unlink(f2path)
> +        oslink(f1path, f2path)

I would use f2path = f1path + '.2' assuming f1path has enough random bits, but
your version, mkstemp+unlink+link, should be more robust for a race condition.
via Mercurial-devel - Aug. 23, 2016, 5:30 p.m.
> On Aug 23, 2016, at 8:28 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> 

> On Fri, 19 Aug 2016 13:58:40 -0700, ttung@fb.com wrote:

>> # HG changeset patch

>> # User Tony Tung <tonytung@merly.org>

>> # Date 1471639931 25200

>> #      Fri Aug 19 13:52:11 2016 -0700

>> # Node ID 4aaeb57d5f4497ef35b48e23b80e43b914afd819

>> # Parent  2efc5a05d80a6d4253767f2ce0c2fb062ba83cb6

>> util: move checknlink away from the dependency of a hard-coded filename

> 

>> --- a/mercurial/util.py

>> +++ b/mercurial/util.py

>> @@ -1305,34 +1305,33 @@

>> def checknlink(testfile):

>>     '''check whether hardlink count reporting works properly'''

>> 

>> +    dirpath, filename = os.path.split(testfile)

>> +

>>     # testfile may be open, so we need a separate file for checking to

>>     # work around issue2543 (or testfile may get lost on Samba shares)

>> -    f1 = testfile + ".hgtmp1"

>> -    if os.path.lexists(f1):

>> -        return False

>> +    f1handle, f1path = tempfile.mkstemp(prefix=filename, dir=dirpath)

>> +    os.close(f1handle)

>> +

>> +    f2handle, f2path = tempfile.mkstemp(prefix=filename, dir=dirpath)

>> +    os.close(f2handle)

>> +    f2handle = None

> 

> As you've mentioned in the previous patch, we should delete f1/f2path on error.

> Other than that, this series looks good to me.


There’s the unlinking of f1/f2path in the finally clause, but I guess that doesn’t cover earlier errors.  Let me fix that.

> 

>> +    # there's a small race condition that another file can jam itself in between

>> +    # the time we remove f2 and the time we create the hard link.  in the

>> +    # unlikely scenario that happens, we'll treat it as nlink being unreliable.

>>     try:

> [...]

>> +        os.unlink(f2path)

>> +        oslink(f1path, f2path)

> 

> I would use f2path = f1path + '.2' assuming f1path has enough random bits, but

> your version, mkstemp+unlink+link, should be more robust for a race condition.


I think I can actually make this more robust. :)

Thanks,
Tony
via Mercurial-devel - Aug. 23, 2016, 5:38 p.m.
> On Aug 23, 2016, at 10:30 AM, Tony Tung <tonytung@instagram.com> wrote:

> 

>> On Aug 23, 2016, at 8:28 AM, Yuya Nishihara <yuya@tcha.org> wrote:

>> 

>>> +    # there's a small race condition that another file can jam itself in between

>>> +    # the time we remove f2 and the time we create the hard link.  in the

>>> +    # unlikely scenario that happens, we'll treat it as nlink being unreliable.

>>>    try:

>> [...]

>>> +        os.unlink(f2path)

>>> +        oslink(f1path, f2path)

>> 

>> I would use f2path = f1path + '.2' assuming f1path has enough random bits, but

>> your version, mkstemp+unlink+link, should be more robust for a race condition.

> 

> I think I can actually make this more robust. :)

> 

> Thanks,

> Tony



… I take that back.  But I’ll fix the cleanup path.

Thanks,
Tony

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1305,34 +1305,33 @@ 
 def checknlink(testfile):
     '''check whether hardlink count reporting works properly'''
 
+    dirpath, filename = os.path.split(testfile)
+
     # testfile may be open, so we need a separate file for checking to
     # work around issue2543 (or testfile may get lost on Samba shares)
-    f1 = testfile + ".hgtmp1"
-    if os.path.lexists(f1):
-        return False
+    f1handle, f1path = tempfile.mkstemp(prefix=filename, dir=dirpath)
+    os.close(f1handle)
+
+    f2handle, f2path = tempfile.mkstemp(prefix=filename, dir=dirpath)
+    os.close(f2handle)
+    f2handle = None
+
+    # there's a small race condition that another file can jam itself in between
+    # the time we remove f2 and the time we create the hard link.  in the
+    # unlikely scenario that happens, we'll treat it as nlink being unreliable.
     try:
-        posixfile(f1, 'w').close()
-    except IOError:
-        try:
-            os.unlink(f1)
-        except OSError:
-            pass
-        return False
-
-    f2 = testfile + ".hgtmp2"
-    fd = None
-    try:
-        oslink(f1, f2)
+        os.unlink(f2path)
+        oslink(f1path, f2path)
         # nlinks() may behave differently for files on Windows shares if
         # the file is open.
-        fd = posixfile(f2)
-        return nlinks(f2) > 1
+        f2handle = posixfile(f2path)
+        return nlinks(f2path) > 1
     except OSError:
         return False
     finally:
-        if fd is not None:
-            fd.close()
-        for f in (f1, f2):
+        if f2handle is not None:
+            f2handle.close()
+        for f in (f1path, f2path):
             try:
                 os.unlink(f)
             except OSError: