On Fri, Nov 04, 2022 at 01:05:31AM +0300, Vitaly Chikunov wrote:
> Mimi,
>
> On Thu, Nov 03, 2022 at 02:38:49PM -0400, Mimi Zohar wrote:
> > Define a log_errno_reset macro to emit the errno string at or near the
> > time of error, similar to the existing log_errno macro, but also reset
> > errno to avoid dangling or duplicate errno messages on exit.
> >
> > The initial usage is for non-critical file open failures.
> >
> > Suggested-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
> > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > ---
> > src/evmctl.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
>
> Reviewed-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
>
> >
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 0412bc0ac2b0..54123bf20f03 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file)
> > int err_padded = -1;
> > int err = -1;
> >
> > - errno = 0;
> > memset(zero, 0, MAX_DIGEST_SIZE);
> >
> > pseudo_padded_banks = init_tpm_banks(&num_banks);
> > @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file)
> > init_public_keys(imaevm_params.keyfile);
> > else /* assume read pubkey from x509 cert */
> > init_public_keys("/etc/keys/x509_evm.der");
> > + if (errno)
> > + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys");
>
> Library prints appropriate error messages, so this is perhaps just to
> clear errno. But it's not necessarily completely failed, but maybe
> failure in one key. So I would say "Failure in initializing public
> keys" to be precise.
>
> ps.
>
> BTW, init_public_keys API call cannot return error except by errno,
> but it does not set it consistently so some errors may be missed.
>
> init_public_keys loops calling read_pub_pkey
>
> entry->key = read_pub_pkey(keyfile, 1);
> if (!entry->key) {
> free(entry);
> continue;
> }
>
> and read_pub_pkey have such code:
>
> if (!keyfile)
> return NULL;
>
> In that case some key is not read but we don't get any error notification.
>
> I think it's legal, by the right of being library, so set `errno =
> EINVAL` there somewhere. But, I'm not sure where - as we should not
> clobber existing errno values. Perhaps, errno setting should be added to
> libimaevm consistently to all functions, but this is huge task, so I
> would not suggest to do it now. Just suggestion for the future
> developments, maybe.
Just to compare with other library - libtracefs sets errno _sometimes_,
for example, their API call tracefs_dynevent_get have:
struct tracefs_dynevent *
tracefs_dynevent_get(enum tracefs_dynevent_type type, const char *system,
const char *event)
{
...
if (!event) {
errno = -EINVAL;
return NULL;
}
count = get_all_dynevents(type, system, &events);
if (count <= 0)
return NULL;
...
So it sets errno sometimes, but not always, which I am not sure is correct
approach. This needs to be discussed more with library experts.
Thanks,
