Patchwork [1,of,2] cext: implement osutil.getfstype() on Windows

login
register
mail settings
Submitter Matt Harbison
Date Dec. 28, 2017, 11:31 p.m.
Message ID <452b23148c906aca4765.1514503898@Envy>
Download mbox | patch
Permalink /patch/26483/
State Accepted
Headers show

Comments

Matt Harbison - Dec. 28, 2017, 11:31 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1514442941 18000
#      Thu Dec 28 01:35:41 2017 -0500
# Node ID 452b23148c906aca4765d568263d9e3615cce228
# Parent  95a9be56c3bb614fc1334a4cc8ec669f9eb7bc5a
cext: implement osutil.getfstype() on Windows

This will allow NTFS to be added to the hardlink whitelist, and resume creating
hardlinks in transactions (which was disabled globally in 07a92bbd02e5; see also
e5ce49a30146).  I'll wait until this is accepted before implementing the pure
version.  It isn't clear to me if the API version should be bumped, but it's new
to Windows, so I assume the answer is "yes".  I opted to report "cifs" for
remote volumes because this shows in `hg debugfs`, which also reports that
hardlinks are supported for these volumes.  So being able to distinguish it from
"unknown" seems useful.

The documentation [1] seems to indicate that SMB isn't supported by these
functions, but experimenting shows that mapped drives are reported as "NTFS" on
Windows 7.  I don't have a second Windows machine, but instead shared a temp
directory on C:\.  In this setup, both of the following were detected as 'cifs'
with the explicit GetDriveType() check:

  Z:\repo>hg ci -A

  C:\>hg -R \\hostname\temp\repo ci -A   # (without Z:\ being mapped)

It looks like this is called 6 times to add and commit a single new file, so I'm
a little surprised this isn't cached.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa364993(v=vs.85).aspx
Yuya Nishihara - Dec. 30, 2017, 7:13 a.m.
On Thu, 28 Dec 2017 18:31:38 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1514442941 18000
> #      Thu Dec 28 01:35:41 2017 -0500
> # Node ID 452b23148c906aca4765d568263d9e3615cce228
> # Parent  95a9be56c3bb614fc1334a4cc8ec669f9eb7bc5a
> cext: implement osutil.getfstype() on Windows
> 
> This will allow NTFS to be added to the hardlink whitelist, and resume creating
> hardlinks in transactions (which was disabled globally in 07a92bbd02e5; see also
> e5ce49a30146).  I'll wait until this is accepted before implementing the pure
> version.

Perhaps we don't need a cext version if we'll reimplement it with ctypes.

> +	fullpath = calloc(size, sizeof(fullpath[0]));
> +	volume = calloc(size, sizeof(volume[0]));

Nit: no need to zero-fill?

