On Fri, Nov 04, 2022 at 01:24:21AM +0300, Vitaly Chikunov wrote:
> 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_,
It "is not consistent" in the sense that error in the API call does not
have always errno set. And this is unlike we have for common libc API
calls where errno is defined. (As a consequence we cannot just add
strerror() to the printing errors from these APIs).
But, it's not necessarily there is standard or common practice about
this matter.
Thanks,
> 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,
