Patchwork [5,of,5] statfs: make getfstype() raise OSError

login
register
mail settings
Submitter Yuya Nishihara
Date March 25, 2017, 10:38 a.m.
Message ID <05dc34258f021523bc1d.1490438331@mimosa>
Download mbox | patch
Permalink /patch/19666/
State Accepted
Headers show

Comments

Yuya Nishihara - March 25, 2017, 10:38 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1490430323 -32400
#      Sat Mar 25 17:25:23 2017 +0900
# Node ID 05dc34258f021523bc1de5c403f671fa4d1b9905
# Parent  69a0abd9c61f0dae12d60b4f6fd4ada8288bd451
statfs: make getfstype() raise OSError

It's better for getfstype() function to not suppress an error. Callers can
handle it as necessary. Now "hg debugfsinfo" will report OSError.
Jun Wu - March 26, 2017, 5:53 a.m.
The series makes the CPython code cleaner. Marked as prereviewed.

I was making "getfstype" a pure C function, so it's possible to be reused
directly for non-cpython code (as a side effect, it does not have Python
error handling). The new "describefstype" provides enough support if we want
a pure C implementation later. There won't be much duplicated code. Thanks!

Excerpts from Yuya Nishihara's message of 2017-03-25 19:38:51 +0900:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1490430323 -32400
> #      Sat Mar 25 17:25:23 2017 +0900
> # Node ID 05dc34258f021523bc1de5c403f671fa4d1b9905
> # Parent  69a0abd9c61f0dae12d60b4f6fd4ada8288bd451
> statfs: make getfstype() raise OSError
> 
> It's better for getfstype() function to not suppress an error. Callers can
> handle it as necessary. Now "hg debugfsinfo" will report OSError.
> 
> diff --git a/mercurial/osutil.c b/mercurial/osutil.c
> --- a/mercurial/osutil.c
> +++ b/mercurial/osutil.c
> @@ -1090,7 +1090,7 @@ static PyObject *getfstype(PyObject *sel
>      memset(&buf, 0, sizeof(buf));
>      r = statfs(path, &buf);
>      if (r != 0)
> -        Py_RETURN_NONE;
> +        return PyErr_SetFromErrno(PyExc_OSError);
>      return Py_BuildValue("s", describefstype(&buf));
>  }
>  #endif /* defined(HAVE_LINUX_STATFS) || defined(HAVE_BSD_STATFS) */
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1089,7 +1089,10 @@ def copyfile(src, dest, hardlink=False, 
>      if hardlink:
>          # Hardlinks are problematic on CIFS (issue4546), do not allow hardlinks
>          # unless we are confident that dest is on a whitelisted filesystem.
> -        fstype = getfstype(os.path.dirname(dest))
> +        try:
> +            fstype = getfstype(os.path.dirname(dest))
> +        except OSError:
> +            fstype = None
>          if fstype not in _hardlinkfswhitelist:
>              hardlink = False
>      if hardlink:
> @@ -1372,7 +1375,7 @@ def fspath(name, root):
>  def getfstype(dirpath):
>      '''Get the filesystem type name from a directory (best-effort)
>  
> -    Returns None if we are unsure, or errors like ENOENT, EPERM happen.
> +    Returns None if we are unsure. Raises OSError on ENOENT, EPERM, etc.
>      '''
>      return getattr(osutil, 'getfstype', lambda x: None)(dirpath)
>  
> diff --git a/tests/hghave.py b/tests/hghave.py
> --- a/tests/hghave.py
> +++ b/tests/hghave.py
> @@ -349,7 +349,10 @@ def has_hardlink():
>  @check("hardlink-whitelisted", "hardlinks on whitelisted filesystems")
>  def has_hardlink_whitelisted():
>      from mercurial import util
> -    fstype = util.getfstype('.')
> +    try:
> +        fstype = util.getfstype('.')
> +    except OSError:
> +        return False
>      return fstype in util._hardlinkfswhitelist
>  
>  @check("rmcwd", "can remove current working directory")
Augie Fackler - March 27, 2017, 6:52 p.m.
On Sat, Mar 25, 2017 at 07:38:51PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1490430323 -32400
> #      Sat Mar 25 17:25:23 2017 +0900
> # Node ID 05dc34258f021523bc1de5c403f671fa4d1b9905
> # Parent  69a0abd9c61f0dae12d60b4f6fd4ada8288bd451
> statfs: make getfstype() raise OSError

Queued, thanks

>
> It's better for getfstype() function to not suppress an error. Callers can
> handle it as necessary. Now "hg debugfsinfo" will report OSError.
>
> diff --git a/mercurial/osutil.c b/mercurial/osutil.c
> --- a/mercurial/osutil.c
> +++ b/mercurial/osutil.c
> @@ -1090,7 +1090,7 @@ static PyObject *getfstype(PyObject *sel
>       memset(&buf, 0, sizeof(buf));
>       r = statfs(path, &buf);
>       if (r != 0)
> -		Py_RETURN_NONE;
> +		return PyErr_SetFromErrno(PyExc_OSError);
>       return Py_BuildValue("s", describefstype(&buf));
>  }
>  #endif /* defined(HAVE_LINUX_STATFS) || defined(HAVE_BSD_STATFS) */
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1089,7 +1089,10 @@ def copyfile(src, dest, hardlink=False,
>      if hardlink:
>          # Hardlinks are problematic on CIFS (issue4546), do not allow hardlinks
>          # unless we are confident that dest is on a whitelisted filesystem.
> -        fstype = getfstype(os.path.dirname(dest))
> +        try:
> +            fstype = getfstype(os.path.dirname(dest))
> +        except OSError:
> +            fstype = None
>          if fstype not in _hardlinkfswhitelist:
>              hardlink = False
>      if hardlink:
> @@ -1372,7 +1375,7 @@ def fspath(name, root):
>  def getfstype(dirpath):
>      '''Get the filesystem type name from a directory (best-effort)
>
> -    Returns None if we are unsure, or errors like ENOENT, EPERM happen.
> +    Returns None if we are unsure. Raises OSError on ENOENT, EPERM, etc.
>      '''
>      return getattr(osutil, 'getfstype', lambda x: None)(dirpath)
>
> diff --git a/tests/hghave.py b/tests/hghave.py
> --- a/tests/hghave.py
> +++ b/tests/hghave.py
> @@ -349,7 +349,10 @@ def has_hardlink():
>  @check("hardlink-whitelisted", "hardlinks on whitelisted filesystems")
>  def has_hardlink_whitelisted():
>      from mercurial import util
> -    fstype = util.getfstype('.')
> +    try:
> +        fstype = util.getfstype('.')
> +    except OSError:
> +        return False
>      return fstype in util._hardlinkfswhitelist
>
>  @check("rmcwd", "can remove current working directory")
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/osutil.c b/mercurial/osutil.c
--- a/mercurial/osutil.c
+++ b/mercurial/osutil.c
@@ -1090,7 +1090,7 @@  static PyObject *getfstype(PyObject *sel
 	memset(&buf, 0, sizeof(buf));
 	r = statfs(path, &buf);
 	if (r != 0)
-		Py_RETURN_NONE;
+		return PyErr_SetFromErrno(PyExc_OSError);
 	return Py_BuildValue("s", describefstype(&buf));
 }
 #endif /* defined(HAVE_LINUX_STATFS) || defined(HAVE_BSD_STATFS) */
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1089,7 +1089,10 @@  def copyfile(src, dest, hardlink=False, 
     if hardlink:
         # Hardlinks are problematic on CIFS (issue4546), do not allow hardlinks
         # unless we are confident that dest is on a whitelisted filesystem.
-        fstype = getfstype(os.path.dirname(dest))
+        try:
+            fstype = getfstype(os.path.dirname(dest))
+        except OSError:
+            fstype = None
         if fstype not in _hardlinkfswhitelist:
             hardlink = False
     if hardlink:
@@ -1372,7 +1375,7 @@  def fspath(name, root):
 def getfstype(dirpath):
     '''Get the filesystem type name from a directory (best-effort)
 
-    Returns None if we are unsure, or errors like ENOENT, EPERM happen.
+    Returns None if we are unsure. Raises OSError on ENOENT, EPERM, etc.
     '''
     return getattr(osutil, 'getfstype', lambda x: None)(dirpath)
 
diff --git a/tests/hghave.py b/tests/hghave.py
--- a/tests/hghave.py
+++ b/tests/hghave.py
@@ -349,7 +349,10 @@  def has_hardlink():
 @check("hardlink-whitelisted", "hardlinks on whitelisted filesystems")
 def has_hardlink_whitelisted():
     from mercurial import util
-    fstype = util.getfstype('.')
+    try:
+        fstype = util.getfstype('.')
+    except OSError:
+        return False
     return fstype in util._hardlinkfswhitelist
 
 @check("rmcwd", "can remove current working directory")