> +	if (!fullpath || !volume) {
> +		PyErr_SetString(PyExc_MemoryError, "out of memory");
> +		goto bail;

Nit: PyErr_NoMemory() is preferred in our code.

> +bail:
> +	if (fullpath)
> +		free(fullpath);
> +	if (volume)
> +		free(volume);

Nit: free(NULL) is valid.
Matt Harbison - Dec. 30, 2017, 4:19 p.m.
> On Dec 30, 2017, at 2:13 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Thu, 28 Dec 2017 18:31:38 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1514442941 18000
>> #      Thu Dec 28 01:35:41 2017 -0500
>> # Node ID 452b23148c906aca4765d568263d9e3615cce228
>> # Parent  95a9be56c3bb614fc1334a4cc8ec669f9eb7bc5a
>> cext: implement osutil.getfstype() on Windows
>> 
>> This will allow NTFS to be added to the hardlink whitelist, and resume creating
>> hardlinks in transactions (which was disabled globally in 07a92bbd02e5; see also
>> e5ce49a30146).  I'll wait until this is accepted before implementing the pure
>> version.
> 
> Perhaps we don't need a cext version if we'll reimplement it with ctypes.

So move the existing util.getfstype to posix, and then “util.getfstype = platform.getfstype”?  Works for me.  Do you want to drop this from hg-committed?

>> +    fullpath = calloc(size, sizeof(fullpath[0]));
>> +    volume = calloc(size, sizeof(volume[0]));
> 
> Nit: no need to zero-fill?
> 
>> +    if (!fullpath || !volume) {
>> +        PyErr_SetString(PyExc_MemoryError, "out of memory");
>> +        goto bail;
> 
> Nit: PyErr_NoMemory() is preferred in our code.
> 
>> +bail:
>> +    if (fullpath)
>> +        free(fullpath);
>> +    if (volume)
>> +        free(volume);
> 
> Nit: free(NULL) is valid.
Yuya Nishihara - Dec. 31, 2017, 1:49 a.m.
On Sat, 30 Dec 2017 11:19:56 -0500, Matt Harbison wrote:
> > On Dec 30, 2017, at 2:13 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> On Thu, 28 Dec 2017 18:31:38 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1514442941 18000
> >> #      Thu Dec 28 01:35:41 2017 -0500
> >> # Node ID 452b23148c906aca4765d568263d9e3615cce228
> >> # Parent  95a9be56c3bb614fc1334a4cc8ec669f9eb7bc5a
> >> cext: implement osutil.getfstype() on Windows
> >> 
> >> This will allow NTFS to be added to the hardlink whitelist, and resume creating
> >> hardlinks in transactions (which was disabled globally in 07a92bbd02e5; see also
> >> e5ce49a30146).  I'll wait until this is accepted before implementing the pure
> >> version.
> > 
> > Perhaps we don't need a cext version if we'll reimplement it with ctypes.
> 
> So move the existing util.getfstype to posix, and then “util.getfstype = platform.getfstype”?

Yep, that's how win32api interface has been evolved.

>  Do you want to drop this from hg-committed?

Pruned 4be2befbec25 and a210a1a19734.

Patch

diff --git a/mercurial/cext/osutil.c b/mercurial/cext/osutil.c
--- a/mercurial/cext/osutil.c
+++ b/mercurial/cext/osutil.c
@@ -1268,6 +1268,84 @@ 
 	PyMem_Free(name);
 	return file_obj;
 }
+
+static PyObject *getfstype(PyObject *self, PyObject *args)
+/* given a directory path, return filesystem type name (best-effort) */
+{
+	const char *path = NULL;
+	char *fullpath = NULL;
+	char *volume = NULL;
+	DWORD size = 0;
+	char fstype[MAX_PATH + 1];
+
+	if (!PyArg_ParseTuple(args, "s", &path))
+		return NULL;
+
+	size = GetFullPathName(path, 0, NULL, NULL);
+
+	if (size == 0) {
+		PyErr_SetFromWindowsErrWithFilename(GetLastError(), path);
+		return NULL;
+	}
+
+	fullpath = calloc(size, sizeof(fullpath[0]));
+	volume = calloc(size, sizeof(volume[0]));
+
+	if (!fullpath || !volume) {
+		PyErr_SetString(PyExc_MemoryError, "out of memory");
+		goto bail;
+	}
+
+	if (!GetFullPathName(path, size, fullpath, NULL)) {
+		PyErr_SetFromWindowsErrWithFilename(GetLastError(), path);
+		goto bail;
+	}
+
+	if (!GetVolumePathName(fullpath, volume, size)) {
+		PyErr_SetFromWindowsErrWithFilename(GetLastError(), path);
+		goto bail;
+	}
+
+	/* Even though the documentation says SMB doesn't support volume management
+	 * functions, passing an SMB path to GetVolumeInformation() returns 'NTFS'.
+	 * So bail unless this is known to _not_ be a network drive. */
+	switch (GetDriveType(volume)) {
+	case DRIVE_FIXED:
+	case DRIVE_REMOVABLE:
+	case DRIVE_CDROM:
+	case DRIVE_RAMDISK:
+		break;
+
+	case DRIVE_REMOTE:
+		free(fullpath);
+		free(volume);
+		return Py_BuildValue("s", "cifs");
+
+	case DRIVE_UNKNOWN:
+	case DRIVE_NO_ROOT_DIR:
+	default:
+		free(fullpath);
+		free(volume);
+		Py_RETURN_NONE;
+	}
+
+	if (!GetVolumeInformation(volume, NULL, 0, NULL, NULL, NULL, fstype,
+			sizeof(fstype))) {
+		PyErr_SetFromWindowsErrWithFilename(GetLastError(), volume);
+		goto bail;
+	}
+
+	free(fullpath);
+	free(volume);
+	return Py_BuildValue("s", fstype);
+
+bail:
+	if (fullpath)
+		free(fullpath);
+	if (volume)
+		free(volume);
+	return NULL;
+}
 #endif
 
 #ifdef __APPLE__
@@ -1307,13 +1385,13 @@ 
 	{"setprocname", (PyCFunction)setprocname, METH_VARARGS,
 	 "set process title (best-effort)\n"},
 #endif
-#if defined(HAVE_BSD_STATFS) || defined(HAVE_LINUX_STATFS)
+	{"unblocksignal", (PyCFunction)unblocksignal, METH_VARARGS,
+	 "change signal mask to unblock a given signal\n"},
+#endif /* ndef _WIN32 */
+#if defined(HAVE_BSD_STATFS) || defined(HAVE_LINUX_STATFS) || defined(_WIN32)
 	{"getfstype", (PyCFunction)getfstype, METH_VARARGS,
 	 "get filesystem type (best-effort)\n"},
 #endif
-	{"unblocksignal", (PyCFunction)unblocksignal, METH_VARARGS,
-	 "change signal mask to unblock a given signal\n"},
-#endif /* ndef _WIN32 */
 #ifdef __APPLE__
 	{
 		"isgui", (PyCFunction)isgui, METH_NOARGS,
@@ -1323,7 +1401,7 @@ 
 	{NULL, NULL}
 };
 
-static const int version = 2;
+static const int version = 3;
 
 #ifdef IS_PY3K
 static struct PyModuleDef osutil_module = {
diff --git a/mercurial/policy.py b/mercurial/policy.py
--- a/mercurial/policy.py
+++ b/mercurial/policy.py
@@ -74,7 +74,7 @@ 
     (r'cext', r'bdiff'): 1,
     (r'cext', r'diffhelpers'): 1,
     (r'cext', r'mpatch'): 1,
-    (r'cext', r'osutil'): 2,
+    (r'cext', r'osutil'): 3,
     (r'cext', r'parsers'): 4,
 }