| Message ID | 20250212064501.314097-1-tchou@synology.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs-progs: Preserve first error in loop of check_extent_refs() | expand |
在 2025/2/12 17:15, tchou 写道: > Previously, the `err` variable inside the loop was updated with > `cur_err` on every iteration, regardless of whether `cur_err` indicated > an error. This caused a bug where an earlier error could be overwritten > by a later successful iteration, losing the original failure. > > This fix ensures that `err` retains the first encountered error and is > not reset by subsequent successful iterations. SoB line please, otherwise looks good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> And if it's a case you hit in real world, mind to extract or craft a minimal image as a test case? Thanks, Qu > --- > check/main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/check/main.c b/check/main.c > index 6290c6d4..974ff685 100644 > --- a/check/main.c > +++ b/check/main.c > @@ -8322,7 +8322,8 @@ static int check_extent_refs(struct btrfs_root *root, > cur_err = 1; > } > next: > - err = cur_err; > + if (cur_err) > + err = cur_err; > remove_cache_extent(extent_cache, cache); > free_all_extent_backrefs(rec); > if (!init_extent_tree && opt_check_repair && (!cur_err || fix))
On Wed, Feb 12, 2025 at 05:44:57PM +1030, Qu Wenruo wrote: > 在 2025/2/12 17:15, tchou 写道: > > Previously, the `err` variable inside the loop was updated with > > `cur_err` on every iteration, regardless of whether `cur_err` indicated > > an error. This caused a bug where an earlier error could be overwritten > > by a later successful iteration, losing the original failure. > > > > This fix ensures that `err` retains the first encountered error and is > > not reset by subsequent successful iterations. > > SoB line please, otherwise looks good to me. For btrfs-progs teh SoB line is optional, I think it's mentioned in the README. It makes sense for kernel to insist on the attribution and some trace to real people but btrfs-progs are much smaller and I think it's better to lower the contribution barriers. > Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, feel free to add the commit to devel. > And if it's a case you hit in real world, mind to extract or craft a > minimal image as a test case? From my experience, casual contributors will rarely write tests and I do not do that when I'm sending a fix or reporting a problem to other projects. In order to keep the track of that it may be best to open an issue, unless you or me will write the test case.
David Sterba 於 2025/2/13 07:29 寫道: > On Wed, Feb 12, 2025 at 05:44:57PM +1030, Qu Wenruo wrote: >> 在 2025/2/12 17:15, tchou 写道: >>> Previously, the `err` variable inside the loop was updated with >>> `cur_err` on every iteration, regardless of whether `cur_err` indicated >>> an error. This caused a bug where an earlier error could be overwritten >>> by a later successful iteration, losing the original failure. >>> >>> This fix ensures that `err` retains the first encountered error and is >>> not reset by subsequent successful iterations. >> >> SoB line please, otherwise looks good to me. > > For btrfs-progs teh SoB line is optional, I think it's mentioned in the > README. It makes sense for kernel to insist on the attribution and some > trace to real people but btrfs-progs are much smaller and I think it's > better to lower the contribution barriers. Thanks, I will add it next time send patch. > >> Reviewed-by: Qu Wenruo <wqu@suse.com> > > Thanks, feel free to add the commit to devel. > Do I need to send PR in github of btrfs-progs, or maintainer will add the commit directly? >> And if it's a case you hit in real world, mind to extract or craft a >> minimal image as a test case? > > From my experience, casual contributors will rarely write tests and I do > not do that when I'm sending a fix or reporting a problem to other > projects. In order to keep the track of that it may be best to open an > issue, unless you or me will write the test case. Not the real case, but fake case by modifying the bytenr of extent to wrong number when test the correctness of function we develop with backref check. I think every backpointer mismatch case can test it. Thanks, TCHou
在 2025/2/13 16:39, tchou 写道: > David Sterba 於 2025/2/13 07:29 寫道: >> On Wed, Feb 12, 2025 at 05:44:57PM +1030, Qu Wenruo wrote: >>> 在 2025/2/12 17:15, tchou 写道: >>>> Previously, the `err` variable inside the loop was updated with >>>> `cur_err` on every iteration, regardless of whether `cur_err` indicated >>>> an error. This caused a bug where an earlier error could be overwritten >>>> by a later successful iteration, losing the original failure. >>>> >>>> This fix ensures that `err` retains the first encountered error and is >>>> not reset by subsequent successful iterations. >>> >>> SoB line please, otherwise looks good to me. >> >> For btrfs-progs teh SoB line is optional, I think it's mentioned in the >> README. It makes sense for kernel to insist on the attribution and some >> trace to real people but btrfs-progs are much smaller and I think it's >> better to lower the contribution barriers. > > Thanks, I will add it next time send patch. > >> >>> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> Thanks, feel free to add the commit to devel. >> > > Do I need to send PR in github of btrfs-progs, or maintainer will add > the commit directly? No worry, feel free to use either/both mail or github pull request. We can handle both at merge time. (In fact, I send mail for personal accounting, and github PR for real merge) > >>> And if it's a case you hit in real world, mind to extract or craft a >>> minimal image as a test case? >> >> From my experience, casual contributors will rarely write tests and I do >> not do that when I'm sending a fix or reporting a problem to other >> projects. In order to keep the track of that it may be best to open an >> issue, unless you or me will write the test case. > > Not the real case, but fake case by modifying the bytenr of extent to > wrong number when test the correctness of function we develop with > backref check. I think every backpointer mismatch case can test it. Thanks, let me try to craft a minimal image with this method. Thanks, Qu > > Thanks, TCHou >
diff --git a/check/main.c b/check/main.c index 6290c6d4..974ff685 100644 --- a/check/main.c +++ b/check/main.c @@ -8322,7 +8322,8 @@ static int check_extent_refs(struct btrfs_root *root, cur_err = 1; } next: - err = cur_err; + if (cur_err) + err = cur_err; remove_cache_extent(extent_cache, cache); free_all_extent_backrefs(rec); if (!init_extent_tree && opt_check_repair && (!cur_err || fix))