Re: [libvirt] [PATCH v4 5/6] virFileInData: preserve errno in cleanup path
On 06/22/2017 08:30 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(a)redhat.com>
---
src/util/virfile.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Looks like I did a lot of writing in my response to the previous
version. Still not clear what value this is as it's been pointed out
previously that lseek shouldn't fail and it had to have succeeded at
least once (since "cur != -1") and maybe failed at least once (each of
those getting a ReportSystemError).
So what value is there in saving errno which may not even be used when
ret = 0? I don't even see it used when ret = -1. The one thing that does
happen is saving the errmsg from patch 2, but nothing w/ errno.
IMO, I think failing that last lseek() should cause failure since the
API hasn't truthfully represented what it does ("Upon its return, the
position in the @fd is left unchanged,..."). If that last lseek() fails
and we return 0, then the caller would be assuming we're back at the
same position we started, but we're not. But I think I stated that a
long time ago when the code was first being added and you convinced me
otherwise!
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.c
@@ -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;
}