* [PATCH] libfile: copy_file: prevent spurious error message @ 2019-10-15 15:59 Robert Karszniewicz 2019-10-16 11:26 ` [PATCH v2] " Robert Karszniewicz 0 siblings, 1 reply; 8+ messages in thread From: Robert Karszniewicz @ 2019-10-15 15:59 UTC (permalink / raw) To: barebox Signed-off-by: Robert Karszniewicz <r.karszniewicz@phytec.de> --- lib/libfile.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/libfile.c b/lib/libfile.c index 3f3ec21..319e66b 100644 --- a/lib/libfile.c +++ b/lib/libfile.c @@ -349,6 +349,7 @@ int copy_file(const char *src, const char *dst, int verbose) ret = stat(dst, &dststat); if (ret && ret != -ENOENT) goto out; + ret = 0; /* Set O_TRUNC only if file exist and is a regular file */ if (!ret && S_ISREG(dststat.st_mode)) -- 2.7.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] libfile: copy_file: prevent spurious error message 2019-10-15 15:59 [PATCH] libfile: copy_file: prevent spurious error message Robert Karszniewicz @ 2019-10-16 11:26 ` Robert Karszniewicz 2019-10-18 11:45 ` Sascha Hauer 0 siblings, 1 reply; 8+ messages in thread From: Robert Karszniewicz @ 2019-10-16 11:26 UTC (permalink / raw) To: barebox Before this, if the function bails out somewhere at a later point, this return value will be outdated and will produce a misleading error message down the line. Signed-off-by: Robert Karszniewicz <r.karszniewicz@phytec.de> --- Changes from v1: - commit message lib/libfile.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/libfile.c b/lib/libfile.c index 3f3ec21..319e66b 100644 --- a/lib/libfile.c +++ b/lib/libfile.c @@ -349,6 +349,7 @@ int copy_file(const char *src, const char *dst, int verbose) ret = stat(dst, &dststat); if (ret && ret != -ENOENT) goto out; + ret = 0; /* Set O_TRUNC only if file exist and is a regular file */ if (!ret && S_ISREG(dststat.st_mode)) -- 2.7.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] libfile: copy_file: prevent spurious error message 2019-10-16 11:26 ` [PATCH v2] " Robert Karszniewicz @ 2019-10-18 11:45 ` Sascha Hauer 2019-10-18 17:10 ` [PATCH v3] " Robert Karszniewicz 0 siblings, 1 reply; 8+ messages in thread From: Sascha Hauer @ 2019-10-18 11:45 UTC (permalink / raw) To: Robert Karszniewicz; +Cc: barebox Hi Robert, On Wed, Oct 16, 2019 at 01:26:06PM +0200, Robert Karszniewicz wrote: > Before this, if the function bails out somewhere at a later point, this > return value will be outdated and will produce a misleading error > message down the line. > > Signed-off-by: Robert Karszniewicz <r.karszniewicz@phytec.de> > --- > Changes from v1: > - commit message > > lib/libfile.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/libfile.c b/lib/libfile.c > index 3f3ec21..319e66b 100644 > --- a/lib/libfile.c > +++ b/lib/libfile.c > @@ -349,6 +349,7 @@ int copy_file(const char *src, const char *dst, int verbose) > ret = stat(dst, &dststat); > if (ret && ret != -ENOENT) > goto out; > + ret = 0; Setting ret = 0 here is wrong as we test for ret one line further: > > /* Set O_TRUNC only if file exist and is a regular file */ > if (!ret && S_ISREG(dststat.st_mode)) Anyway, it seems the error handling in copy_file is broken since 0ec6bd3e1b ("libfile: copy_file: Only open regular files with O_TRUNC"). Before that 'ret' was initialized to 1 and only when all was done it was set to 0 and returned. After this commit 'ret' was set to the return value of the stat() call and accidently returned after each 'goto out'. 'ret' should be set explicitly to the desired return value right before each 'goto out'. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] libfile: copy_file: prevent spurious error message 2019-10-18 11:45 ` Sascha Hauer @ 2019-10-18 17:10 ` Robert Karszniewicz 2019-10-18 17:15 ` Robert Karszniewicz 0 siblings, 1 reply; 8+ messages in thread From: Robert Karszniewicz @ 2019-10-18 17:10 UTC (permalink / raw) To: barebox Before this, ret was falsely polluted, which caused a misleading error message if the function bailed out at a later point. Signed-off-by: Robert Karszniewicz <r.karszniewicz@phytec.de> --- lib/libfile.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/libfile.c b/lib/libfile.c index 3f3ec21..fe11d34 100644 --- a/lib/libfile.c +++ b/lib/libfile.c @@ -330,7 +330,7 @@ int copy_file(const char *src, const char *dst, int verbose) { char *rw_buf = NULL; int srcfd = 0, dstfd = 0; - int r; + int r, s; int ret = 1, err1 = 0; int mode; int total = 0; @@ -346,12 +346,12 @@ int copy_file(const char *src, const char *dst, int verbose) mode = O_WRONLY | O_CREAT; - ret = stat(dst, &dststat); - if (ret && ret != -ENOENT) + s = stat(dst, &dststat); + if (s && s != -ENOENT) goto out; /* Set O_TRUNC only if file exist and is a regular file */ - if (!ret && S_ISREG(dststat.st_mode)) + if (!s && S_ISREG(dststat.st_mode)) mode |= O_TRUNC; dstfd = open(dst, mode); -- 2.7.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] libfile: copy_file: prevent spurious error message 2019-10-18 17:10 ` [PATCH v3] " Robert Karszniewicz @ 2019-10-18 17:15 ` Robert Karszniewicz 2019-10-21 7:37 ` Sascha Hauer 0 siblings, 1 reply; 8+ messages in thread From: Robert Karszniewicz @ 2019-10-18 17:15 UTC (permalink / raw) To: barebox In all this haste, I forgot my question. What error codes should be used? For each goto its own code, in sequence, starting from -1? _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] libfile: copy_file: prevent spurious error message 2019-10-18 17:15 ` Robert Karszniewicz @ 2019-10-21 7:37 ` Sascha Hauer 2019-10-22 13:47 ` [PATCH v4] libfile: copy_file: fix error handling Robert Karszniewicz 0 siblings, 1 reply; 8+ messages in thread From: Sascha Hauer @ 2019-10-21 7:37 UTC (permalink / raw) To: Robert Karszniewicz; +Cc: barebox On Fri, Oct 18, 2019 at 07:15:27PM +0200, Robert Karszniewicz wrote: > In all this haste, I forgot my question. > > What error codes should be used? For each goto its own code, in sequence, > starting from -1? Please propagate the error code returned from the call that failed. i.e. something like: r = read(srcfd, rw_buf, RW_BUF_SIZE); if (r < 0) { perror("read"); ret = r; goto out; } Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4] libfile: copy_file: fix error handling 2019-10-21 7:37 ` Sascha Hauer @ 2019-10-22 13:47 ` Robert Karszniewicz 2019-10-23 7:16 ` Sascha Hauer 0 siblings, 1 reply; 8+ messages in thread From: Robert Karszniewicz @ 2019-10-22 13:47 UTC (permalink / raw) To: barebox Before this, ret was falsely polluted, which caused a misleading error message if the function bailed out at a later point. Signed-off-by: Robert Karszniewicz <r.karszniewicz@phytec.de> --- lib/libfile.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/libfile.c b/lib/libfile.c index 3f3ec21..7bade39 100644 --- a/lib/libfile.c +++ b/lib/libfile.c @@ -330,7 +330,7 @@ int copy_file(const char *src, const char *dst, int verbose) { char *rw_buf = NULL; int srcfd = 0, dstfd = 0; - int r; + int r, s; int ret = 1, err1 = 0; int mode; int total = 0; @@ -341,22 +341,27 @@ int copy_file(const char *src, const char *dst, int verbose) srcfd = open(src, O_RDONLY); if (srcfd < 0) { printf("could not open %s: %s\n", src, errno_str()); + ret = srcfd; goto out; } mode = O_WRONLY | O_CREAT; - ret = stat(dst, &dststat); - if (ret && ret != -ENOENT) + s = stat(dst, &dststat); + if (s && s != -ENOENT) { + printf("could not stat %s: %s\n", dst, errno_str()); + ret = s; goto out; + } /* Set O_TRUNC only if file exist and is a regular file */ - if (!ret && S_ISREG(dststat.st_mode)) + if (!s && S_ISREG(dststat.st_mode)) mode |= O_TRUNC; dstfd = open(dst, mode); if (dstfd < 0) { printf("could not open %s: %s\n", dst, errno_str()); + ret = dstfd; goto out; } @@ -371,12 +376,14 @@ int copy_file(const char *src, const char *dst, int verbose) r = read(srcfd, rw_buf, RW_BUF_SIZE); if (r < 0) { perror("read"); + ret = r; goto out; } if (!r) break; - if (write_full(dstfd, rw_buf, r) < 0) { + ret = write_full(dstfd, rw_buf, r); + if (ret < 0) { perror("write"); goto out; } -- 2.7.4 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] libfile: copy_file: fix error handling 2019-10-22 13:47 ` [PATCH v4] libfile: copy_file: fix error handling Robert Karszniewicz @ 2019-10-23 7:16 ` Sascha Hauer 0 siblings, 0 replies; 8+ messages in thread From: Sascha Hauer @ 2019-10-23 7:16 UTC (permalink / raw) To: Robert Karszniewicz; +Cc: barebox On Tue, Oct 22, 2019 at 03:47:32PM +0200, Robert Karszniewicz wrote: > Before this, ret was falsely polluted, which caused a misleading error > message if the function bailed out at a later point. > > Signed-off-by: Robert Karszniewicz <r.karszniewicz@phytec.de> > --- > lib/libfile.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) Applied, thanks Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-23 7:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-15 15:59 [PATCH] libfile: copy_file: prevent spurious error message Robert Karszniewicz 2019-10-16 11:26 ` [PATCH v2] " Robert Karszniewicz 2019-10-18 11:45 ` Sascha Hauer 2019-10-18 17:10 ` [PATCH v3] " Robert Karszniewicz 2019-10-18 17:15 ` Robert Karszniewicz 2019-10-21 7:37 ` Sascha Hauer 2019-10-22 13:47 ` [PATCH v4] libfile: copy_file: fix error handling Robert Karszniewicz 2019-10-23 7:16 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox