Re: [PATCH v10 09/12] Add reftable library

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Apr 28, 2020, 20:21 UTC
Message-ID: <CAFQ2z_NtsmyqqGPRr7YWfTWGRAjrgBv5NtV2k6oMfoSbwuAOEA@mail.gmail.com>
In-Reply-To: <20200428145541.GC31366@danh.dev>

On Tue, Apr 28, 2020 at 4:55 PM Danh Doan <congdanhqx@gmail.com> wrote:

Show 5 quoted lines

> > +#include "system.h"
> > +
> > +void put_be24(byte *out, uint32_t i)
>
> So, we introduce a new type `byte`?

The reftable is structured as a library. The declaration for 'byte', as well as 'bool' is private to the library.

Show 10 quoted lines

> > +{
> > +     out[0] = (byte)((i >> 16) & 0xff);
> > +     out[1] = (byte)((i >> 8) & 0xff);
> > +     out[2] = (byte)((i)&0xff);
>
> At my first glance, I thought that code is:
>
>         out[2] = (byte)( (i)(&0xff) )
>
> It's totally un-parsable.

I removed the parens here, in https://github.com/google/reftable/commit/6f883714588d2658122fefd85a2e2e76401ae2be

that also changes a number of 'int's to 'size_t's.

> > +     return (uint32_t)(in[0]) << 16 | (uint32_t)(in[1]) << 8 |
> > +            (uint32_t)(in[2]);
>
> I don't think we need this much cast. One cast can rule them all.

would you have a concrete suggestion to rewrite this? The rules for undefined behavior and integer casts are subtle, and I know they are a cause of concern in crypto code, and I'd rather be correct than rely on deep knowledge of the C standard.

Show 5 quoted lines

> > +int binsearch(int sz, int (*f)(int k, void *args), void *args)
>
> binary search?
>
> I think we want all size to be `size_t`

fixed this.

Show 10 quoted lines

>
> > +{
> > +     int lo = 0;
> > +     int hi = sz;
> > +
> > +     /* invariant: (hi == sz) || f(hi) == true
> > +        (lo == 0 && f(0) == true) || fi(lo) == false
> > +      */
>
> Comment style

Review style

Show 12 quoted lines

> > diff --git a/reftable/basics.h b/reftable/basics.h
> > new file mode 100644
> > index 00000000000..6d89eb5d931
> > --- /dev/null
> > +++ b/reftable/basics.h
> > @@ -0,0 +1,53 @@
> [..snip..]
> > +  find smallest index i in [0, sz) at which f(i) is true, assuming
> > +  that f is ascending. Return sz if f(i) is false for all indices.
> > +*/
>
> So, this is about partitioning, not searching

there is precedent for this to be called binary search. See for example, https://golang.org/pkg/sort/#Search

> > +int block_writer_add(struct block_writer *w, struct record rec)
>
> So, we're gonna pass a large structure into a function now?

The record structure contains two pointers.

> > +     if (block_writer_register_restart(w, start.len - out.len, restart,
> > +                                       key) < 0) {
>
> Eh, naming is hard, and so does this function.

I can't parse this sentence.

> It's overly verbose,
> and long. I haven't go though all of this patch (because it's too
> long), but I guess some of them can be made static and their name can
> be shortened.

For uniformity all functions taking a struct Xyz* as the first argument are called

  xyz_do_something

since this takes a block_writer, and it registers a restart, it is called block_writer_register_restart. The naming of this is uniform throughout the library.

Show 6 quoted lines

> > +             block_source_return_block(block->source, block);
> > +             block->data = uncompressed.buf;
> > +             block->len = dst_len; /* XXX: 4 bytes missing? */
>
> Even this is a very lengthy patch, some bytes is missing but we
> couldn't figured out.

good point, I'll look into this again.

> > +     {
>
> This cute curly braces is new to me

Pleased to make your acquaintance!

> > +static int restart_key_less(int idx, void *args)
>
> idx? index? Shouldn't it be size_t?

fixed this.

Show 10 quoted lines

>
> > +{
> > +     struct restart_find_args *a = (struct restart_find_args *)args;
> > +     uint32_t off = block_reader_restart_offset(a->r, idx);
> > +     struct slice in = {
> > +             .buf = a->r->block.data + off,
> > +             .len = a->r->block_len - off,
>
> C99 designated initialisation.
> We aren't ready, yet.

I'm sorry, gcc -std=c89 said this was OK.

Show 12 quoted lines

> > +void block_iter_copy_from(struct block_iter *dest, struct block_iter *src)
> > +{
> > +     dest->br = src->br;
> > +     dest->next_off = src->next_off;
> > +     slice_copy(&dest->last_key, src->last_key);
> > +}
> > +
> > +/* return < 0 for error, 0 for OK, > 0 for EOF. */
> > +int block_iter_next(struct block_iter *it, struct record rec)
>
> If this function is used only in this file, it should be static,
> otherwise, the comment should go to header file

moved comment to header file.

Show 12 quoted lines

> > diff --git a/reftable/block.h b/reftable/block.h
> > new file mode 100644
> > index 00000000000..62b2e0fec6d
> > +/*
> > +  Writes reftable blocks. The block_writer is reused across blocks to minimize
> > +  allocation overhead.
> > +*/
> > +struct block_writer {
> > +     byte *buf;
> > +     uint32_t block_size;
>
> I suppose this means 2^32 block should be enough for everyone?

No. 2^24-1 is the maximum size, as detailed in the specification.

Show 5 quoted lines

> > diff --git a/reftable/bytes.c b/reftable/bytes.c
> > new file mode 100644
> > index 00000000000..e69de29bb2d
>
> Empty?

thanks, I've removed these.

Show 9 quoted lines

> > diff --git a/reftable/config.h b/reftable/config.h
> > new file mode 100644
> > index 00000000000..40a8c178f10
> > --- /dev/null
> > +++ b/reftable/config.h
> > @@ -0,0 +1 @@
> > +/* empty */
>
> Empty?

Removed

> > +++ b/reftable/dump.c

This was moved to a different directory in upstream; I've removed it now.

Show 7 quoted lines

> > +bool iterator_is_null(struct reftable_iterator it)
> > +{
> > +     return it.ops == NULL;
> > +}
>
> Do we want this verbose?
> Or it'll be use in some kind of vtable?

I've added a doc comment.

Show 11 quoted lines

> > +
> > +static int filtering_ref_iterator_next(void *iter_arg, struct record rec)
> > +{
> > +     struct filtering_ref_iterator *fri =
> > +             (struct filtering_ref_iterator *)iter_arg;
> > +     struct reftable_ref_record *ref =
> > +             (struct reftable_ref_record *)rec.data;
>
> We're passing a pointer, references a member of a variable in local
> scope around, this will ask for trouble if callee doesn't aware about
> it, and save that pointer somewhere for later access.

I don't understand your criticism; could you be more precise?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado