| Message ID | 20181114133520.16069-7-jthumshirn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix compiler warning with make W=1 | expand |
On 14.11.18 г. 15:35 ч., Johannes Thumshirn wrote: > Currently the function prototypes of: > * search_free_space_info() > * convert_free_space_to_bitmaps() > * convert_free_space_to_extents() > * free_space_test_bit() > * __remove_from_free_space_tree() > * __add_to_free_space_tree() > are hidden behind CONFIG_BTRFS_FS_RUN_SANITY_TESTS. > > If CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not set, these functions are only > used within free-space-tree.c so technically there would be no need to provide > the function prototypes. But when CONFIG_BTRFS_FS_RUN_SANITY_TESTS they are > also used from tests/free-space-tree-tests.c which explains the need for > the function prototypes and the non-static declaration of the functions. > > When compiling with -Wmissing-prototypes and > CONFIG_BTRFS_FS_RUN_SANITY_TESTS=n gcc emits a warning for each of these > functions, so remove the ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS guard to > make the compiler happy when running 'make W=1'. I agree with this patch, however you go into the gray area of "everything which is exported should have btrfs_ prefix". It's up to David to see if he is content with this change. > > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > fs/btrfs/free-space-tree.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/btrfs/free-space-tree.h b/fs/btrfs/free-space-tree.h > index 3133651d7d70..0f4db2ad68c8 100644 > --- a/fs/btrfs/free-space-tree.h > +++ b/fs/btrfs/free-space-tree.h > @@ -27,7 +27,6 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans, > int remove_from_free_space_tree(struct btrfs_trans_handle *trans, > u64 start, u64 size); > > -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS > struct btrfs_free_space_info * > search_free_space_info(struct btrfs_trans_handle *trans, > struct btrfs_fs_info *fs_info, > @@ -47,6 +46,5 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans, > struct btrfs_path *path); > int free_space_test_bit(struct btrfs_block_group_cache *block_group, > struct btrfs_path *path, u64 offset); > -#endif > > #endif >
On 14/11/2018 14:52, Nikolay Borisov wrote: > > I agree with this patch, however you go into the gray area of > "everything which is exported should have btrfs_ prefix". It's up to > David to see if he is content with this change. Yep you're right. I think I should change it, but I'll wait for David's response first. Byte, Johannes
On Wed, Nov 14, 2018 at 02:54:37PM +0100, Johannes Thumshirn wrote: > On 14/11/2018 14:52, Nikolay Borisov wrote: > > I agree with this patch, however you go into the gray area of > > "everything which is exported should have btrfs_ prefix". It's up to > > David to see if he is content with this change. > > Yep you're right. I think I should change it, but I'll wait for David's > response first. There's one prior example of the conditionally exported functions: btrfs_find_lock_delalloc_range . This function declaration and definition are under the ifdef and it's a simple wrapper around a static function find_lock_delalloc_range. I'd do the same for the functions in your list, where applies. A static function can be better optimized and I don't want to make it harder for the compiler just to have it exported for the tests. They're not enabled by default and never in production builds. So: free-space-tree.h: #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS struct btrfs_free_space_info *btrfs_search_free_space_info( struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, struct btrfs_block_group_cache *block_group, struct btrfs_path *path, int cow); ... #endif free-space-tree.c: #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS struct btrfs_free_space_info *btrfs_search_free_space_info( struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, struct btrfs_block_group_cache *block_group, struct btrfs_path *path, int cow) { return search_free_space_info(trans, fs_info, block_group, path, cow); } ... #endif I hope it's a reasonable compromise among the options.
On 14.11.18 г. 21:53 ч., David Sterba wrote: > On Wed, Nov 14, 2018 at 02:54:37PM +0100, Johannes Thumshirn wrote: >> On 14/11/2018 14:52, Nikolay Borisov wrote: >>> I agree with this patch, however you go into the gray area of >>> "everything which is exported should have btrfs_ prefix". It's up to >>> David to see if he is content with this change. >> >> Yep you're right. I think I should change it, but I'll wait for David's >> response first. > > There's one prior example of the conditionally exported functions: > btrfs_find_lock_delalloc_range . This function declaration and > definition are under the ifdef and it's a simple wrapper around a static > function find_lock_delalloc_range. I have a patch which actually simplifies this somewhat because, frankly I find it stupid and redundant to do this frickin' dance for functions which are used only in debugging. Here is the diff (still not submitted since it's waiting for some other cleanups to be merged): --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1556,10 +1556,13 @@ static noinline int lock_delalloc_pages(struct inode *inode, * * 1 is returned if we find something, 0 if nothing was in the tree */ -static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode, - struct extent_io_tree *tree, - struct page *locked_page, u64 *start, - u64 *end) +#ifndef CONFIG_BTRFS_FS_RUN_SANITY_TESTS +static +#endif +noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode, + struct extent_io_tree *tree, + struct page *locked_page, + u64 *start, u64 *end) { u64 max_bytes = BTRFS_MAX_EXTENT_SIZE; u64 delalloc_start; @@ -1637,16 +1640,6 @@ static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode, return found; } -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS -u64 btrfs_find_lock_delalloc_range(struct inode *inode, - struct extent_io_tree *tree, - struct page *locked_page, u64 *start, - u64 *end) -{ - return find_lock_delalloc_range(inode, tree, locked_page, start, end); -} -#endif - static int __process_pages_contig(struct address_space *mapping, struct page *locked_page, pgoff_t start_index, pgoff_t end_index, diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index 30bfeefa2d89..12d300fae7a0 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -522,10 +522,8 @@ int free_io_failure(struct extent_io_tree *failure_tree, struct extent_io_tree *io_tree, struct io_failure_record *rec); #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS -u64 btrfs_find_lock_delalloc_range(struct inode *inode, - struct extent_io_tree *tree, - struct page *locked_page, u64 *start, - u64 *end); +u64 find_lock_delalloc_range(struct inode *inode, struct extent_io_tree *tree, + struct page *locked_page, u64 *start, u64 *end); #endif struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info, > > I'd do the same for the functions in your list, where applies. A static > function can be better optimized and I don't want to make it harder for > the compiler just to have it exported for the tests. They're not enabled > by default and never in production builds. > > So: > > free-space-tree.h: > > #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS > > struct btrfs_free_space_info *btrfs_search_free_space_info( > struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, > struct btrfs_block_group_cache *block_group, struct btrfs_path *path, > int cow); > ... > > #endif > > free-space-tree.c: > > #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS > > struct btrfs_free_space_info *btrfs_search_free_space_info( > struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, > struct btrfs_block_group_cache *block_group, struct btrfs_path *path, > int cow) > { > return search_free_space_info(trans, fs_info, block_group, path, cow); > } > > ... > > #endif > > I hope it's a reasonable compromise among the options. >
On Wed, Nov 14, 2018 at 11:05:16PM +0200, Nikolay Borisov wrote: > > > On 14.11.18 г. 21:53 ч., David Sterba wrote: > > On Wed, Nov 14, 2018 at 02:54:37PM +0100, Johannes Thumshirn wrote: > >> On 14/11/2018 14:52, Nikolay Borisov wrote: > >>> I agree with this patch, however you go into the gray area of > >>> "everything which is exported should have btrfs_ prefix". It's up to > >>> David to see if he is content with this change. > >> > >> Yep you're right. I think I should change it, but I'll wait for David's > >> response first. > > > > There's one prior example of the conditionally exported functions: > > btrfs_find_lock_delalloc_range . This function declaration and > > definition are under the ifdef and it's a simple wrapper around a static > > function find_lock_delalloc_range. > > I have a patch which actually simplifies this somewhat because, > frankly I find it stupid and redundant to do this frickin' dance for > functions which are used only in debugging. Here is the diff (still > not submitted since it's waiting for some other cleanups to be > merged): > > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -1556,10 +1556,13 @@ static noinline int lock_delalloc_pages(struct inode *inode, > * > * 1 is returned if we find something, 0 if nothing was in the tree > */ > -static noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode, > - struct extent_io_tree *tree, > - struct page *locked_page, u64 *start, > - u64 *end) > +#ifndef CONFIG_BTRFS_FS_RUN_SANITY_TESTS > +static > +#endif > +noinline_for_stack u64 find_lock_delalloc_range(struct inode *inode, > + struct extent_io_tree *tree, > + struct page *locked_page, > + u64 *start, u64 *end) If there's nostatic_for_tests or export_for_tests (possibly all caps), then ok.
diff --git a/fs/btrfs/free-space-tree.h b/fs/btrfs/free-space-tree.h index 3133651d7d70..0f4db2ad68c8 100644 --- a/fs/btrfs/free-space-tree.h +++ b/fs/btrfs/free-space-tree.h @@ -27,7 +27,6 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans, int remove_from_free_space_tree(struct btrfs_trans_handle *trans, u64 start, u64 size); -#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS struct btrfs_free_space_info * search_free_space_info(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, @@ -47,6 +46,5 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans, struct btrfs_path *path); int free_space_test_bit(struct btrfs_block_group_cache *block_group, struct btrfs_path *path, u64 offset); -#endif #endif
Currently the function prototypes of: * search_free_space_info() * convert_free_space_to_bitmaps() * convert_free_space_to_extents() * free_space_test_bit() * __remove_from_free_space_tree() * __add_to_free_space_tree() are hidden behind CONFIG_BTRFS_FS_RUN_SANITY_TESTS. If CONFIG_BTRFS_FS_RUN_SANITY_TESTS is not set, these functions are only used within free-space-tree.c so technically there would be no need to provide the function prototypes. But when CONFIG_BTRFS_FS_RUN_SANITY_TESTS they are also used from tests/free-space-tree-tests.c which explains the need for the function prototypes and the non-static declaration of the functions. When compiling with -Wmissing-prototypes and CONFIG_BTRFS_FS_RUN_SANITY_TESTS=n gcc emits a warning for each of these functions, so remove the ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS guard to make the compiler happy when running 'make W=1'. Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> --- fs/btrfs/free-space-tree.h | 2 -- 1 file changed, 2 deletions(-)