[11/26] libbtrfsutil: add btrfs_util_create_snapshot()

Message ID	ffe3cf7d259065bbce8e54b7a5bae5528faca3d0.1516991902.git.osandov@fb.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-btrfs-owner@kernel.org> From: Omar Sandoval <osandov@osandov.com> To: linux-btrfs@vger.kernel.org Cc: kernel-team@fb.com Subject: [PATCH 11/26] libbtrfsutil: add btrfs_util_create_snapshot() Date: Fri, 26 Jan 2018 10:40:59 -0800 Message-Id: <ffe3cf7d259065bbce8e54b7a5bae5528faca3d0.1516991902.git.osandov@fb.com> In-Reply-To: <cover.1516991902.git.osandov@fb.com> References: <cover.1516991902.git.osandov@fb.com> In-Reply-To: <cover.1516991902.git.osandov@fb.com> References: <cover.1516991902.git.osandov@fb.com> Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk

Omar Sandoval Jan. 26, 2018, 6:40 p.m. UTC

From: Omar Sandoval <osandov@fb.com>

Thanks to subvolume iterators, we can also implement recursive snapshot
fairly easily.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 libbtrfsutil/btrfsutil.h                    |  55 +++++++++
 libbtrfsutil/python/btrfsutilpy.h           |   1 +
 libbtrfsutil/python/module.c                |  11 ++
 libbtrfsutil/python/subvolume.c             |  49 ++++++++
 libbtrfsutil/python/tests/test_qgroup.py    |  10 ++
 libbtrfsutil/python/tests/test_subvolume.py |  55 +++++++++
 libbtrfsutil/subvolume.c                    | 166 ++++++++++++++++++++++++++++
 7 files changed, 347 insertions(+)

Goffredo Baroncelli Jan. 26, 2018, 7:31 p.m. UTC | #1

On 01/26/2018 07:40 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>


Hi,

this is a great work; only few comments:
1) I found not intuitive the naming of the function: i.e. you have 

btrfs_util_create_snapshot()
btrfs_util_f_create_snapshot()

To me it seems more clear to have

btrfs_util_create_snapshot()
btrfs_util_create_snapshot_f()

I think that it is better move the 'f' at the end: at the begin you have the library "btrfs_util", in the middle you have the library function 'create_snapshot', at the end there is the function variant ('f', because it uses a file descriptor).

This is my opinion, even tough there are both examples like you (stat/fstat/lstat) and like my one (capt_get_fd/cap_get_file)...


2) I find the prefix 'btrfs_util_' a bit verbose. Why not a simple 'btrfs_', even at the cost of a possible renaming of the conflicting function in the current btrfs code.

3) regarding the btrfs_util_create_snapshot() function, I think that it would be useful to add some more information:
a) if used recursive is NOT atomic
b) if used recursive, root capabilities are needed

The same for the other functions: mark with a 'root required' tag all the functions which require the root capabilities.

BR
G.Baroncelli



