diff mbox series

[3/9] fsmonitor: de-duplicate BUG()s around dirty bits

Message ID 838922de2e9756c00f4a159b2b8722ae4d28b011.1611161639.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series More index cleanups | expand

Commit Message

Derrick Stolee Jan. 20, 2021, 4:53 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

The index has an fsmonitor_dirty bitmap that records which index entries
are "dirty" based on the response from the FSMonitor. If this bitmap
ever grows larger than the index, then there was an error in how it was
constructed, and it was probably a developer's bug.

There are several BUG() statements that are very similar, so replace
these uses with a simpler assert_index_minimum(). Since there is one
caller that uses a custom 'pos' value instead of the bit_size member, we
cannot simplify it too much. However, the error string is identical in
each, so this simplifies things.

The end result is that the code is simpler to read while also preserving
these assertions for developers in the FSMonitor space.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 fsmonitor.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Comments

Elijah Newren Jan. 20, 2021, 5:26 p.m. UTC | #1
On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The index has an fsmonitor_dirty bitmap that records which index entries
> are "dirty" based on the response from the FSMonitor. If this bitmap
> ever grows larger than the index, then there was an error in how it was
> constructed, and it was probably a developer's bug.
>
> There are several BUG() statements that are very similar, so replace
> these uses with a simpler assert_index_minimum(). Since there is one
> caller that uses a custom 'pos' value instead of the bit_size member, we
> cannot simplify it too much. However, the error string is identical in
> each, so this simplifies things.
>
> The end result is that the code is simpler to read while also preserving
> these assertions for developers in the FSMonitor space.

Indeed, looking through the patch, the end result is simpler to read.
Nice cleanup.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  fsmonitor.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/fsmonitor.c b/fsmonitor.c
> index ca031c3abb8..52a50a9545a 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -13,14 +13,19 @@
>
>  struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
>
> +static void assert_index_minimum(struct index_state *istate, size_t pos)
> +{
> +       if (pos > istate->cache_nr)
> +               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> +                   (uintmax_t)pos, istate->cache_nr);
> +}
> +
>  static void fsmonitor_ewah_callback(size_t pos, void *is)
>  {
>         struct index_state *istate = (struct index_state *)is;
>         struct cache_entry *ce;
>
> -       if (pos >= istate->cache_nr)
> -               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
> -                   (uintmax_t)pos, istate->cache_nr);
> +       assert_index_minimum(istate, pos);
>
>         ce = istate->cache[pos];
>         ce->ce_flags &= ~CE_FSMONITOR_VALID;
> @@ -82,10 +87,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
>         }
>         istate->fsmonitor_dirty = fsmonitor_dirty;
>
> -       if (!istate->split_index &&
> -           istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +       if (!istate->split_index)
> +               assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
>
>         trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
>         return 0;
> @@ -110,10 +113,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
>         uint32_t ewah_size = 0;
>         int fixup = 0;
>
> -       if (!istate->split_index &&
> -           istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +       if (!istate->split_index)
> +               assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
>
>         put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
>         strbuf_add(sb, &hdr_version, sizeof(uint32_t));
> @@ -335,9 +336,7 @@ void tweak_fsmonitor(struct index_state *istate)
>                         }
>
>                         /* Mark all previously saved entries as dirty */
> -                       if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
> -                               BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> -                                   (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
> +                       assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
>                         ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
>
>                         refresh_fsmonitor(istate);
> --
> gitgitgadget
>
Chris Torek Jan. 21, 2021, 12:53 p.m. UTC | #2
On Wed, Jan 20, 2021 at 8:58 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The index has an fsmonitor_dirty bitmap that records which index entries
> are "dirty" based on the response from the FSMonitor. If this bitmap
> ever grows larger than the index, then there was an error in how it was
> constructed, and it was probably a developer's bug.

Curious: some of the tests were >=, some were > (not >=).  Now
that they're shared in a function they are all ">".

It's pretty clear that for size-based ones, greater-than is the
right test, but for position ones, isn't it still greater-or-equal?  So
perhaps the calls that pass an actual position should add 1...

Chris
Derrick Stolee Jan. 21, 2021, 3:56 p.m. UTC | #3
On 1/21/2021 7:53 AM, Chris Torek wrote:
> On Wed, Jan 20, 2021 at 8:58 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The index has an fsmonitor_dirty bitmap that records which index entries
>> are "dirty" based on the response from the FSMonitor. If this bitmap
>> ever grows larger than the index, then there was an error in how it was
>> constructed, and it was probably a developer's bug.
> 
> Curious: some of the tests were >=, some were > (not >=).  Now
> that they're shared in a function they are all ">".
> 
> It's pretty clear that for size-based ones, greater-than is the
> right test, but for position ones, isn't it still greater-or-equal?  So
> perhaps the calls that pass an actual position should add 1...

That's a good point. I should pass "pos + 1" in the appropriate
places.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/fsmonitor.c b/fsmonitor.c
index ca031c3abb8..52a50a9545a 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -13,14 +13,19 @@ 
 
 struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
 
+static void assert_index_minimum(struct index_state *istate, size_t pos)
+{
+	if (pos > istate->cache_nr)
+		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
+		    (uintmax_t)pos, istate->cache_nr);
+}
+
 static void fsmonitor_ewah_callback(size_t pos, void *is)
 {
 	struct index_state *istate = (struct index_state *)is;
 	struct cache_entry *ce;
 
-	if (pos >= istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" >= %u)",
-		    (uintmax_t)pos, istate->cache_nr);
+	assert_index_minimum(istate, pos);
 
 	ce = istate->cache[pos];
 	ce->ce_flags &= ~CE_FSMONITOR_VALID;
@@ -82,10 +87,8 @@  int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	}
 	istate->fsmonitor_dirty = fsmonitor_dirty;
 
-	if (!istate->split_index &&
-	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index)
+		assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 
 	trace_printf_key(&trace_fsmonitor, "read fsmonitor extension successful");
 	return 0;
@@ -110,10 +113,8 @@  void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	uint32_t ewah_size = 0;
 	int fixup = 0;
 
-	if (!istate->split_index &&
-	    istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+	if (!istate->split_index)
+		assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 
 	put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
@@ -335,9 +336,7 @@  void tweak_fsmonitor(struct index_state *istate)
 			}
 
 			/* Mark all previously saved entries as dirty */
-			if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)
-				BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
-				    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
+			assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size);
 			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
 
 			refresh_fsmonitor(istate);