[PATCH STABLE] implement nlinks() for _WIN32 in osutil.c (issue1922)
Adrian Buehlmann
adrian at cadifra.com
Sat Jan 22 16:23:26 CST 2011
On 2011-01-22 19:46, Adrian Buehlmann wrote:
> # HG changeset patch
> # User Adrian Buehlmann <adrian at cadifra.com>
> # Date 1295720495 -3600
> # Branch stable
> # Node ID 6441aef24506a94c3e19c6eb81fc0a8d74132214
> # Parent d0e0d3d43e1439d63564ab4dddfe0daa69ae2d86
> implement nlinks() for _WIN32 in osutil.c (issue1922)
>
> If the pywin32 package was not installed, the import of win32 in
> windows.py silently failed and (before this patch) util.nlinks was then
> used as a fallback when running on Windows.
>
> util.nlinks() returned 0 for all files when called on Windows, because
> Python's
>
> os.lstat(name).st_nlink
>
> is 0 on Windows for all files.
>
> If nlinks() returns 0, util.opener failed to break up hardlinks, which
> could lead to repository corruption when committing or pushing to a
> hardlinked clone (hg verify detects it).
>
> We now provide our own C implementation of nlinks() in osutil.c for
> Windows, so we don't depend on the pywin32 package being installed for
> nlinks().
>
> And we force breaking up hardlinks on every append file access in the
> opener if nlinks() returns < 1, thus making sure that we can't cause
> any hardlink repository corruption.
>
> It would have been possible to simply require the pywin32 package on
> Windows and abort with an import error if it's not installed, but such
> a policy change should be avoided on the stable branch. Previous packages
> like for example
>
> mercurial-1.7.3-1.win32-py2.6.exe
>
> didn't make it obvious that pywin32 was needed as a dependency. It just
> silently caused repository corruption in hardlinked clones if pywin32
> was not installed.
>
> (This patch is supposed to completely fix issue1922)
>
> diff --git a/mercurial/osutil.c b/mercurial/osutil.c
> --- a/mercurial/osutil.c
> +++ b/mercurial/osutil.c
> @@ -517,6 +517,53 @@ bail:
> PyMem_Free(name);
> return file_obj;
> }
> +
> +static PyObject *nlinks(PyObject *self, PyObject *args)
> +{
> + char *name = NULL;
> + PyObject *res = NULL;
> + HANDLE fh = INVALID_HANDLE_VALUE;
> + long n = 1000;
> + BY_HANDLE_FILE_INFORMATION fi;
> +
> + if (!PyArg_ParseTuple(args, "et:nlinks",
> + Py_FileSystemDefaultEncoding,
> + &name))
> + return NULL;
> +
> + fh = CreateFile(name, 0,
> + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
> + NULL,
> + OPEN_EXISTING,
> + 0,
> + NULL);
Some remarks here (in case anyone is actually looking at this...):
For comparison, mercurial/win32.py is doing:
fh = win32file.CreateFile(pathname,
win32file.GENERIC_READ, win32file.FILE_SHARE_READ,
None, win32file.OPEN_EXISTING, 0, None)
I changed two things in the CreateFile call:
- I set dwDesiredAccess to 0 (2nd argument) since we don't even need
to read the file contents -- just some metadata about the file.
This is documented in MSDN. GENERIC_READ is overkill.
- I set dwShareMode to also allow other processes to write and delete
(FILE_SHARE_WRITE and FILE_SHARE_DELETE). This is the maximum allowed
for other processes and is the same as we already do with posixfile.
If we only allow FILE_SHARE_READ, e.g. deletes by other processes will
fail for as long as we have the file handle open. Rather short, but
still against the grain of POSIX semantics.
I've tested this with local NTFS on Windows 7, a share on a very old
Samba server and with a share served by Windows 7. Worked as expected.
> + if (fh == INVALID_HANDLE_VALUE) {
> + PyErr_SetFromWindowsErrWithFilename(GetLastError(), name);
> + goto bail;
> + }
> +
> + fi.nNumberOfLinks = 1000;
> + if (!GetFileInformationByHandle(fh, &fi)) {
> + PyErr_SetFromWindowsErrWithFilename(GetLastError(), name);
> + goto bail;
> + }
> +
> + /*
> + nNumberOfLinks is a DWORD, which is a 32 bit unsigned
> + The return value is a Python int, which is signed
> + */
> + if (fi.nNumberOfLinks > (DWORD)PyInt_GetMax())
> + n = PyInt_GetMax();
> + else
> + n = (long)fi.nNumberOfLinks;
> + res = PyInt_FromLong(n);
> +bail:
> + if (fh != INVALID_HANDLE_VALUE)
> + CloseHandle(fh);
> + PyMem_Free(name);
> + return res;
> +}
> #endif
>
> static char osutil_doc[] = "Native operating system services.";
> @@ -528,6 +575,8 @@ static PyMethodDef methods[] = {
> {"posixfile", (PyCFunction)posixfile, METH_VARARGS | METH_KEYWORDS,
> "Open a file with POSIX-like semantics.\n"
> "On error, this function may raise either a WindowsError or an IOError."},
> + {"nlinks", nlinks, METH_VARARGS,
> + "return number of hardlinks for the given file\n"},
> #endif
> {NULL, NULL}
> };
> diff --git a/mercurial/posix.py b/mercurial/posix.py
> --- a/mercurial/posix.py
> +++ b/mercurial/posix.py
> @@ -23,6 +23,10 @@ def openhardlinks():
> '''return true if it is safe to hold open file handles to hardlinks'''
> return True
>
> +def nlinks(name):
> + """return number of hardlinks for the given file"""
> + return os.lstat(name).st_nlink
> +
> def rcfiles(path):
> rcs = [os.path.join(path, 'hgrc')]
> rcdir = os.path.join(path, 'hgrc.d')
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -550,10 +550,6 @@ class path_auditor(object):
> # want to add "foo/bar/baz" before checking if there's a "foo/.hg"
> self.auditeddir.update(prefixes)
>
> -def nlinks(pathname):
> - """Return number of hardlinks for the given file."""
> - return os.lstat(pathname).st_nlink
> -
> if hasattr(os, 'link'):
> os_link = os.link
> else:
> @@ -913,6 +909,8 @@ class opener(object):
> # shares if the file is open.
> fd = open(f)
> nlink = nlinks(f)
> + if nlink < 1:
> + nlink = 2 # force mktempcopy (issue1922)
> fd.close()
> except (OSError, IOError):
> nlink = 0
> diff --git a/mercurial/win32.py b/mercurial/win32.py
> --- a/mercurial/win32.py
> +++ b/mercurial/win32.py
> @@ -41,10 +41,6 @@ def _getfileinfo(pathname):
> finally:
> fh.Close()
>
> -def nlinks(pathname):
> - """Return number of hardlinks for the given file."""
> - return _getfileinfo(pathname)[7]
> -
> def samefile(fpath1, fpath2):
> """Returns whether fpath1 and fpath2 refer to the same file. This is only
> guaranteed to work for files, not directories."""
> diff --git a/mercurial/windows.py b/mercurial/windows.py
> --- a/mercurial/windows.py
> +++ b/mercurial/windows.py
> @@ -20,6 +20,13 @@ def posixfile(name, mode='r', buffering=
> raise IOError(err.errno, '%s: %s' % (name, err.strerror))
> posixfile.__doc__ = osutil.posixfile.__doc__
>
> +def nlinks(name):
> + try:
> + return osutil.nlinks(name)
> + except WindowsError, err:
> + raise OSError(err.errno, '%s: %s' % (name, err.strerror))
> +nlinks.__doc__ = osutil.nlinks.__doc__
> +
> class winstdout(object):
> '''stdout on windows misbehaves if sent through a pipe'''
>
More information about the Mercurial-devel
mailing list