Re: [PATCH 2/3] Introduce libfsverity

[Jes Sorensen <jes.sorensen@gmail.com>]

On 5/21/20 12:59 PM, Eric Biggers wrote:
> 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 see, I thought I had only left the __u32 ones in place, but just goes
to show I didn't do my job properly.

>>>> 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.

Whoops egg on face :)

> I'll add the "get" to the names so that all function names start with a verb.

Sounds good!

>>>>> +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.

It's not a showstopper, my primary interest is a working release :)

Cheers,
Jes