[libvirt] [PATCH] virFileInData: Preserve errno on error

Michal Privoznik posted 1 patch 5 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/3a2b5abade4864f0384e63f742dfa8c442e1ebb0.1539875553.git.mprivozn@redhat.com
Test syntax-check passed
src/util/virfile.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
[libvirt] [PATCH] virFileInData: Preserve errno on error
Posted by Michal Privoznik 5 years, 5 months ago
The virFileInData() function should return to the caller if the
current position the passed file is in is a data section or a
hole (and also how long the current section is). At any rate,
upon return from this function (be it successful or not) the
original position in the file is restored. This may mess up with
errno which might have been set earlier. Save the errno into a
local variable so it can be restored for the caller's sake.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virfile.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index e09992e41a..f6f01ec1e1 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4072,11 +4072,18 @@ virFileInData(int fd,
     ret = 0;
  cleanup:
     /* At any rate, reposition back to where we started. */
-    if (cur != (off_t) -1 &&
-        lseek(fd, cur, SEEK_SET) == (off_t) -1) {
-        virReportSystemError(errno, "%s",
-                             _("unable to restore position in file"));
-        ret = -1;
+    if (cur != (off_t) -1) {
+        int theerrno = errno;
+
+        if (lseek(fd, cur, SEEK_SET) == (off_t) -1) {
+            virReportSystemError(errno, "%s",
+                                 _("unable to restore position in file"));
+            ret = -1;
+            if (theerrno == 0)
+                theerrno = errno;
+        }
+
+        errno = theerrno;
     }
     return ret;
 }
-- 
2.18.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virFileInData: Preserve errno on error
Posted by Martin Kletzander 5 years, 5 months ago
On Thu, Oct 18, 2018 at 05:12:33PM +0200, Michal Privoznik wrote:
>The virFileInData() function should return to the caller if the
>current position the passed file is in is a data section or a
>hole (and also how long the current section is). At any rate,
>upon return from this function (be it successful or not) the
>original position in the file is restored. This may mess up with
>errno which might have been set earlier. Save the errno into a
>local variable so it can be restored for the caller's sake.
>

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/util/virfile.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index e09992e41a..f6f01ec1e1 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -4072,11 +4072,18 @@ virFileInData(int fd,
>     ret = 0;
>  cleanup:
>     /* At any rate, reposition back to where we started. */
>-    if (cur != (off_t) -1 &&
>-        lseek(fd, cur, SEEK_SET) == (off_t) -1) {
>-        virReportSystemError(errno, "%s",
>-                             _("unable to restore position in file"));
>-        ret = -1;
>+    if (cur != (off_t) -1) {
>+        int theerrno = errno;
>+
>+        if (lseek(fd, cur, SEEK_SET) == (off_t) -1) {
>+            virReportSystemError(errno, "%s",
>+                                 _("unable to restore position in file"));
>+            ret = -1;
>+            if (theerrno == 0)
>+                theerrno = errno;
>+        }
>+
>+        errno = theerrno;
>     }
>     return ret;
> }
>-- 
>2.18.1
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virFileInData: Preserve errno on error
Posted by Erik Skultety 5 years, 5 months ago
On Thu, Oct 18, 2018 at 05:12:33PM +0200, Michal Privoznik wrote:
> The virFileInData() function should return to the caller if the
> current position the passed file is in is a data section or a
> hole (and also how long the current section is). At any rate,
> upon return from this function (be it successful or not) the
> original position in the file is restored. This may mess up with
> errno which might have been set earlier. Save the errno into a
> local variable so it can be restored for the caller's sake.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/util/virfile.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index e09992e41a..f6f01ec1e1 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -4072,11 +4072,18 @@ virFileInData(int fd,
>      ret = 0;
>   cleanup:
>      /* At any rate, reposition back to where we started. */
> -    if (cur != (off_t) -1 &&
> -        lseek(fd, cur, SEEK_SET) == (off_t) -1) {
> -        virReportSystemError(errno, "%s",
> -                             _("unable to restore position in file"));
> -        ret = -1;
> +    if (cur != (off_t) -1) {
> +        int theerrno = errno;

quick grepping through the code shows that we're fairly consistent
(surprisingly) in naming here and call ^this save_errno instead.

> +
> +        if (lseek(fd, cur, SEEK_SET) == (off_t) -1) {
> +            virReportSystemError(errno, "%s",
> +                                 _("unable to restore position in file"));
> +            ret = -1;


> +            if (theerrno == 0)
> +                theerrno = errno;

I'm not entirely sold on this condition, we use the errno produced by this
"re-wind" piece of code only if no other errno has been produced until this
point - feels like we should just throw this one away every time, but that's
just the feeling I get, not that it would matter much.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list