> 
> Thanks to subvolume iterators, we can also implement recursive snapshot
> fairly easily.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  libbtrfsutil/btrfsutil.h                    |  55 +++++++++
>  libbtrfsutil/python/btrfsutilpy.h           |   1 +
>  libbtrfsutil/python/module.c                |  11 ++
>  libbtrfsutil/python/subvolume.c             |  49 ++++++++
>  libbtrfsutil/python/tests/test_qgroup.py    |  10 ++
>  libbtrfsutil/python/tests/test_subvolume.py |  55 +++++++++
>  libbtrfsutil/subvolume.c                    | 166 ++++++++++++++++++++++++++++
>  7 files changed, 347 insertions(+)
> 
> diff --git a/libbtrfsutil/btrfsutil.h b/libbtrfsutil/btrfsutil.h
> index 3f78a34d..829f72b2 100644
> --- a/libbtrfsutil/btrfsutil.h
> +++ b/libbtrfsutil/btrfsutil.h
> @@ -332,6 +332,61 @@ enum btrfs_util_error btrfs_util_f_create_subvolume(int parent_fd,
>  						    uint64_t *async_transid,
>  						    struct btrfs_util_qgroup_inherit *qgroup_inherit);
>  
> +/**
> + * BTRFS_UTIL_CREATE_SNAPSHOT_RECURSIVE - Also snapshot subvolumes beneath the
> + * source subvolume onto the same location on the new snapshot. Note that this
> + * modifies the newly-created snapshot, so it cannot be combined with
> + * %BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY.
> + */
> +#define BTRFS_UTIL_CREATE_SNAPSHOT_RECURSIVE (1 << 0)
> +/**
> + * BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY - Create a read-only snapshot.
> + */
> +#define BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY (1 << 1)
> +#define BTRFS_UTIL_CREATE_SNAPSHOT_MASK ((1 << 2) - 1)
> +
> +/**
> + * btrfs_util_create_snapshot() - Create a new snapshot from a source subvolume
> + * path.
> + * @source: Path of the existing subvolume to snapshot.
> + * @path: Where to create the snapshot.
> + * @flags: Bitmask of BTRFS_UTIL_CREATE_SNAPSHOT_* flags.
> + * @async_transid: See btrfs_util_create_subvolume(). If
> + * %BTRFS_UTIL_CREATE_SNAPSHOT_RECURSIVE was in @flags, then this will contain
> + * the largest transaction ID of all created subvolumes.
> + * @qgroup_inherit: See btrfs_util_create_subvolume().
> + *
> + * Return: %BTRFS_UTIL_OK on success, non-zero error code on failure.
> + */
> +enum btrfs_util_error btrfs_util_create_snapshot(const char *source,
> +						 const char *path, int flags,
> +						 uint64_t *async_transid,
> +						 struct btrfs_util_qgroup_inherit *qgroup_inherit);
> +
> +/**
> + * btrfs_util_f_create_snapshot() - See btrfs_util_create_snapshot().
> + */
> +enum btrfs_util_error btrfs_util_f_create_snapshot(int fd, const char *path,
> +						   int flags,
> +						   uint64_t *async_transid,
> +						   struct btrfs_util_qgroup_inherit *qgroup_inherit);
> +
> +/**
> + * btrfs_util_f2_create_snapshot() - Create a new snapshot from a source
> + * subvolume file descriptor and a target parent file descriptor and name.
> + * @fd: File descriptor of the existing subvolume to snapshot.
> + * @parent_fd: File descriptor of the parent directory where the snapshot should
> + * be created.
> + * @name: Name of the snapshot to create.
> + * @flags: See btrfs_util_create_snapshot().
> + * @async_transid: See btrfs_util_create_snapshot().
> + * @qgroup_inherit: See btrfs_util_create_snapshot().
> + */
> +enum btrfs_util_error btrfs_util_f2_create_snapshot(int fd, int parent_fd,
> +						    const char *name, int flags,
> +						    uint64_t *async_transid,
> +						    struct btrfs_util_qgroup_inherit *qgroup_inherit);
> +
>  struct btrfs_util_subvolume_iterator;
>  
>  /**
> diff --git a/libbtrfsutil/python/btrfsutilpy.h b/libbtrfsutil/python/btrfsutilpy.h
> index a9c15219..d552e416 100644
> --- a/libbtrfsutil/python/btrfsutilpy.h
> +++ b/libbtrfsutil/python/btrfsutilpy.h
> @@ -70,6 +70,7 @@ PyObject *set_subvolume_read_only(PyObject *self, PyObject *args, PyObject *kwds
>  PyObject *get_default_subvolume(PyObject *self, PyObject *args, PyObject *kwds);
>  PyObject *set_default_subvolume(PyObject *self, PyObject *args, PyObject *kwds);
>  PyObject *create_subvolume(PyObject *self, PyObject *args, PyObject *kwds);
> +PyObject *create_snapshot(PyObject *self, PyObject *args, PyObject *kwds);
>  
>  void add_module_constants(PyObject *m);
>  
> diff --git a/libbtrfsutil/python/module.c b/libbtrfsutil/python/module.c
> index daf0747f..d8f797cb 100644
> --- a/libbtrfsutil/python/module.c
> +++ b/libbtrfsutil/python/module.c
> @@ -195,6 +195,17 @@ static PyMethodDef btrfsutil_methods[] = {
>  	 "path -- string, bytes, or path-like object\n"
>  	 "async -- create the subvolume without waiting for it to commit to\n"
>  	 "disk and return the transaction ID"},
> +	{"create_snapshot", (PyCFunction)create_snapshot,
> +	 METH_VARARGS | METH_KEYWORDS,
> +	 "create_snapshot(source, path, recursive=False, read_only=False, async=False)\n\n"
> +	 "Create a new snapshot.\n\n"
> +	 "Arguments:\n"
> +	 "source -- string, bytes, path-like object, or open file descriptor\n"
> +	 "path -- string, bytes, or path-like object\n"
> +	 "recursive -- also snapshot child subvolumes\n"
> +	 "read_only -- create a read-only snapshot\n"
> +	 "async -- create the subvolume without waiting for it to commit to\n"
> +	 "disk and return the transaction ID"},
>  	{},
>  };
>  
> diff --git a/libbtrfsutil/python/subvolume.c b/libbtrfsutil/python/subvolume.c
> index 23163176..763a06d9 100644
> --- a/libbtrfsutil/python/subvolume.c
> +++ b/libbtrfsutil/python/subvolume.c
> @@ -347,6 +347,55 @@ PyObject *create_subvolume(PyObject *self, PyObject *args, PyObject *kwds)
>  		Py_RETURN_NONE;
>  }
>  
> +PyObject *create_snapshot(PyObject *self, PyObject *args, PyObject *kwds)
> +{
> +	static char *keywords[] = {
> +		"source", "path", "recursive", "read_only", "async",
> +		"qgroup_inherit", NULL,
> +	};
> +	struct path_arg src = {.allow_fd = true}, dst = {.allow_fd = false};
> +	enum btrfs_util_error err;
> +	int recursive = 0, read_only = 0, async = 0;
> +	int flags = 0;
> +	QgroupInherit *inherit = NULL;
> +	uint64_t transid;
> +
> +	if (!PyArg_ParseTupleAndKeywords(args, kwds, "O&O&|pppO!:create_snapshot",
> +					 keywords, &path_converter, &src,
> +					 &path_converter, &dst, &recursive,
> +					 &read_only, &async,
> +					 &QgroupInherit_type, &inherit))
> +		return NULL;
> +
> +	if (recursive)
> +		flags |= BTRFS_UTIL_CREATE_SNAPSHOT_RECURSIVE;
> +	if (read_only)
> +		flags |= BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY;
> +
> +	if (src.path) {
> +		err = btrfs_util_create_snapshot(src.path, dst.path, flags,
> +						 async ? &transid : NULL,
> +						 inherit ? inherit->inherit : NULL);
> +	} else {
> +		err = btrfs_util_f_create_snapshot(src.fd, dst.path, flags,
> +						   async ? &transid : NULL,
> +						   inherit ? inherit->inherit : NULL);
> +	}
> +	if (err) {
> +		SetFromBtrfsUtilErrorWithPaths(err, &src, &dst);
> +		path_cleanup(&src);
> +		path_cleanup(&dst);
> +		return NULL;
> +	}
> +
> +	path_cleanup(&src);
> +	path_cleanup(&dst);
> +	if (async)
> +		return PyLong_FromUnsignedLongLong(transid);
> +	else
> +		Py_RETURN_NONE;
> +}
> +
>  typedef struct {
>  	PyObject_HEAD
>  	struct btrfs_util_subvolume_iterator *iter;
> diff --git a/libbtrfsutil/python/tests/test_qgroup.py b/libbtrfsutil/python/tests/test_qgroup.py
> index 19e6b05a..74fc46b6 100644
> --- a/libbtrfsutil/python/tests/test_qgroup.py
> +++ b/libbtrfsutil/python/tests/test_qgroup.py
> @@ -31,6 +31,16 @@ class TestQgroup(BtrfsTestCase):
>  
>          btrfsutil.create_subvolume(subvol, qgroup_inherit=inherit)
>  
> +    def test_snapshot_inherit(self):
> +        subvol = os.path.join(self.mountpoint, 'subvol')
> +        snapshot = os.path.join(self.mountpoint, 'snapshot')
> +
> +        inherit = btrfsutil.QgroupInherit()
> +        inherit.add_group(5)
> +
> +        btrfsutil.create_subvolume(subvol)
> +        btrfsutil.create_snapshot(subvol, snapshot, qgroup_inherit=inherit)
> +
>  
>  class TestQgroupInherit(unittest.TestCase):
>      def test_new(self):
> diff --git a/libbtrfsutil/python/tests/test_subvolume.py b/libbtrfsutil/python/tests/test_subvolume.py
> index b43beca7..2951154e 100644
> --- a/libbtrfsutil/python/tests/test_subvolume.py
> +++ b/libbtrfsutil/python/tests/test_subvolume.py
> @@ -129,6 +129,13 @@ class TestSubvolume(BtrfsTestCase):
>          self.assertEqual(info.stime, 0)
>          self.assertEqual(info.rtime, 0)
>  
> +        subvol_uuid = info.uuid
> +        snapshot = os.path.join(self.mountpoint, 'snapshot')
> +        btrfsutil.create_snapshot(subvol, snapshot)
> +
> +        info = btrfsutil.subvolume_info(snapshot)
> +        self.assertEqual(info.parent_uuid, subvol_uuid)
> +
>          # TODO: test received_uuid, stransid, rtransid, stime, and rtime
>  
>          for arg in self.path_or_fd(self.mountpoint):
> @@ -215,6 +222,54 @@ class TestSubvolume(BtrfsTestCase):
>          self.assertTrue(os.WIFEXITED(wstatus))
>          self.assertEqual(os.WEXITSTATUS(wstatus), 0)
>  
> +    def test_create_snapshot(self):
> +        subvol = os.path.join(self.mountpoint, 'subvol')
> +
> +        btrfsutil.create_subvolume(subvol)
> +        os.mkdir(os.path.join(subvol, 'dir'))
> +
> +        for i, arg in enumerate(self.path_or_fd(subvol)):
> +            with self.subTest(type=type(arg)):
> +                snapshots_dir = os.path.join(self.mountpoint, 'snapshots{}'.format(i))
> +                os.mkdir(snapshots_dir)
> +                snapshot = os.path.join(snapshots_dir, 'snapshot')
> +
> +                btrfsutil.create_snapshot(subvol, snapshot + '1')
> +                self.assertTrue(btrfsutil.is_subvolume(snapshot + '1'))
> +                self.assertTrue(os.path.exists(os.path.join(snapshot + '1', 'dir')))
> +
> +                btrfsutil.create_snapshot(subvol, (snapshot + '2').encode())
> +                self.assertTrue(btrfsutil.is_subvolume(snapshot + '2'))
> +                self.assertTrue(os.path.exists(os.path.join(snapshot + '2', 'dir')))
> +
> +                if HAVE_PATH_LIKE:
> +                    btrfsutil.create_snapshot(subvol, PurePath(snapshot + '3'))
> +                    self.assertTrue(btrfsutil.is_subvolume(snapshot + '3'))
> +                    self.assertTrue(os.path.exists(os.path.join(snapshot + '3', 'dir')))
> +
> +        nested_subvol = os.path.join(subvol, 'nested')
> +        more_nested_subvol = os.path.join(nested_subvol, 'more_nested')
> +        btrfsutil.create_subvolume(nested_subvol)
> +        btrfsutil.create_subvolume(more_nested_subvol)
> +        os.mkdir(os.path.join(more_nested_subvol, 'nested_dir'))
> +
> +        snapshot = os.path.join(self.mountpoint, 'snapshot')
> +
> +        btrfsutil.create_snapshot(subvol, snapshot + '1')
> +        # Dummy subvolume.
> +        self.assertEqual(os.stat(os.path.join(snapshot + '1', 'nested')).st_ino, 2)
> +        self.assertFalse(os.path.exists(os.path.join(snapshot + '1', 'nested', 'more_nested')))
> +
> +        btrfsutil.create_snapshot(subvol, snapshot + '2', recursive=True)
> +        self.assertTrue(os.path.exists(os.path.join(snapshot + '2', 'nested/more_nested/nested_dir')))
> +
> +        transid = btrfsutil.create_snapshot(subvol, snapshot + '3', recursive=True, async=True)
> +        self.assertTrue(os.path.exists(os.path.join(snapshot + '3', 'nested/more_nested/nested_dir')))
> +        self.assertGreater(transid, 0)
> +
> +        btrfsutil.create_snapshot(subvol, snapshot + '4', read_only=True)
> +        self.assertTrue(btrfsutil.get_subvolume_read_only(snapshot + '4'))
> +
>      def test_subvolume_iterator(self):
>          pwd = os.getcwd()
>          try:
> diff --git a/libbtrfsutil/subvolume.c b/libbtrfsutil/subvolume.c
> index 2fb4e59f..27f64657 100644
> --- a/libbtrfsutil/subvolume.c
> +++ b/libbtrfsutil/subvolume.c
> @@ -856,6 +856,172 @@ out_iter:
>  	return err;
>  }
>  
> +static enum btrfs_util_error snapshot_subvolume_children(int fd, int parent_fd,
> +							 const char *name,
> +							 uint64_t *async_transid)
> +{
> +	struct btrfs_util_subvolume_iterator *iter;
> +	enum btrfs_util_error err;
> +	int dstfd;
> +
> +	dstfd = openat(parent_fd, name, O_RDONLY);
> +	if (dstfd == -1)
> +		return BTRFS_UTIL_ERROR_OPEN_FAILED;
> +
> +	err = btrfs_util_f_create_subvolume_iterator(fd, 0, 0, &iter);
> +	if (err)
> +		goto out;
> +
> +	for (;;) {
> +		char child_name[BTRFS_SUBVOL_NAME_MAX + 1];
> +		char *child_path;
> +		int child_fd, new_parent_fd;
> +		uint64_t tmp_transid;
> +
> +		err = btrfs_util_subvolume_iterator_next(iter, &child_path,
> +							 NULL);
> +		if (err) {
> +			if (err == BTRFS_UTIL_ERROR_STOP_ITERATION)
> +				err = BTRFS_UTIL_OK;
> +			break;
> +		}
> +
> +		/* Remove the placeholder directory. */
> +		if (unlinkat(dstfd, child_path, AT_REMOVEDIR) == -1) {
> +			free(child_path);
> +			err = BTRFS_UTIL_ERROR_RMDIR_FAILED;
> +			break;
> +		}
> +
> +		child_fd = openat(fd, child_path, O_RDONLY);
> +		if (child_fd == -1) {
> +			free(child_path);
> +			err = BTRFS_UTIL_ERROR_OPEN_FAILED;
> +			break;
> +		}
> +
> +		err = openat_parent_and_name(dstfd, child_path, child_name,
> +					     sizeof(child_name),
> +					     &new_parent_fd);
> +		free(child_path);
> +		if (err) {
> +			SAVE_ERRNO_AND_CLOSE(child_fd);
> +			break;
> +		}
> +
> +		err = btrfs_util_f2_create_snapshot(child_fd, new_parent_fd,
> +						    child_name, 0,
> +						    async_transid ? &tmp_transid : NULL,
> +						    NULL);
> +		SAVE_ERRNO_AND_CLOSE(child_fd);
> +		SAVE_ERRNO_AND_CLOSE(new_parent_fd);
> +		if (err)
> +			break;
> +		if (async_transid && tmp_transid > *async_transid)
> +			*async_transid = tmp_transid;
> +	}
> +
> +	btrfs_util_destroy_subvolume_iterator(iter);
> +out:
> +	SAVE_ERRNO_AND_CLOSE(dstfd);
> +	return err;
> +}
> +
> +__attribute__((visibility("default")))
> +enum btrfs_util_error btrfs_util_create_snapshot(const char *source,
> +						 const char *path, int flags,
> +						 uint64_t *async_transid,
> +						 struct btrfs_util_qgroup_inherit *qgroup_inherit)
> +{
> +	enum btrfs_util_error err;
> +	int fd;
> +
> +	fd = open(source, O_RDONLY);
> +	if (fd == -1)
> +		return BTRFS_UTIL_ERROR_OPEN_FAILED;
> +
> +	err = btrfs_util_f_create_snapshot(fd, path, flags, async_transid,
> +					   qgroup_inherit);
> +	SAVE_ERRNO_AND_CLOSE(fd);
> +	return err;
> +}
> +
> +__attribute__((visibility("default")))
> +enum btrfs_util_error btrfs_util_f_create_snapshot(int fd, const char *path,
> +						   int flags,
> +						   uint64_t *async_transid,
> +						   struct btrfs_util_qgroup_inherit *qgroup_inherit)
> +{
> +	char name[BTRFS_SUBVOL_NAME_MAX + 1];
> +	enum btrfs_util_error err;
> +	int parent_fd;
> +
> +	err = openat_parent_and_name(AT_FDCWD, path, name, sizeof(name),
> +				     &parent_fd);
> +	if (err)
> +		return err;
> +
> +	err = btrfs_util_f2_create_snapshot(fd, parent_fd, name, flags,
> +					    async_transid, qgroup_inherit);
> +	SAVE_ERRNO_AND_CLOSE(parent_fd);
> +	return err;
> +}
> +
> +__attribute__((visibility("default")))
> +enum btrfs_util_error btrfs_util_f2_create_snapshot(int fd, int parent_fd,
> +						    const char *name, int flags,
> +						    uint64_t *async_transid,
> +						    struct btrfs_util_qgroup_inherit *qgroup_inherit)
> +{
> +	struct btrfs_ioctl_vol_args_v2 args = {.fd = fd};
> +	enum btrfs_util_error err;
> +	size_t len;
> +	int ret;
> +
> +	if ((flags & ~BTRFS_UTIL_CREATE_SNAPSHOT_MASK) ||
> +	    ((flags & BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY) &&
> +	     (flags & BTRFS_UTIL_CREATE_SNAPSHOT_RECURSIVE))) {
> +		errno = EINVAL;
> +		return BTRFS_UTIL_ERROR_INVALID_ARGUMENT;
> +	}
> +
> +	if (flags & BTRFS_UTIL_CREATE_SNAPSHOT_READ_ONLY)
> +		args.flags |= BTRFS_SUBVOL_RDONLY;
> +	if (async_transid)
> +		args.flags |= BTRFS_SUBVOL_CREATE_ASYNC;
> +	if (qgroup_inherit) {
> +		args.flags |= BTRFS_SUBVOL_QGROUP_INHERIT;
> +		args.qgroup_inherit = (struct btrfs_qgroup_inherit *)qgroup_inherit;
> +		args.size = (sizeof(*args.qgroup_inherit) +
> +			     args.qgroup_inherit->num_qgroups *
> +			     sizeof(args.qgroup_inherit->qgroups[0]));
> +	}
> +
> +	len = strlen(name);
> +	if (len >= sizeof(args.name)) {
> +		errno = ENAMETOOLONG;
> +		return BTRFS_UTIL_ERROR_INVALID_ARGUMENT;
> +	}
> +	memcpy(args.name, name, len);
> +	args.name[len] = '\0';
> +
> +	ret = ioctl(parent_fd, BTRFS_IOC_SNAP_CREATE_V2, &args);
> +	if (ret == -1)
> +		return BTRFS_UTIL_ERROR_SUBVOL_CREATE_FAILED;
> +
> +	if (async_transid)
> +		*async_transid = args.transid;
> +
> +	if (flags & BTRFS_UTIL_CREATE_SNAPSHOT_RECURSIVE) {
> +		err = snapshot_subvolume_children(fd, parent_fd, name,
> +						  async_transid);
> +		if (err)
> +			return err;
> +	}
> +
> +	return BTRFS_UTIL_OK;
> +}
> +
>  __attribute__((visibility("default")))
>  void btrfs_util_destroy_subvolume_iterator(struct btrfs_util_subvolume_iterator *iter)
>  {
>

Omar Sandoval Jan. 26, 2018, 7:46 p.m. UTC | #2

On Fri, Jan 26, 2018 at 08:31:06PM +0100, Goffredo Baroncelli wrote:
> On 01/26/2018 07:40 PM, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> 
> 
> Hi,
> 
> this is a great work; only few comments:
> 1) I found not intuitive the naming of the function: i.e. you have 
> 
> btrfs_util_create_snapshot()
> btrfs_util_f_create_snapshot()
> 
> To me it seems more clear to have
> 
> btrfs_util_create_snapshot()
> btrfs_util_create_snapshot_f()
> 
> I think that it is better move the 'f' at the end: at the begin you have the library "btrfs_util", in the middle you have the library function 'create_snapshot', at the end there is the function variant ('f', because it uses a file descriptor).
> 
> This is my opinion, even tough there are both examples like you (stat/fstat/lstat) and like my one (capt_get_fd/cap_get_file)...

Yup, I was going off of the fstat/fsync/etc. convention. I don't
particularly like, e.g., btrfs_create_snapshot_f(), but
btrfs_create_snapshot_fd() isn't so bad.

> 2) I find the prefix 'btrfs_util_' a bit verbose. Why not a simple 'btrfs_', even at the cost of a possible renaming of the conflicting function in the current btrfs code.

That's a reasonable idea, I mostly wanted to avoid naming conflicts but
if this is the "one true Btrfs library" it shouldn't be a concern.

I'll wait a bit for people to bikeshed on the naming before I go and
rename everything, but I'm leaning towards the shorter name and
appending _fd instead of prepending f_.

> 3) regarding the btrfs_util_create_snapshot() function, I think that it would be useful to add some more information:
> a) if used recursive is NOT atomic
> b) if used recursive, root capabilities are needed
> 
> The same for the other functions: mark with a 'root required' tag all the functions which require the root capabilities.

That's a great point, I'll document that.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Qu Wenruo Jan. 27, 2018, 5 a.m. UTC | #3

On 2018年01月27日 03:46, Omar Sandoval wrote:
> On Fri, Jan 26, 2018 at 08:31:06PM +0100, Goffredo Baroncelli wrote:
>> On 01/26/2018 07:40 PM, Omar Sandoval wrote:
>>> From: Omar Sandoval <osandov@fb.com>
>>
>>
>> Hi,
>>
>> this is a great work; only few comments:
>> 1) I found not intuitive the naming of the function: i.e. you have 
>>
>> btrfs_util_create_snapshot()
>> btrfs_util_f_create_snapshot()
>>
>> To me it seems more clear to have
>>
>> btrfs_util_create_snapshot()
>> btrfs_util_create_snapshot_f()
>>
>> I think that it is better move the 'f' at the end: at the begin you have the library "btrfs_util", in the middle you have the library function 'create_snapshot', at the end there is the function variant ('f', because it uses a file descriptor).
>>
>> This is my opinion, even tough there are both examples like you (stat/fstat/lstat) and like my one (capt_get_fd/cap_get_file)...
> 
> Yup, I was going off of the fstat/fsync/etc. convention. I don't
> particularly like, e.g., btrfs_create_snapshot_f(), but
> btrfs_create_snapshot_fd() isn't so bad.

_fd() suffix sounds more reasonable to me too.

> 
>> 2) I find the prefix 'btrfs_util_' a bit verbose. Why not a simple 'btrfs_', even at the cost of a possible renaming of the conflicting function in the current btrfs code.
> 
> That's a reasonable idea, I mostly wanted to avoid naming conflicts but
> if this is the "one true Btrfs library" it shouldn't be a concern.

Unfortunately, at least there is also some planned work to bring a
shared code base between kernel and btrfs-progs, which is also named
libbtrfs, inspired by libxfs.

And depending on the respect of view, some developer may prefer the
short btrfs_ prefix for libbtrfs, while other developers/users will
definitely prefer btrfs_ prefix for libbtrfsutil.

What about shorted prefix like butil_ or btrutil_?

Thanks,
Qu

> 
> I'll wait a bit for people to bikeshed on the naming before I go and
> rename everything, but I'm leaning towards the shorter name and
> appending _fd instead of prepending f_.
> 
>> 3) regarding the btrfs_util_create_snapshot() function, I think that it would be useful to add some more information:
>> a) if used recursive is NOT atomic
>> b) if used recursive, root capabilities are needed
>>
>> The same for the other functions: mark with a 'root required' tag all the functions which require the root capabilities.
> 
> That's a great point, I'll document that.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Omar Sandoval Jan. 27, 2018, 5:45 a.m. UTC | #4

On Sat, Jan 27, 2018 at 01:00:58PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年01月27日 03:46, Omar Sandoval wrote:
> > On Fri, Jan 26, 2018 at 08:31:06PM +0100, Goffredo Baroncelli wrote:
> >> On 01/26/2018 07:40 PM, Omar Sandoval wrote:
> >>> From: Omar Sandoval <osandov@fb.com>
> >>
> >>
> >> Hi,
> >>
> >> this is a great work; only few comments:
> >> 1) I found not intuitive the naming of the function: i.e. you have 
> >>
> >> btrfs_util_create_snapshot()
> >> btrfs_util_f_create_snapshot()
> >>
> >> To me it seems more clear to have
> >>
> >> btrfs_util_create_snapshot()
> >> btrfs_util_create_snapshot_f()
> >>
> >> I think that it is better move the 'f' at the end: at the begin you have the library "btrfs_util", in the middle you have the library function 'create_snapshot', at the end there is the function variant ('f', because it uses a file descriptor).
> >>
> >> This is my opinion, even tough there are both examples like you (stat/fstat/lstat) and like my one (capt_get_fd/cap_get_file)...
> > 
> > Yup, I was going off of the fstat/fsync/etc. convention. I don't
> > particularly like, e.g., btrfs_create_snapshot_f(), but
> > btrfs_create_snapshot_fd() isn't so bad.
> 
> _fd() suffix sounds more reasonable to me too.
> 
> > 
> >> 2) I find the prefix 'btrfs_util_' a bit verbose. Why not a simple 'btrfs_', even at the cost of a possible renaming of the conflicting function in the current btrfs code.
> > 
> > That's a reasonable idea, I mostly wanted to avoid naming conflicts but
> > if this is the "one true Btrfs library" it shouldn't be a concern.
> 
> Unfortunately, at least there is also some planned work to bring a
> shared code base between kernel and btrfs-progs, which is also named
> libbtrfs, inspired by libxfs.

