[PATCH] libbtrfsutil: don't close fd on error in btrfs_util_subvolume_id_fd()

[PATCH] libbtrfsutil: don't close fd on error in btrfs_util_subvolume_id_fd()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The caller owns the fd passed to btrfs_util_subvolume_id_fd(), so we
shouldn't close it on error. Fix it, add a regression test, and bump the
library patch version.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
Based on the devel branch.

 libbtrfsutil/btrfsutil.h                    |  2 +-
 libbtrfsutil/python/tests/test_subvolume.py | 12 ++++++++++++
 libbtrfsutil/subvolume.c                    |  4 +---
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/libbtrfsutil/btrfsutil.h b/libbtrfsutil/btrfsutil.h
index ad4f043e..0442af6e 100644
--- a/libbtrfsutil/btrfsutil.h
+++ b/libbtrfsutil/btrfsutil.h
@@ -27,7 +27,7 @@
 
 #define BTRFS_UTIL_VERSION_MAJOR 1
 #define BTRFS_UTIL_VERSION_MINOR 1
-#define BTRFS_UTIL_VERSION_PATCH 0
+#define BTRFS_UTIL_VERSION_PATCH 1
 
 #ifdef __cplusplus
 extern "C" {
diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
index b06a1d3d..61055f53 100644
--- a/libbtrfsutil/python/tests/test_subvolume.py
+++ b/libbtrfsutil/python/tests/test_subvolume.py
@@ -64,6 +64,18 @@ class TestSubvolume(BtrfsTestCase):
             with self.subTest(type=type(arg)):
                 self.assertEqual(btrfsutil.subvolume_id(arg), 5)
 
+    def test_subvolume_id_error(self):
+        fd = os.open('/dev/null', os.O_RDONLY)
+        try:
+            btrfsutil.subvolume_id(fd)
+        except Exception:
+            pass
+        finally:
+            # btrfs_util_subvolume_id_fd() had a bug that would erroneously
+            # close the provided file descriptor. In that case, this will fail
+            # with EBADF.
+            os.close(fd)
+
     def test_subvolume_path(self):
         btrfsutil.create_subvolume(os.path.join(self.mountpoint, 'subvol1'))
         os.mkdir(os.path.join(self.mountpoint, 'dir1'))
diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index 60ab9f9d..f794868f 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -122,10 +122,8 @@ PUBLIC enum btrfs_util_error btrfs_util_subvolume_id_fd(int fd,
 	int ret;
 
 	ret = ioctl(fd, BTRFS_IOC_INO_LOOKUP, &args);
-	if (ret == -1) {
-		close(fd);
+	if (ret == -1)
 		return BTRFS_UTIL_ERROR_INO_LOOKUP_FAILED;
-	}
 
 	*id_ret = args.treeid;
 
-- 
2.21.0

Re: [PATCH] libbtrfsutil: don't close fd on error in btrfs_util_subvolume_id_fd()

[David Sterba]

On Mon, Mar 25, 2019 at 11:17:56AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> The caller owns the fd passed to btrfs_util_subvolume_id_fd(), so we
> shouldn't close it on error. Fix it, add a regression test, and bump the
> library patch version.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Applied, thanks.