| 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
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.
> 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
> 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: