Re: [PATCH 11/26] libbtrfsutil: add btrfs_util_create_snapshot()

[Omar Sandoval <osandov@osandov.com>]

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
> > 
>