[libvirt] [PATCH v3 5/6] virFileInData: preserve errno in cleanup path

[John Ferlan]

On 06/05/2017 04:23 AM, Michal Privoznik wrote:
> Callers might be interested in the original value of errno. Let's
> not overwrite it with lseek() done in cleanup path.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/util/virfile.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

One could argue that if the original lseek fails, rather than the goto
cleanup, the return -1 could be done.  That way, the "if" condition in
cleanup: isn't necessary...

Additionally, if the second if the second lseek fails to reposition back
to where we started and we don't tell the user that, then we're off in
never never land possibly, right?

Of course IIRC the argument about this was that lseek is highly unlikely
to fail in that second scenario.

Up to this point we don't say/guarantee anything about the errno return
value generally, but I see the subsequent patch adds some verbiage -
whether it can be accurately held over any libvirt call perhaps remains
to be seen.

Still the premise of this is, don't mess with an errno that was set to
something with a second one that could change the original intention.

Before the ACK/R-B applies, is there any odd special casing that needs
to be done for that ENXIO case?  That is setting errno = 0 if we decide
to not goto cleanup as I don't think in that case we're considering an
errno value to be problematic, instead it'd be expected.

As long as the errno != ENXIO case is handled... Since we learned
nothing and are moving on happily.

Reviewed-by: John Ferlan <jferlan at redhat.com>


John

> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index d444b32f8..2be64f1db 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.cf
> @@ -3900,8 +3900,11 @@ virFileInData(int fd,
>      ret = 0;
>   cleanup:
>      /* At any rate, reposition back to where we started. */
> -    if (cur != (off_t) -1)
> +    if (cur != (off_t) -1) {
> +        int save_errno = errno;
>          ignore_value(lseek(fd, cur, SEEK_SET));
> +        errno = save_errno;
> +    }
>      return ret;
>  }
>  
>