Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 25, 2023, 15:44 UTC
Message-ID: <ZTk3zoncT6nvV3aQ@nand.local>
In-Reply-To: <ZTjKjkRMkmCuxDU1@tanuki>

On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:

Show 10 quoted lines

> On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> > arbitrary blob contents from memory into the bulk-checkin pack.
> >
> > In order to support streaming from a location in memory, we must
> > implement a new kind of bulk_checkin_source that does just that. These
> > implementation in spread out across:
>
> Nit: the commit message is a bit off here. Probably not worth a reroll
> though.

Your eyes are definitely mine, because I'm not seeing where the commit message is off! But hopefully since you already don't think it's worth a reroll, and I'm not even sure what the issue is that we can just leave it ;-).

Show 8 quoted lines

> > +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> > +					     off_t offset)
> > +{
> > +	if (!(0 <= offset && offset < source->size))
> > +		return (off_t)-1;
>
> At the risk of showing my own ignorance, but why is the cast here
> necessary?

It's not necessary, see this godbolt example that shows two functions doing the same thing (one with the explicit cast, one without):

  https://godbolt.org/z/f737EcGfG

But there are a couple of other spots within the bulk-checkin code (specifically within the deflate_blob_to_pack() routine) that explicitly cast -1 to an off_t, so I was more trying to imitate the local style.

Thanks, Taylor