That's right, I forgot about that. There's definitely value in having a
distinction between btrfs_util_ (userspace interfaces) and btrfs_
(filesystem disk format).

> And depending on the respect of view, some developer may prefer the
> short btrfs_ prefix for libbtrfs, while other developers/users will
> definitely prefer btrfs_ prefix for libbtrfsutil.
> 
> What about shorted prefix like butil_ or btrutil_?

Those aren't very informative, I think sticking with btrfs_util_ is
fine, it's not that bad to type out.

> Thanks,
> Qu
> 
> > 
> > I'll wait a bit for people to bikeshed on the naming before I go and
> > rename everything, but I'm leaning towards the shorter name and
> > appending _fd instead of prepending f_.
> > 
> >> 3) regarding the btrfs_util_create_snapshot() function, I think that it would be useful to add some more information:
> >> a) if used recursive is NOT atomic
> >> b) if used recursive, root capabilities are needed
> >>
> >> The same for the other functions: mark with a 'root required' tag all the functions which require the root capabilities.
> > 
> > That's a great point, I'll document that.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 



--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goffredo Baroncelli Jan. 27, 2018, 2:19 p.m. UTC | #5

On 01/27/2018 06:45 AM, Omar Sandoval wrote:
> On Sat, Jan 27, 2018 at 01:00:58PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年01月27日 03:46, Omar Sandoval wrote:
>>> On Fri, Jan 26, 2018 at 08:31:06PM +0100, Goffredo Baroncelli wrote:
>>>> On 01/26/2018 07:40 PM, Omar Sandoval wrote:
>>>>> From: Omar Sandoval <osandov@fb.com>
[...]
>> Unfortunately, at least there is also some planned work to bring a
>> shared code base between kernel and btrfs-progs, which is also named
>> libbtrfs, inspired by libxfs.
> 
> That's right, I forgot about that. There's definitely value in having a
> distinction between btrfs_util_ (userspace interfaces) and btrfs_
> (filesystem disk format).
> 
>> And depending on the respect of view, some developer may prefer the
>> short btrfs_ prefix for libbtrfs, while other developers/users will
>> definitely prefer btrfs_ prefix for libbtrfsutil.
>>
>> What about shorted prefix like butil_ or btrutil_?
> 
> Those aren't very informative, I think sticking with btrfs_util_ is
> fine, it's not that bad to type out.

Let me another chance: what about 'ubtrfs_'...



> 
>> Thanks,
>> Qu
>>
>>>
>>> I'll wait a bit for people to bikeshed on the naming before I go and
>>> rename everything, but I'm leaning towards the shorter name and
>>> appending _fd instead of prepending f_.
>>>
>>>> 3) regarding the btrfs_util_create_snapshot() function, I think that it would be useful to add some more information:
>>>> a) if used recursive is NOT atomic
>>>> b) if used recursive, root capabilities are needed
>>>>
>>>> The same for the other functions: mark with a 'root required' tag all the functions which require the root capabilities.
>>>
>>> That's a great point, I'll document that.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> 
> 
>

Nikolay Borisov Jan. 27, 2018, 4:31 p.m. UTC | #6

On 27.01.2018 07:45, Omar Sandoval wrote:
> On Sat, Jan 27, 2018 at 01:00:58PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年01月27日 03:46, Omar Sandoval wrote:
>>> On Fri, Jan 26, 2018 at 08:31:06PM +0100, Goffredo Baroncelli wrote:
>>>> On 01/26/2018 07:40 PM, Omar Sandoval wrote:
>>>>> From: Omar Sandoval <osandov@fb.com>
>>>>
>>>>
>>>> Hi,
>>>>
>>>> this is a great work; only few comments:
>>>> 1) I found not intuitive the naming of the function: i.e. you have 
>>>>
>>>> btrfs_util_create_snapshot()
>>>> btrfs_util_f_create_snapshot()
>>>>
>>>> To me it seems more clear to have
>>>>
>>>> btrfs_util_create_snapshot()
>>>> btrfs_util_create_snapshot_f()
>>>>
>>>> I think that it is better move the 'f' at the end: at the begin you have the library "btrfs_util", in the middle you have the library function 'create_snapshot', at the end there is the function variant ('f', because it uses a file descriptor).
>>>>
>>>> This is my opinion, even tough there are both examples like you (stat/fstat/lstat) and like my one (capt_get_fd/cap_get_file)...
>>>
>>> Yup, I was going off of the fstat/fsync/etc. convention. I don't
>>> particularly like, e.g., btrfs_create_snapshot_f(), but
>>> btrfs_create_snapshot_fd() isn't so bad.
>>
>> _fd() suffix sounds more reasonable to me too.
>>
>>>
>>>> 2) I find the prefix 'btrfs_util_' a bit verbose. Why not a simple 'btrfs_', even at the cost of a possible renaming of the conflicting function in the current btrfs code.
>>>
>>> That's a reasonable idea, I mostly wanted to avoid naming conflicts but
>>> if this is the "one true Btrfs library" it shouldn't be a concern.
>>
>> Unfortunately, at least there is also some planned work to bring a
>> shared code base between kernel and btrfs-progs, which is also named
>> libbtrfs, inspired by libxfs.

