Re: [PATCH v4 4/7] bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 23, 2023, 09:19 UTC
- Message-ID
- <ZTY6kTZT-ni16usH@tanuki>
- In-Reply-To
- <e427fe6ad383cc238c13f313dc9f11eab37a3840.1697736516.git.me@ttaylorr.com>
On Thu, Oct 19, 2023 at 01:28:51PM -0400, Taylor Blau wrote:
Show 53 quoted lines
> Continue to prepare for streaming an object's contents directly from
> memory by teaching `bulk_checkin_source` how to perform reads and seeks
> based on an address in memory.
>
> Unlike file descriptors, which manage their own offset internally, we
> have to keep track of how many bytes we've read out of the buffer, and
> make sure we don't read past the end of the buffer.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> bulk-checkin.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 28bc8d5ab4..60361b3e2e 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -141,11 +141,15 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
> }
>
> struct bulk_checkin_source {
> - enum { SOURCE_FILE } type;
> + enum { SOURCE_FILE, SOURCE_INCORE } type;
>
> /* SOURCE_FILE fields */
> int fd;
>
> + /* SOURCE_INCORE fields */
> + const void *buf;
> + size_t read;
> +
> /* common fields */
> size_t size;
> const char *path;
> @@ -157,6 +161,11 @@ static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
> switch (source->type) {
> case SOURCE_FILE:
> return lseek(source->fd, offset, SEEK_SET);
> + case SOURCE_INCORE:
> + if (!(0 <= offset && offset < source->size))
> + return (off_t)-1;
> + source->read = offset;
> + return source->read;
> default:
> BUG("unknown bulk-checkin source: %d", source->type);
> }
> @@ -168,6 +177,13 @@ static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
> switch (source->type) {
> case SOURCE_FILE:
> return read_in_full(source->fd, buf, nr);
> + case SOURCE_INCORE:
> + assert(source->read <= source->size);Is there any guideline around when to use `assert()` vs `BUG()`? I think that this assertion here is quite critical, because when it does not hold we can end up performing out-of-bounds reads and writes. But as asserts are typically missing in non-debug builds, this safeguard would not do anything for our end users, right?
Patrick
Show 11 quoted lines
> + if (nr > source->size - source->read)
> + nr = source->size - source->read;
> + memcpy(buf, (unsigned char *)source->buf + source->read, nr);
> + source->read += nr;
> + return nr;
> default:
> BUG("unknown bulk-checkin source: %d", source->type);
> }
> --
> 2.42.0.405.g86fe3250c2
>