On Thu, 2022-09-15 at 18:36 +0300, Vitaly Chikunov wrote:
> On Thu, Sep 15, 2022 at 07:58:51AM -0400, Mimi Zohar wrote:
> > Hi Vitaly,
> >
> > Thank you for this and the other reviews. They'll be addressed in the
> > next version.
> >
> > On Wed, 2022-09-14 at 17:51 +0300, Vitaly Chikunov wrote:
> > > > @@ -350,12 +356,11 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
> > > > #if OPENSSL_VERSION_NUMBER < 0x10100000
> > > > EVP_MD_CTX ctx;
> > > > pctx = &ctx;
> > > > -#else
> > > > - pctx = EVP_MD_CTX_new();
> > > > #endif
> > > >
> > > > if (lstat(file, &st)) {
> > > > log_err("Failed to stat: %s\n", file);
> > > > + errno = 0;
> > >
> > > Why it clears errno (here and below)?
> > >
> > > errno(3) says "The value of errno is never set to zero by any system
> > > call or library function."
> >
> > evmctl, itself, is not a system call or a library function.
>
> Ah, I wasn't attentive this is only evmctl. [But there's similar commit
> acb19d1 ("Reset 'errno' after failure to open or access a file")
> changing libimaevm.c which is wrong.]
>
> Perhaps it should be noted in commit message that errno is cleared
> because it's error message is already printed to avoid double printing
> at exit of cmd handler.
>
> > Is this a
> > generic statement or here in particular as to how evmctl should handle
> > failed system calls? Another example is reading the keyfile. The
> > existence of which is not required.
>
> log_err("Failed to stat: %s\n", file);
>
> This does not even output errno code, but it could be very informative
> to user. I think it's better to print (at least errno or) strerror for
> users there (and on other syscall errors log_err instances.
>
> Maybe to add special log function (like log_strerr) just for evmctl
> which will print (non "\n"-terminated) error message (similar to
> warn(3)) with strerror output appended (and commented in the code why
> it) clears errno (so that later handlers do not print it again).
>
> ps. About libimaevm.c--I think errno should not be touched there as this
> breaks what coders expect from libraries. If this affects exit of evmctl
> then it should be handled in evmctl, not in the library. (Of course it's
> better to add strerror(errno) to log_err there too, but not by the
> proposed above function.)
Thank you for the suggestions. log_errno() is already defined. Not
sure how I missed that. So use log_errno() in libimaevm.c. For
evmctl.c, define a wrapper named log_and_reset_errno(), with your
suggested patch description.
Since none of the changes in next-testing have been released, they can
still be fixed/squashed as needed.
thanks!
Mimi
