Re: [PATCH v7 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

From: Mimi Zohar <zohar@linux.ibm.com>
To: Vitaly Chikunov <vt@altlinux.org>,
	Mimi Zohar <zohar@linux.vnet.ibm.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	linux-integrity@vger.kernel.org
Subject: Re: [PATCH v7 3/3] ima-evm-utils: Read keyid from the cert appended to the key file
Date: Mon, 05 Jul 2021 16:04:52 -0400	[thread overview]
Message-ID: <f304a136df93bd74e06a68737f9512e003e329ab.camel@linux.ibm.com> (raw)
In-Reply-To: <20210701011323.2377251-4-vt@altlinux.org>

Hi Vitaly,

On Thu, 2021-07-01 at 04:13 +0300, Vitaly Chikunov wrote:
> 
> +/**
> + * read_keyid_from_key() - Read 32-bit keyid from the key file
> + * @keyid_be:   Pointer to 32-bit value in network order (BE, unaligned).
> + * @keyfile:    PEM file with private key with optionally appended x509 cert.
> + * Return:      0 on success and keyid_be is written;
> + *              -1 on error, logged error message, and keyid_be isn't written.
> + */
> +static int read_keyid_from_key(uint32_t *keyid_be, const char *keyfile)

(With  the new option "--keyid-from-cert" is this patch really still
needed?)

The function name is a bit off.  Both imaevm_read_keyid() and this
function are getting the keyid from a cert.  There's also quite a bit
of code duplication between them.  Refactoring the code might help. 
For example, perhaps imaevm_read_keyid() could be a wrapper for
read_keyid_from_cert().

Otherwise renaming this function to read_keyid_from_keyfile(),
read_appended_keyid() or read_appended_keyid_from_cert(), which is
really wordy, would be better.

thanks,

Mimi