So why don't we implement the shared code as part of this library, won't
it result in just some more files + btrfs_* named function? This "core"
part will be built always when compiling the kernel/userspace progs. In
case we are compiling the userspace progs, additionally we can compile
in the libbtrfsutil essentially + the python modules? Why do we need to
keep the 2 libraries completely separate? For example we can have
something like:

libbtrfs/common/ - here lives the common Userspace/kernel code
libbtrfs/userspace/ - here lives the code of libbtrfsutil, which will
presumably consume certain function from the common dir?

> 
> That's right, I forgot about that. There's definitely value in having a
> distinction between btrfs_util_ (userspace interfaces) and btrfs_
> (filesystem disk format).
> 
>> And depending on the respect of view, some developer may prefer the
>> short btrfs_ prefix for libbtrfs, while other developers/users will
>> definitely prefer btrfs_ prefix for libbtrfsutil.
>>
>> What about shorted prefix like butil_ or btrutil_?
> 
> Those aren't very informative, I think sticking with btrfs_util_ is
> fine, it's not that bad to type out.
> 
>> Thanks,
>> Qu
>>
>>>
>>> I'll wait a bit for people to bikeshed on the naming before I go and
>>> rename everything, but I'm leaning towards the shorter name and
>>> appending _fd instead of prepending f_.
>>>
>>>> 3) regarding the btrfs_util_create_snapshot() function, I think that it would be useful to add some more information:
>>>> a) if used recursive is NOT atomic
>>>> b) if used recursive, root capabilities are needed
>>>>
>>>> The same for the other functions: mark with a 'root required' tag all the functions which require the root capabilities.
>>>
>>> That's a great point, I'll document that.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Omar Sandoval Jan. 27, 2018, 6:54 p.m. UTC | #7

On Sat, Jan 27, 2018 at 06:31:11PM +0200, Nikolay Borisov wrote:
> 
> 
> On 27.01.2018 07:45, Omar Sandoval wrote:
> > On Sat, Jan 27, 2018 at 01:00:58PM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2018年01月27日 03:46, Omar Sandoval wrote:
> >>> On Fri, Jan 26, 2018 at 08:31:06PM +0100, Goffredo Baroncelli wrote:
> >>>> On 01/26/2018 07:40 PM, Omar Sandoval wrote:
> >>>>> From: Omar Sandoval <osandov@fb.com>
> >>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> this is a great work; only few comments:
> >>>> 1) I found not intuitive the naming of the function: i.e. you have 
> >>>>
> >>>> btrfs_util_create_snapshot()
> >>>> btrfs_util_f_create_snapshot()
> >>>>
> >>>> To me it seems more clear to have
> >>>>
> >>>> btrfs_util_create_snapshot()
> >>>> btrfs_util_create_snapshot_f()
> >>>>
> >>>> I think that it is better move the 'f' at the end: at the begin you have the library "btrfs_util", in the middle you have the library function 'create_snapshot', at the end there is the function variant ('f', because it uses a file descriptor).
> >>>>
> >>>> This is my opinion, even tough there are both examples like you (stat/fstat/lstat) and like my one (capt_get_fd/cap_get_file)...
> >>>
> >>> Yup, I was going off of the fstat/fsync/etc. convention. I don't
> >>> particularly like, e.g., btrfs_create_snapshot_f(), but
> >>> btrfs_create_snapshot_fd() isn't so bad.
> >>
> >> _fd() suffix sounds more reasonable to me too.
> >>
> >>>
> >>>> 2) I find the prefix 'btrfs_util_' a bit verbose. Why not a simple 'btrfs_', even at the cost of a possible renaming of the conflicting function in the current btrfs code.
> >>>
> >>> That's a reasonable idea, I mostly wanted to avoid naming conflicts but
> >>> if this is the "one true Btrfs library" it shouldn't be a concern.
> >>
> >> Unfortunately, at least there is also some planned work to bring a
> >> shared code base between kernel and btrfs-progs, which is also named
> >> libbtrfs, inspired by libxfs.
> 
> So why don't we implement the shared code as part of this library, won't
> it result in just some more files + btrfs_* named function? This "core"
> part will be built always when compiling the kernel/userspace progs. In
> case we are compiling the userspace progs, additionally we can compile
> in the libbtrfsutil essentially + the python modules? Why do we need to
> keep the 2 libraries completely separate? For example we can have
> something like:
> 
> libbtrfs/common/ - here lives the common Userspace/kernel code
> libbtrfs/userspace/ - here lives the code of libbtrfsutil, which will
> presumably consume certain function from the common dir?

The problem with this is that I really want libbtrfsutil to be LGPL, so
it has to be separate from the GPL filesystem code. Implementation-wise,
libbtrfsutil doesn't need to know anything about the filesystem format,
and I found it nice to avoid all of the kernel-isms, just writing
standard C. And the filesystem format code is fundamentally different
from the userspace interface code, so I think the split makes sense
regardless.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[11/26] libbtrfsutil: add btrfs_util_create_snapshot()

Commit Message

Comments

Patch