[PATCH 0/5] libbtrfsutil: misc fixes

[PATCH 0/5] libbtrfsutil: misc fixes

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Hi, Dave,

This is a handful of minor fixes for libbtrfsutil that would be good to
get in before the release. Patch 1 fixes an issue reported by Tomohiro a
few weeks back. Patch 2 fixes a memory leak I noticed while writing
patch 1. Patch 3 makes us use the local mkfs.btrfs so that the tests can
run on the CI system. Patch 4 fixes an issue I noticed while testing
that fix. Patch 5 fixes a test failure caused by "btrfs-progs: mkfs: add
uuid and otime to ROOT_ITEM of, FS_TREE".

Based on your devel branch. Please apply!

Omar Sandoval (5):
  libbtrfsutil: don't return free space cache inodes from
    deleted_subvolumes()
  libbtrfsutil: fix memory leak in deleted_subvolumes()
  libbtrfsutil: use local mkfs.btrfs for tests if it exists
  libbtrfsutil: always build libbtrfsutil.so.$MAJOR
  libbtrfsutil: fix test assumptions about top-level subvolume

 Makefile                                    |  4 ++--
 libbtrfsutil/python/tests/__init__.py       |  6 ++++-
 libbtrfsutil/python/tests/test_subvolume.py |  8 ++++---
 libbtrfsutil/subvolume.c                    | 34 +++++++++++++++++++----------
 4 files changed, 35 insertions(+), 17 deletions(-)

-- 
2.16.3

[PATCH 1/5] libbtrfsutil: don't return free space cache inodes from deleted_subvolumes()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Deleted free space cache inodes also get an orphan item in the root
tree, but we shouldn't report those as deleted subvolumes. Deleted
subvolumes will still have the root item, so we can just do an extra
tree search.

Reported-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/subvolume.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index d9728281..d6c0ced8 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -1339,21 +1339,31 @@ PUBLIC enum btrfs_util_error btrfs_util_deleted_subvolumes_fd(int fd,
 		}
 
 		header = (struct btrfs_ioctl_search_header *)(search.buf + buf_off);
