Re: [PATCH v2 5/5] pack-bitmap.c: using error() instead of silently returning -1

From: Teng Long <dyroneteng@gmail.com>
Date: May 6, 2022, 12:55 UTC
Message-ID: <20220506125530.70989-1-dyroneteng@gmail.com>
In-Reply-To: <220421.86o80u77xj.gmgdl@evledraar.gmail.com>

On Thu, 21 Apr 2022 17:41:36 +0200, Ævar Arnfjörð Bjarmason wrote:

> First, I wondered if we were missing _(), but looking at other string in
> the file they're not using that already, looks like these all should,
> but we can fix this all up some other time in some i18n commit. It's
> fine to keep this for now.

Yes, I agree with you.

I also think I have a willingness to make another patchset to solve the _() missing problems recently.

> But more importantly: I think this should be your 4/5. I.e. just make
> these an error() and you won't need to add e.g. this
> trace2_data_string() for a failed stat.

Make sense. Will adjust the order in next path.

> You will be inside your trace2 region, so any failure to stat etc. will
> be obvious from the structure of the data and the "error" event, no
> reason to have an additional trace2_data_string().

Yeah, I forgot about the "error()" already load the trace2 functions in. Will remove the redundant trace2_data_string() where it's obviously will return error().

Show 9 quoted lines

> Aside from that & as a general matter: Unless you have some use-case for
> trace2 data in this detail I'd think that it would be better just to
> skip logging it (except if we get it for free, such as with error()).
> 
> I.e. is this particular callsite really different from other places we
> fail to stat() or open() something?
> 
> It's all a moot point with the region + error, but just something to
> keep in mind.

Make sense.

And I think it's a good case in "open_midx_bitmap_1()" to add related trace2_data_string() because there only a general error info in "cleanup:".

Thanks.