Re: [PATCH v3] btrfs-progs: Remove support for BTRFS_SUBVOL_CREATE_ASYNC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 21, 2020 at 12:36:21PM +0300, Nikolay Borisov wrote:
> 
> 
> On 21.04.20 г. 1:56 ч., David Sterba wrote:
> > On Thu, Apr 02, 2020 at 03:31:47PM +0300, Nikolay Borisov wrote:
> >> Kernel has removed support for this feature in 5.7 so let's remove
> >> support from progs as well.
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> >> Reviewed-by: Omar Sandoval <osandov@xxxxxx>
> >> ---
> >> Changelog V3:
> >>  * Deleted unnecessary function documentation (Omar)
> >>  * Decapitalize some words (Omar)
> >>
> >> Changelog v2:
> >>  * Removed async mentions in README.md
> >>  * Changed docs in libbtrfsutil/btrfsutil.h to mention async is unused.
> >>  * Removed tests using async_
> >>  * Changed python module's doc to mention the async_ parameter is unused.
> >>  ioctl.h                                     |  4 +--
> >>  libbtrfsutil/README.md                      | 14 ++------
> >>  libbtrfsutil/btrfs.h                        |  4 +--
> >>  libbtrfsutil/btrfsutil.h                    | 23 +++++--------
> >>  libbtrfsutil/python/module.c                |  6 ++--
> >>  libbtrfsutil/python/tests/test_subvolume.py | 12 ++-----
> >>  libbtrfsutil/subvolume.c                    | 38 ++++++---------------
> >>  7 files changed, 29 insertions(+), 72 deletions(-)
> >>
> >> diff --git a/ioctl.h b/ioctl.h
> >> index ade6dcb91044..b63391f904c4 100644
> >> --- a/ioctl.h
> >> +++ b/ioctl.h
> >> @@ -49,15 +49,13 @@ BUILD_ASSERT(sizeof(struct btrfs_ioctl_vol_args) == 4096);
> >>
> >>  #define BTRFS_DEVICE_PATH_NAME_MAX 1024
> >>
> >> -#define BTRFS_SUBVOL_CREATE_ASYNC	(1ULL << 0)
> > 
> > We got the report that removing the symbol breaks compilation, and ioctl.h is
> > exported to libbtrfs. I'm not aware of any 3rd party tool using this symbol, we
> > may want to make it more relaxed and keep the definition, but warn if is
> > used in any of the public interfaces.
> 
> IMO that symbol should really be exposed from only one place - namely
> the UAPI headers.

It has been part of the libbtrfs public API for a long time so we need
to consider the backward compatibility, regardless what should have been
done differently.

> So instead of having it defined here we ought to
> include the respective UAPI header in libbtrfsutil.

No. The point of libbtrfsutil is to be independent and self contained,
and it has it's own copy of all btrfs structures that also happen to be
exported via UAPI.

> Also, I don't see an
> include "ioctl.h" in any libbtrfsutil file:
> 
> git grep ioctl.h libbtrfsutil/
> libbtrfsutil/btrfs.h:#include <linux/ioctl.h>
> libbtrfsutil/filesystem.c:#include <sys/ioctl.h>
> libbtrfsutil/subvolume.c:#include <sys/ioctl.h>

That's exactly right. Libbtrfsutil is library done the right way, unlike
libbtrfs, so the code is not shared.



[Index of Archives]     [Linux Filesystem Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux