On Thu, May 21, 2020 at 12:45:57PM -0400, Jes Sorensen wrote:
> >> My biggest objection is the export of kernel datatypes to userland and I
> >> really don't think using u32/u16/u8 internally in the library adds any
> >> value. I had explicitly converted it to uint32_t/uint16_t/uint8_t in my
> >> version.
> >
> > I prefer u64/u32/u16/u8 since they're shorter and easier to read, and it's the
> > same convention that is used in the kernel code (which is where the other half
> > of fs-verity is).
>
> I like them too, but I tend to live in kernel space.
>
> > Note that these types aren't "exported" to or from anywhere but rather are just
> > typedefs in common/common_defs.h. It's just a particular convention.
> >
> > Also, fsverity-utils is already using this convention prior to this patchset.
> > If we did decide to change it, then we should change it in all the code, not
> > just in certain places.
>
> I thought I did it everywhere in my patch set?
No, they were still left in various places.
>
> >> I also wonder if we should introduce an
> >> libfsverity_get_digest_size(alg_nr) function? It would be useful for a
> >> caller trying to allocate buffers to store them in, to be able to do
> >> this prior to calculating the first digest.
> >
> > That already exists; it's called libfsverity_digest_size().
> >
> > Would it be clearer if we renamed:
> >
> > libfsverity_digest_size() => libfsverity_get_digest_size()
> > libfsverity_hash_name() => libfsverity_get_hash_name()
>
> Oh I missed you added that. Probably a good idea to rename them for
> consistency.
libfsverity_digest_size() was actually in your patchset too.
I'll add the "get" to the names so that all function names start with a verb.
> >>> +static void *xmalloc(size_t size)
> >>> +{
> >>> + void *p = malloc(size);
> >>> +
> >>> + if (!p)
> >>> + libfsverity_error_msg("out of memory");
> >>> + return p;
> >>> +}
> >>> +
> >>> +void *libfsverity_zalloc(size_t size)
> >>> +{
> >>> + void *p = xmalloc(size);
> >>> +
> >>> + if (!p)
> >>> + return NULL;
> >>> + return memset(p, 0, size);
> >>> +}
> >>
> >> I suggest we get rid of xmalloc() and libfsverity_zalloc(). libc
> >> provides perfectly good malloc() and calloc() functions, and printing an
> >> out of memory error in a generic location doesn't tell us where the
> >> error happened. If anything those error messages should be printed by
> >> the calling function IMO.
> >>
> >
> > Maybe. I'm not sure knowing the specific allocation sites would be useful
> > enough to make all the callers handle printing the error message (which is
> > easily forgotten about). We could also add the allocation size that failed to
> > the message here.
>
> My point is mostly at this point, this just adds code obfuscation rather
> than adding real value. I can see how it was useful during initial
> development.
It's helpful to eliminate the need for callers to print the error message.
We also still need libfsverity_memdup() anyway, unless we hard-code
malloc() + memcpy().
I also had in mind that we'd follow the (increasingly recommended) practice of
initializing all heap memory. This can be done by only providing allocation
functions that initialize memory.
I'll think about it.
- Eric
|