[PATCH] libfile: copy_file: prevent spurious error message

[PATCH] libfile: copy_file: prevent spurious error message

[Robert Karszniewicz]

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

[PATCH v2] libfile: copy_file: prevent spurious error message

[Robert Karszniewicz]

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

Re: [PATCH v2] libfile: copy_file: prevent spurious error message

[Sascha Hauer]

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

[PATCH v3] libfile: copy_file: prevent spurious error message

[Robert Karszniewicz]

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

Re: [PATCH v3] libfile: copy_file: prevent spurious error message

[Robert Karszniewicz]

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

Re: [PATCH v3] libfile: copy_file: prevent spurious error message

[Sascha Hauer]

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

[PATCH v4] libfile: copy_file: fix error handling

[Robert Karszniewicz]

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

Re: [PATCH v4] libfile: copy_file: fix error handling

[Sascha Hauer]

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