reftable: fix some sparse warnings

Commit Message

Ramsay Jones Sept. 22, 2020, 10:47 p.m. UTC

Signed-off-by: Ramsay Jones <[e-mail]>
---

Hi Han-Wen Nienhuys,

If you need to re-roll your 'hn/reftable' branch, could you please squash this
into the relevant patches.

This patch is based on top of 'seen' and removes 20 sparse warnings (19 of the
"symbol 'whatever' was not declared. Should it be static?" type and a single
"Using plain integer as NULL pointer").

However, this branch also adds many symbols to the output of my 'static-check.pl'
script (this patch actually removes 19 of those symbols :) ).

Looking at the first such symbol:

  $ git grep -n block_writer_register_restart
  reftable/block.c:40:int block_writer_register_restart(struct block_writer *w, int n, int restart,
  reftable/block.c:93:    if (block_writer_register_restart(w, start.len - out.len, is_restart,
  reftable/block.c:105:int block_writer_register_restart(struct block_writer *w, int n, int is_restart,
  $ 

... it looks like you forward declare the function on line 40, use it in line 93
and define it in line 105, all in the single 'block.c' source file. This would
suggest that this symbol is local to this file and should be marked as 'static'.
(Also, unless you have mutually recursive functions, I would avoid the need to
forward declare this function).

Just for your information, you may want to look at the following 27 symbols:

  $ diff nsc ssc1
  ...
  > reftable/block.o	- block_writer_register_restart
  > reftable/merged.o	- merged_table_seek_record
  > reftable/merged.o	- reftable_merged_table_hash_id
  > reftable/merged.o	- reftable_merged_table_max_update_index
  > reftable/merged.o	- reftable_merged_table_min_update_index
  > reftable/merged.o	- reftable_merged_table_seek_log_at
  > reftable/pq.o	- pq_less
  > reftable/publicbasics.o	- reftable_error_to_errno
  > reftable/publicbasics.o	- reftable_set_alloc
  > reftable/reader.o	- reftable_reader_seek_log_at
  > reftable/record.o	- reftable_record_yield
  > reftable/record.o	- reftable_ref_record_vtable
  > reftable/refname.o	- modification_has_ref
  > reftable/refname.o	- modification_has_ref_with_prefix
  > reftable/refname.o	- validate_refname
  > reftable/stack.o	- reftable_addition_close
  > reftable/stack.o	- reftable_stack_auto_compact
  > reftable/stack.o	- reftable_stack_reload_maybe_reuse
  > reftable/stack.o	- stack_check_addition
  > reftable/stack.o	- stack_try_add
  > reftable/stack.o	- stack_write_compact
  > reftable/test_framework.o	- new_test_case
  > reftable/writer.o	- reftable_writer_add_logs
  > reftable/writer.o	- reftable_writer_add_refs
  > reftable/writer.o	- writer_clear_index
  > reftable/writer.o	- writer_finish_public_section
  > reftable/writer.o	- writer_flush_block
  ...
  $ 

To be clear, all of the above symbols are defined, as an external symbol, in
the given object file, but not called/referenced outside of that file.

Thanks!

ATB,
Ramsay Jones


 reftable/blocksource.c    | 8 ++++----
 reftable/iter.c           | 6 +++---
 reftable/merged.c         | 2 +-
 reftable/publicbasics.c   | 6 +++---
 reftable/reader.c         | 2 +-
 reftable/record.c         | 6 +++---
 reftable/test_framework.c | 8 ++++----
 reftable/writer.c         | 2 +-
 8 files changed, 20 insertions(+), 20 deletions(-)

Comments

Han-Wen Nienhuys Sept. 30, 2020, 4:51 p.m. UTC | #1

On Wed, Sep 23, 2020 at 12:47 AM Ramsay Jones
<[e-mail]> wrote:
>
>
> Signed-off-by: Ramsay Jones <[e-mail]>
> ---
>
> Hi Han-Wen Nienhuys,
>
> If you need to re-roll your 'hn/reftable' branch, could you please squash this
> into the relevant patches.
>

Thanks for the heads-up. I fixed some of these issues in the source at
google/reftable. I've seen a Helped-By footer used to acknowledge
these types of contributions, but I'm not sure on which of the 13
commits I should put that; suggestions?

> This patch is based on top of 'seen' and removes 20 sparse warnings (19 of the

Could you tell me how I can run these checks myself?

> Just for your information, you may want to look at the following 27 symbols:

>   > reftable/merged.o   - reftable_merged_table_hash_id
>   > reftable/merged.o   - reftable_merged_table_max_update_index
>   > reftable/merged.o   - reftable_merged_table_min_update_index
>   > reftable/merged.o   - reftable_merged_table_seek_log_at
>   > reftable/publicbasics.o     - reftable_error_to_errno
>   > reftable/publicbasics.o     - reftable_set_alloc
>   > reftable/reader.o   - reftable_reader_seek_log_at
>  > reftable/stack.o    - reftable_addition_close
>   > reftable/stack.o    - reftable_stack_auto_compact

These functions are part of the public API. We'll need to get the
reftable glue code into seen. Perhaps some need unittest coverage too.

Ramsay Jones Sept. 30, 2020, 8:18 p.m. UTC | #2

On 30/09/2020 17:51, Han-Wen Nienhuys wrote:
> On Wed, Sep 23, 2020 at 12:47 AM Ramsay Jones
> <[e-mail]> wrote:

>> If you need to re-roll your 'hn/reftable' branch, could you please squash this
>> into the relevant patches.
>>
> 
> Thanks for the heads-up. I fixed some of these issues in the source at
> google/reftable. I've seen a Helped-By footer used to acknowledge
> these types of contributions, but I'm not sure on which of the 13
> commits I should put that; suggestions?

I will leave you to decide, but I didn't actually do much (sparse
did most of the heavy lifting)! ;-)

> 
>> This patch is based on top of 'seen' and removes 20 sparse warnings (19 of the
> 
> Could you tell me how I can run these checks myself?

  $ make sparse

You will need to install a suitably new version of sparse, of course.
I believe (but don't quote me) that debian testing has a suitable
version based on the 'maint-v0.6.2' branch. ('git describe maint-v0.6.2'
shows that it has four commits on top of the last official release
version: v0.6.2-4-gb47eba20). Having said that, v0.6.2 should also be
fine (I build from source, so have version v0.6.2-201-g24bdaac6).

[for more sparse info, see: https://sparse.docs.kernel.org]

> 
>> Just for your information, you may want to look at the following 27 symbols:
> 
>>   > reftable/merged.o   - reftable_merged_table_hash_id
>>   > reftable/merged.o   - reftable_merged_table_max_update_index
>>   > reftable/merged.o   - reftable_merged_table_min_update_index
>>   > reftable/merged.o   - reftable_merged_table_seek_log_at
>>   > reftable/publicbasics.o     - reftable_error_to_errno
>>   > reftable/publicbasics.o     - reftable_set_alloc
>>   > reftable/reader.o   - reftable_reader_seek_log_at
>>  > reftable/stack.o    - reftable_addition_close
>>   > reftable/stack.o    - reftable_stack_auto_compact
> 
> These functions are part of the public API. We'll need to get the
> reftable glue code into seen. Perhaps some need unittest coverage too.

So, do I take it that the other 18 symbols are now marked 'static'?

[you would need 'static-check.pl' to catch symbols like the above].

ATB,
Ramsay Jones

Patch

diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index f12cea2472..7f29b864f9 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -39,7 +39,7 @@  static uint64_t strbuf_size(void *b)
 	return ((struct strbuf *)b)->len;
 }
 
-struct reftable_block_source_vtable strbuf_vtable = {
+static struct reftable_block_source_vtable strbuf_vtable = {
 	.size = &strbuf_size,
 	.read_block = &strbuf_read_block,
 	.return_block = &strbuf_return_block,
@@ -60,11 +60,11 @@  static void malloc_return_block(void *b, struct reftable_block *dest)
 	reftable_free(dest->data);
 }
 
-struct reftable_block_source_vtable malloc_vtable = {
+static struct reftable_block_source_vtable malloc_vtable = {
 	.return_block = &malloc_return_block,
 };
 
-struct reftable_block_source malloc_block_source_instance = {
+static struct reftable_block_source malloc_block_source_instance = {
 	.ops = &malloc_vtable,
 };
 
@@ -112,7 +112,7 @@  static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,
 	return size;
 }
 
-struct reftable_block_source_vtable file_vtable = {
+static struct reftable_block_source_vtable file_vtable = {
 	.size = &file_size,
 	.read_block = &file_read_block,
 	.return_block = &file_return_block,
diff --git a/reftable/iter.c b/reftable/iter.c
index 6631408ef8..2cff447323 100644
--- a/reftable/iter.c
+++ b/reftable/iter.c
@@ -29,7 +29,7 @@  static void empty_iterator_close(void *arg)
 {
 }
 
-struct reftable_iterator_vtable empty_vtable = {
+static struct reftable_iterator_vtable empty_vtable = {
 	.next = &empty_iterator_next,
 	.close = &empty_iterator_close,
 };
@@ -126,7 +126,7 @@  static int filtering_ref_iterator_next(void *iter_arg,
 	return err;
 }
 
-struct reftable_iterator_vtable filtering_ref_iterator_vtable = {
+static struct reftable_iterator_vtable filtering_ref_iterator_vtable = {
 	.next = &filtering_ref_iterator_next,
 	.close = &filtering_ref_iterator_close,
 };
@@ -228,7 +228,7 @@  int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest,
 	return err;
 }
 
-struct reftable_iterator_vtable indexed_table_ref_iter_vtable = {
+static struct reftable_iterator_vtable indexed_table_ref_iter_vtable = {
 	.next = &indexed_table_ref_iter_next,
 	.close = &indexed_table_ref_iter_close,
 };
diff --git a/reftable/merged.c b/reftable/merged.c
index 22b071cb5d..63fa69bc27 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -155,7 +155,7 @@  static int merged_iter_next_void(void *p, struct reftable_record *rec)
 	return merged_iter_next(mi, rec);
 }
 
-struct reftable_iterator_vtable merged_iter_vtable = {
+static struct reftable_iterator_vtable merged_iter_vtable = {
 	.next = &merged_iter_next_void,
 	.close = &merged_iter_close,
 };
diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c
index a31463ff9a..12d547d70d 100644
--- a/reftable/publicbasics.c
+++ b/reftable/publicbasics.c
@@ -59,9 +59,9 @@  int reftable_error_to_errno(int err)
 	}
 }
 
-void *(*reftable_malloc_ptr)(size_t sz) = &malloc;
-void *(*reftable_realloc_ptr)(void *, size_t) = &realloc;
-void (*reftable_free_ptr)(void *) = &free;
+static void *(*reftable_malloc_ptr)(size_t sz) = &malloc;
+static void *(*reftable_realloc_ptr)(void *, size_t) = &realloc;
+static void (*reftable_free_ptr)(void *) = &free;
 
 void *reftable_malloc(size_t sz)
 {
diff --git a/reftable/reader.c b/reftable/reader.c
index fae2dbb64e..c7f56b5fdc 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -376,7 +376,7 @@  static void table_iter_close(void *p)
 	block_iter_close(&ti->bi);
 }
 
-struct reftable_iterator_vtable table_iter_vtable = {
+static struct reftable_iterator_vtable table_iter_vtable = {
 	.next = &table_iter_next_void,
 	.close = &table_iter_close,
 };
diff --git a/reftable/record.c b/reftable/record.c
index 21c9bba077..3b6884131b 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -539,7 +539,7 @@  static int not_a_deletion(const void *p)
 	return 0;
 }
 
-struct reftable_record_vtable reftable_obj_record_vtable = {
+static struct reftable_record_vtable reftable_obj_record_vtable = {
 	.key = &reftable_obj_record_key,
 	.type = BLOCK_TYPE_OBJ,
 	.copy_from = &reftable_obj_record_copy_from,
@@ -821,7 +821,7 @@  static int reftable_log_record_is_deletion_void(const void *p)
 		(const struct reftable_log_record *)p);
 }
 
-struct reftable_record_vtable reftable_log_record_vtable = {
+static struct reftable_record_vtable reftable_log_record_vtable = {
 	.key = &reftable_log_record_key,
 	.type = BLOCK_TYPE_LOG,
 	.copy_from = &reftable_log_record_copy_from,
@@ -947,7 +947,7 @@  static int reftable_index_record_decode(void *rec, struct strbuf key,
 	return start.len - in.len;
 }
 
-struct reftable_record_vtable reftable_index_record_vtable = {
+static struct reftable_record_vtable reftable_index_record_vtable = {
 	.key = &reftable_index_record_key,
 	.type = BLOCK_TYPE_INDEX,
 	.copy_from = &reftable_index_record_copy_from,
diff --git a/reftable/test_framework.c b/reftable/test_framework.c
index f304a2773a..b5870bea08 100644
--- a/reftable/test_framework.c
+++ b/reftable/test_framework.c
@@ -11,9 +11,9 @@  license that can be found in the LICENSE file or at
 #include "system.h"
 #include "basics.h"
 
-struct test_case **test_cases;
-int test_case_len;
-int test_case_cap;
+static struct test_case **test_cases;
+static int test_case_len;
+static int test_case_cap;
 
 struct test_case *new_test_case(const char *name, void (*testfunc)(void))
 {
@@ -56,7 +56,7 @@  int test_main(int argc, const char *argv[])
 		reftable_free(test_cases[i]);
 	}
 	reftable_free(test_cases);
-	test_cases = 0;
+	test_cases = NULL;
 	test_case_len = 0;
 	test_case_cap = 0;
 	return 0;
diff --git a/reftable/writer.c b/reftable/writer.c
index 44ddcc6757..f569d15ff0 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -589,7 +589,7 @@  void writer_clear_index(struct reftable_writer *w)
 	w->index_cap = 0;
 }
 
-const int debug = 0;
+static const int debug;
 
 static int writer_flush_nonempty_block(struct reftable_writer *w)
 {