Patrick Steinhardt <ps@xxxxxx> writes:
[snip]
> diff --git a/reftable/record.c b/reftable/record.c
> index 04429d23fe..4e6541c307 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -21,47 +21,40 @@ static void *reftable_record_data(struct reftable_record *rec);
>
> int get_var_int(uint64_t *dest, struct string_view *in)
> {
> - int ptr = 0;
> + const unsigned char *buf = in->buf;
> + unsigned char c;
> uint64_t val;
>
> - if (in->len == 0)
> + if (!in->len)
> return -1;
> - val = in->buf[ptr] & 0x7f;
> -
> - while (in->buf[ptr] & 0x80) {
> - ptr++;
> - if (ptr > in->len) {
> + c = *buf++;
> + val = c & 0x7f;
> +
> + while (c & 0x80) {
> + val += 1;
I was at first confused, I understand that we add 1 to check if there is
an overflow before adding the next section. But this actually modifies
the value itself, but looking below at `put_var_int()`, we did value--
before storing each continuation byte. So during decoding.
Nit: it would be nice to explain that part a bit here with comments.
> + if (!val || (val & (uint64_t)(~0ULL << (64 - 7))))
> + return -1; /* overflow */
> + if (buf >= in->buf + in->len)
> return -1;
> - }
> - val = (val + 1) << 7 | (uint64_t)(in->buf[ptr] & 0x7f);
> + c = *buf++;
> + val = (val << 7) | (c & 0x7f);
> }
>
> *dest = val;
> - return ptr + 1;
> + return buf - in->buf;
> }
>
> -int put_var_int(struct string_view *dest, uint64_t val)
> +int put_var_int(struct string_view *dest, uint64_t value)
> {
> - uint8_t buf[10] = { 0 };
> - int i = 9;
> - int n = 0;
> - buf[i] = (uint8_t)(val & 0x7f);
> - i--;
> - while (1) {
> - val >>= 7;
> - if (!val) {
> - break;
> - }
> - val--;
> - buf[i] = 0x80 | (uint8_t)(val & 0x7f);
> - i--;
> - }
> -
> - n = sizeof(buf) - i - 1;
> - if (dest->len < n)
> + unsigned char varint[10];
> + unsigned pos = sizeof(varint) - 1;
> + varint[pos] = value & 127;
Nit: While the `get_var_int()` uses hexes, here we use ints. Would be
nicer to use `0x7f` and so on and be consistent.
> + while (value >>= 7)
> + varint[--pos] = 128 | (--value & 127);
> + if (dest->len < sizeof(varint) - pos)
> return -1;
> - memcpy(dest->buf, &buf[i + 1], n);
> - return n;
> + memcpy(dest->buf, varint + pos, sizeof(varint) - pos);
> + return sizeof(varint) - pos;
> }
>
> int reftable_is_block_type(uint8_t typ)
> diff --git a/reftable/record.h b/reftable/record.h
> index a24cb23bd4..721d6c949a 100644
> --- a/reftable/record.h
> +++ b/reftable/record.h
> @@ -34,6 +34,10 @@ static inline void string_view_consume(struct string_view *s, int n)
>
> /* utilities for de/encoding varints */
>
We should remove this, no?
> +/*
> + * Decode and encode a varint. Returns the number of bytes read/written, or a
> + * negative value in case encoding/decoding the varint has failed.
> + */
> int get_var_int(uint64_t *dest, struct string_view *in);
> int put_var_int(struct string_view *dest, uint64_t val);
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> index 42bc64cec8..6d912b9c8f 100644
> --- a/t/unit-tests/t-reftable-record.c
> +++ b/t/unit-tests/t-reftable-record.c
> @@ -58,6 +58,22 @@ static void t_varint_roundtrip(void)
> }
> }
[snip]
Attachment:
signature.asc
Description: PGP signature
