Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
From: Jes Sorensen <jes.sorensen@gmail.com>
To: Eric Biggers <ebiggers@kernel.org>
Cc: linux-fscrypt@vger.kernel.org, jsorensen@fb.com, kernel-team@fb.com
Subject: Re: [PATCH 2/3] Introduce libfsverity
Date: Thu, 21 May 2020 13:13:15 -0400
Message-ID: <0079bdb8-dc8b-bd3e-718a-b994602e9a07@gmail.com> (raw)
In-Reply-To: <20200521165941.GB12790@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



  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15  4:10 [PATCH 0/3] fsverity-utils: introduce libfsverity Eric Biggers
2020-05-15  4:10 ` [PATCH 1/3] Split up cmd_sign.c Eric Biggers
2020-05-21 15:26   ` Jes Sorensen
2020-05-15  4:10 ` [PATCH 2/3] Introduce libfsverity Eric Biggers
2020-05-21 15:24   ` Jes Sorensen
2020-05-21 16:08     ` Eric Biggers
2020-05-21 16:45       ` Jes Sorensen
2020-05-21 16:59         ` Eric Biggers
2020-05-21 17:13           ` Jes Sorensen [this message]
2020-05-15  4:10 ` [PATCH 3/3] Add some basic test programs for libfsverity Eric Biggers
2020-05-21 15:29   ` Jes Sorensen
2020-05-15 20:50 ` [PATCH 0/3] fsverity-utils: introduce libfsverity Jes Sorensen
2020-05-20  3:06   ` Eric Biggers
2020-05-20 13:26     ` Jes Sorensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0079bdb8-dc8b-bd3e-718a-b994602e9a07@gmail.com \
    --to=jes.sorensen@gmail.com \
    --cc=ebiggers@kernel.org \
    --cc=jsorensen@fb.com \
    --cc=kernel-team@fb.com \
    --cc=linux-fscrypt@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git