-		if (*n >= capacity) {
-			size_t new_capacity = capacity ? capacity * 2 : 1;
-			uint64_t *new_ids;
 
-			new_ids = reallocarray(*ids, new_capacity,
-					       sizeof(**ids));
-			if (!new_ids)
-				return BTRFS_UTIL_ERROR_NO_MEMORY;
-
-			*ids = new_ids;
-			capacity = new_capacity;
+		/*
+		 * The orphan item might be for a free space cache inode, so
+		 * check if there's a matching root item.
+		 */
+		err = btrfs_util_subvolume_info_fd(fd, header->offset, NULL);
+		if (!err) {
+			if (*n >= capacity) {
+				size_t new_capacity;
+				uint64_t *new_ids;
+
+				new_capacity = capacity ? capacity * 2 : 1;
+				new_ids = reallocarray(*ids, new_capacity,
+						       sizeof(**ids));
+				if (!new_ids)
+					return BTRFS_UTIL_ERROR_NO_MEMORY;
+
+				*ids = new_ids;
+				capacity = new_capacity;
+			}
+			(*ids)[(*n)++] = header->offset;
+		} else if (err != BTRFS_UTIL_ERROR_SUBVOLUME_NOT_FOUND) {
+			goto out;
 		}
 
-		(*ids)[(*n)++] = header->offset;
-
 		items_pos++;
 		buf_off += sizeof(*header) + header->len;
 		search.key.min_offset = header->offset + 1;
-- 
2.16.3

[PATCH 2/5] libbtrfsutil: fix memory leak in deleted_subvolumes()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

If we fail to reallocate the ID array, we still need to free it.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/subvolume.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
index d6c0ced8..867b3e10 100644
--- a/libbtrfsutil/subvolume.c
+++ b/libbtrfsutil/subvolume.c
@@ -1353,8 +1353,10 @@ PUBLIC enum btrfs_util_error btrfs_util_deleted_subvolumes_fd(int fd,
 				new_capacity = capacity ? capacity * 2 : 1;
 				new_ids = reallocarray(*ids, new_capacity,
 						       sizeof(**ids));
-				if (!new_ids)
-					return BTRFS_UTIL_ERROR_NO_MEMORY;
+				if (!new_ids) {
+					err = BTRFS_UTIL_ERROR_NO_MEMORY;
+					goto out;
+				}
 
 				*ids = new_ids;
 				capacity = new_capacity;
-- 
2.16.3

[PATCH 3/5] libbtrfsutil: use local mkfs.btrfs for tests if it exists

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The system might not have mkfs installed at all.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 Makefile                              | 2 +-
 libbtrfsutil/python/tests/__init__.py | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 8afc8f6e..699f864d 100644
--- a/Makefile
+++ b/Makefile
@@ -357,7 +357,7 @@ testsuite: btrfs-corrupt-block fssum
 	$(Q)cd tests && ./export-testsuite.sh
 
 ifeq ($(PYTHON_BINDINGS),1)
-test-libbtrfsutil: libbtrfsutil_python
+test-libbtrfsutil: libbtrfsutil_python mkfs.btrfs
 	$(Q)cd libbtrfsutil/python; \
 		LD_LIBRARY_PATH=../.. $(PYTHON) -m unittest discover -v tests
 
diff --git a/libbtrfsutil/python/tests/__init__.py b/libbtrfsutil/python/tests/__init__.py
index d2c6ff28..35550e0a 100644
--- a/libbtrfsutil/python/tests/__init__.py
+++ b/libbtrfsutil/python/tests/__init__.py
@@ -37,8 +37,12 @@ class BtrfsTestCase(unittest.TestCase):
             os.rmdir(self.mountpoint)
             raise e
 
+        if os.path.exists('../../mkfs.btrfs'):
+            mkfs = '../../mkfs.btrfs'
+        else:
+            mkfs = 'mkfs.btrfs'
         try:
-            subprocess.check_call(['mkfs.btrfs', '-q', self.image])
+            subprocess.check_call([mkfs, '-q', self.image])
             subprocess.check_call(['mount', '-o', 'loop', '--', self.image, self.mountpoint])
         except Exception as e:
             os.remove(self.image)
-- 
2.16.3

[PATCH 4/5] libbtrfsutil: always build libbtrfsutil.so.$MAJOR

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Otherwise, make test-libbtrfsutil from a fresh checkout fails.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 699f864d..fbd6677a 100644
--- a/Makefile
+++ b/Makefile
@@ -417,7 +417,7 @@ libbtrfsutil.so.$(libbtrfsutil_major) libbtrfsutil.so: libbtrfsutil.so.$(libbtrf
 	$(Q)$(LN_S) -f $< $@
 
 ifeq ($(PYTHON_BINDINGS),1)
-libbtrfsutil_python: libbtrfsutil.so libbtrfsutil/btrfsutil.h
+libbtrfsutil_python: libbtrfsutil.so.$(libbtrfsutil_major) libbtrfsutil.so libbtrfsutil/btrfsutil.h
 	@echo "    [PY]     libbtrfsutil"
 	$(Q)cd libbtrfsutil/python; \
 		CFLAGS= LDFLAGS= $(PYTHON) setup.py $(SETUP_PY_Q) build_ext -i build
-- 
2.16.3

[PATCH 5/5] libbtrfsutil: fix test assumptions about top-level subvolume

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Since "btrfs-progs: mkfs: add uuid and otime to ROOT_ITEM of, FS_TREE",
the top-level subvolume has a non-zero UUID, ctime, and otime. Fix the
subvolume_info() test to not check for zero.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/python/tests/test_subvolume.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
index a46d4a34..93396cba 100644
--- a/libbtrfsutil/python/tests/test_subvolume.py
+++ b/libbtrfsutil/python/tests/test_subvolume.py
@@ -95,7 +95,8 @@ class TestSubvolume(BtrfsTestCase):
                 self.assertEqual(info.parent_id, 0)
                 self.assertEqual(info.dir_id, 0)
                 self.assertEqual(info.flags, 0)
-                self.assertEqual(info.uuid, bytes(16))
+                self.assertIsInstance(info.uuid, bytes)
+                self.assertEqual(len(info.uuid), 16)
                 self.assertEqual(info.parent_uuid, bytes(16))
                 self.assertEqual(info.received_uuid, bytes(16))
                 self.assertNotEqual(info.generation, 0)
@@ -103,8 +104,8 @@ class TestSubvolume(BtrfsTestCase):
                 self.assertEqual(info.otransid, 0)
                 self.assertEqual(info.stransid, 0)
                 self.assertEqual(info.rtransid, 0)
-                self.assertEqual(info.ctime, 0)
-                self.assertEqual(info.otime, 0)
+                self.assertIsInstance(info.ctime, float)
+                self.assertIsInstance(info.otime, float)
                 self.assertEqual(info.stime, 0)
                 self.assertEqual(info.rtime, 0)
 
@@ -117,6 +118,7 @@ class TestSubvolume(BtrfsTestCase):
         self.assertEqual(info.dir_id, 256)
         self.assertEqual(info.flags, 0)
         self.assertIsInstance(info.uuid, bytes)
+        self.assertEqual(len(info.uuid), 16)
         self.assertEqual(info.parent_uuid, bytes(16))
         self.assertEqual(info.received_uuid, bytes(16))
         self.assertNotEqual(info.generation, 0)
-- 
2.16.3

Re: [PATCH 5/5] libbtrfsutil: fix test assumptions about top-level subvolume

[Misono Tomohiro]

On 2018/03/29 16:53, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Since "btrfs-progs: mkfs: add uuid and otime to ROOT_ITEM of, FS_TREE",
> the top-level subvolume has a non-zero UUID, ctime, and otime. Fix the
> subvolume_info() test to not check for zero.

Sorry, I didn't notice this.

I checked this works in devel branch:
Reviewed-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>

> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  libbtrfsutil/python/tests/test_subvolume.py | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
> index a46d4a34..93396cba 100644
> --- a/libbtrfsutil/python/tests/test_subvolume.py
> +++ b/libbtrfsutil/python/tests/test_subvolume.py
> @@ -95,7 +95,8 @@ class TestSubvolume(BtrfsTestCase):
>                  self.assertEqual(info.parent_id, 0)
>                  self.assertEqual(info.dir_id, 0)
>                  self.assertEqual(info.flags, 0)
> -                self.assertEqual(info.uuid, bytes(16))
> +                self.assertIsInstance(info.uuid, bytes)
> +                self.assertEqual(len(info.uuid), 16)
>                  self.assertEqual(info.parent_uuid, bytes(16))
>                  self.assertEqual(info.received_uuid, bytes(16))
>                  self.assertNotEqual(info.generation, 0)
> @@ -103,8 +104,8 @@ class TestSubvolume(BtrfsTestCase):
>                  self.assertEqual(info.otransid, 0)
>                  self.assertEqual(info.stransid, 0)
>                  self.assertEqual(info.rtransid, 0)
> -                self.assertEqual(info.ctime, 0)
> -                self.assertEqual(info.otime, 0)
> +                self.assertIsInstance(info.ctime, float)
> +                self.assertIsInstance(info.otime, float)>                  self.assertEqual(info.stime, 0)
>                  self.assertEqual(info.rtime, 0)
>  
> @@ -117,6 +118,7 @@ class TestSubvolume(BtrfsTestCase):
>          self.assertEqual(info.dir_id, 256)
>          self.assertEqual(info.flags, 0)
>          self.assertIsInstance(info.uuid, bytes)
> +        self.assertEqual(len(info.uuid), 16)
>          self.assertEqual(info.parent_uuid, bytes(16))
>          self.assertEqual(info.received_uuid, bytes(16))
>          self.assertNotEqual(info.generation, 0)
> 

Re: [PATCH 0/5] libbtrfsutil: misc fixes

[David Sterba]

On Thu, Mar 29, 2018 at 12:53:52AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> This is a handful of minor fixes for libbtrfsutil that would be good to
> get in before the release. Patch 1 fixes an issue reported by Tomohiro a
> few weeks back. Patch 2 fixes a memory leak I noticed while writing
> patch 1. Patch 3 makes us use the local mkfs.btrfs so that the tests can
> run on the CI system. Patch 4 fixes an issue I noticed while testing
> that fix. Patch 5 fixes a test failure caused by "btrfs-progs: mkfs: add
> uuid and otime to ROOT_ITEM of, FS_TREE".
> 
> Based on your devel branch. Please apply!

Done, thanks.