[U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes

[U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes

[Andre Przywara]

This fixes two issues I had when trying to create an envimage from a
more complex pipe:
- The read process stops when the read(2) syscall returns less bytes
  than requested.
- Specifying an input filename expects this to be a regular file.

See the respective commit messages for more details.

Thanks!
Andre

Andre Przywara (2):
  tools: mkenvimage: Fix reading from slow pipe
  tools: mkenvimage: Always consider non-regular files

 tools/mkenvimage.c | 71 ++++++++++++++++--------------------------------------
 1 file changed, 21 insertions(+), 50 deletions(-)

-- 
2.14.5

[U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe

[Andre Przywara]

It is perfectly fine for the read(2) syscall to return with less than
the requested number of bytes read (short read, see the "RETURN VALUE"
section of the man page). This typically happens with slow input
(keyboard, network) or with complex pipes.

So far mkenvimage expects the exact number of requested bytes to be
read, assuming an end-of-file condition otherwise. This wrong behaviour
can be easily shown with:
$ (echo "foo=bar"; sleep 1; echo "bar=baz") | mkenvimage -s 256 -o out -
The second line will be missing from the output.

Correct this by checking for any positive, non-zero return value.

This fixes a problem with a complex pipe in one of my scripts, where
the environment consist of two parts.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/mkenvimage.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/mkenvimage.c b/tools/mkenvimage.c
index 75967d0c2d..ffaebd5565 100644
--- a/tools/mkenvimage.c
+++ b/tools/mkenvimage.c
@@ -173,8 +173,7 @@ int main(int argc, char **argv)
 				return EXIT_FAILURE;
 			}
 			filesize += readbytes;
-		} while (readbytes == readlen);
-
+		} while (readbytes > 0);
 	} else {
 		txt_filename = argv[optind];
 		txt_fd = open(txt_filename, O_RDONLY);
-- 
2.14.5

[U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe

[Alexander Dahl]

Hello Andre,

Am Sonntag, 30. Juni 2019, 02:45:00 CEST schrieb Andre Przywara:
> It is perfectly fine for the read(2) syscall to return with less than
> the requested number of bytes read (short read, see the "RETURN VALUE"
> section of the man page). This typically happens with slow input
> (keyboard, network) or with complex pipes.
> 
> So far mkenvimage expects the exact number of requested bytes to be
> read, assuming an end-of-file condition otherwise. This wrong behaviour
> can be easily shown with:
> $ (echo "foo=bar"; sleep 1; echo "bar=baz") | mkenvimage -s 256 -o out -
> The second line will be missing from the output.
> 
> Correct this by checking for any positive, non-zero return value.
> 
> This fixes a problem with a complex pipe in one of my scripts, where
> the environment consist of two parts.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

From reading the code and 'man 2 read' again, not tested locally:

Acked-by: Alexander Dahl <ada@thorsis.com>

Greets
Alex

[U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe

[Tom Rini]

On Sun, Jun 30, 2019 at 02:45:00AM +0100, Andre Przywara wrote:

> It is perfectly fine for the read(2) syscall to return with less than
> the requested number of bytes read (short read, see the "RETURN VALUE"
> section of the man page). This typically happens with slow input
> (keyboard, network) or with complex pipes.
> 
> So far mkenvimage expects the exact number of requested bytes to be
> read, assuming an end-of-file condition otherwise. This wrong behaviour
> can be easily shown with:
> $ (echo "foo=bar"; sleep 1; echo "bar=baz") | mkenvimage -s 256 -o out -
> The second line will be missing from the output.
> 
> Correct this by checking for any positive, non-zero return value.
> 
> This fixes a problem with a complex pipe in one of my scripts, where
> the environment consist of two parts.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Alexander Dahl <ada@thorsis.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/202c2851/attachment.sig>

[U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files

[Andre Przywara]

At the moment mkenvimage has two separate read paths: One to read from
a potential pipe, while dynamically increasing the buffer size, and a
second one using mmap(2), using the input file's size. This is
problematic for two reasons:
- The "pipe" path will be chosen if the input filename is missing or
  "-".  Any named, but non-regular file will use the other path, which
  typically will cause mmap() to fail:
  $ mkenvimage -s 256 -o out <(echo "foo=bar")
- There is no reason to have *two* ways of reading a file, since the
  "pipe way" will always work, even for regular files.

Fix this (and simplify the code on the way) by always using the method
of dynamically resizing the buffer. The existing distinction between
the two cases will merely be used to use the open() syscall or not.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 tools/mkenvimage.c | 70 ++++++++++++++++--------------------------------------
 1 file changed, 21 insertions(+), 49 deletions(-)

diff --git a/tools/mkenvimage.c b/tools/mkenvimage.c
index ffaebd5565..a8eebab6c3 100644
--- a/tools/mkenvimage.c
+++ b/tools/mkenvimage.c
@@ -65,10 +65,12 @@ long int xstrtol(const char *s)
 	exit(EXIT_FAILURE);
 }
 
+#define CHUNK_SIZE 4096
+
 int main(int argc, char **argv)
 {
 	uint32_t crc, targetendian_crc;
-	const char *txt_filename = NULL, *bin_filename = NULL;
+	const char *bin_filename = NULL;
 	int txt_fd, bin_fd;
 	unsigned char *dataptr, *envptr;
 	unsigned char *filebuf = NULL;
@@ -76,12 +78,11 @@ int main(int argc, char **argv)
 	int bigendian = 0;
 	int redundant = 0;
 	unsigned char padbyte = 0xff;
+	int readbytes = 0;
 
 	int option;
 	int ret = EXIT_SUCCESS;
 
-	struct stat txt_file_stat;
-
 	int fp, ep;
 	const char *prg;
 
@@ -156,62 +157,33 @@ int main(int argc, char **argv)
 
 	/* Open the input file ... */
 	if (optind >= argc || strcmp(argv[optind], "-") == 0) {
-		int readbytes = 0;
-		int readlen = sizeof(*envptr) * 4096;
 		txt_fd = STDIN_FILENO;
-
-		do {
-			filebuf = realloc(filebuf, filesize + readlen);
-			if (!filebuf) {
-				fprintf(stderr, "Can't realloc memory for the input file buffer\n");
-				return EXIT_FAILURE;
-			}
-			readbytes = read(txt_fd, filebuf + filesize, readlen);
-			if (readbytes < 0) {
-				fprintf(stderr, "Error while reading stdin: %s\n",
-						strerror(errno));
-				return EXIT_FAILURE;
-			}
-			filesize += readbytes;
-		} while (readbytes > 0);
 	} else {
-		txt_filename = argv[optind];
-		txt_fd = open(txt_filename, O_RDONLY);
+		txt_fd = open(argv[optind], O_RDONLY);
 		if (txt_fd == -1) {
 			fprintf(stderr, "Can't open \"%s\": %s\n",
-					txt_filename, strerror(errno));
+					argv[optind], strerror(errno));
 			return EXIT_FAILURE;
 		}
-		/* ... and check it */
-		ret = fstat(txt_fd, &txt_file_stat);
-		if (ret == -1) {
-			fprintf(stderr, "Can't stat() on \"%s\": %s\n",
-					txt_filename, strerror(errno));
+	}
+
+	do {
+		filebuf = realloc(filebuf, filesize + CHUNK_SIZE);
+		if (!filebuf) {
+			fprintf(stderr, "Can't realloc memory for the input file buffer\n");
 			return EXIT_FAILURE;
 		}
-
-		filesize = txt_file_stat.st_size;
-
-		filebuf = mmap(NULL, sizeof(*envptr) * filesize, PROT_READ,
-			       MAP_PRIVATE, txt_fd, 0);
-		if (filebuf == MAP_FAILED) {
-			fprintf(stderr, "mmap (%zu bytes) failed: %s\n",
-					sizeof(*envptr) * filesize,
-					strerror(errno));
-			fprintf(stderr, "Falling back to read()\n");
-
-			filebuf = malloc(sizeof(*envptr) * filesize);
-			ret = read(txt_fd, filebuf, sizeof(*envptr) * filesize);
-			if (ret != sizeof(*envptr) * filesize) {
-				fprintf(stderr, "Can't read the whole input file (%zu bytes): %s\n",
-					sizeof(*envptr) * filesize,
-					strerror(errno));
-
-				return EXIT_FAILURE;
-			}
+		readbytes = read(txt_fd, filebuf + filesize, CHUNK_SIZE);
+		if (readbytes < 0) {
+			fprintf(stderr, "Error while reading: %s\n",
+				strerror(errno));
+			return EXIT_FAILURE;
 		}
+		filesize += readbytes;
+	} while (readbytes > 0);
+
+	if (txt_fd != STDIN_FILENO)
 		ret = close(txt_fd);
-	}
 
 	/* Parse a byte at time until reaching the file OR until the environment fills
 	 * up. Check ep against envsize - 1 to allow for extra trailing '\0'. */
-- 
2.14.5

[U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files

[Tom Rini]

On Sun, Jun 30, 2019 at 02:45:01AM +0100, Andre Przywara wrote:

> At the moment mkenvimage has two separate read paths: One to read from
> a potential pipe, while dynamically increasing the buffer size, and a
> second one using mmap(2), using the input file's size. This is
> problematic for two reasons:
> - The "pipe" path will be chosen if the input filename is missing or
>   "-".  Any named, but non-regular file will use the other path, which
>   typically will cause mmap() to fail:
>   $ mkenvimage -s 256 -o out <(echo "foo=bar")
> - There is no reason to have *two* ways of reading a file, since the
>   "pipe way" will always work, even for regular files.
> 
> Fix this (and simplify the code on the way) by always using the method
> of dynamically resizing the buffer. The existing distinction between
> the two cases will merely be used to use the open() syscall or not.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190718/af96f185/attachment-0001.sig>

[U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes

[Tom Rini]

On Sun, Jun 30, 2019 at 02:44:59AM +0100, Andre Przywara wrote:

> This fixes two issues I had when trying to create an envimage from a
> more complex pipe:
> - The read process stops when the read(2) syscall returns less bytes
>   than requested.
> - Specifying an input filename expects this to be a regular file.
> 
> See the respective commit messages for more details.
> 
> Thanks!
> Andre
> 
> Andre Przywara (2):
>   tools: mkenvimage: Fix reading from slow pipe
>   tools: mkenvimage: Always consider non-regular files
> 
>  tools/mkenvimage.c | 71 ++++++++++++++++--------------------------------------
>  1 file changed, 21 insertions(+), 50 deletions(-)

These all look fine but in the interest of avoiding unintended
consequences I'm going to pull these in after v2019.07 as they're
long-standing bugs rather than regressions being fixed, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190630/a2f17ee2/attachment.sig>

[U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes

[Andre Przywara]

On Sun, 30 Jun 2019 10:40:44 -0400
Tom Rini <trini@konsulko.com> wrote:

Hi Tom,

> On Sun, Jun 30, 2019 at 02:44:59AM +0100, Andre Przywara wrote:
> 
> > This fixes two issues I had when trying to create an envimage from a
> > more complex pipe:
> > - The read process stops when the read(2) syscall returns less bytes
> >   than requested.
> > - Specifying an input filename expects this to be a regular file.
> > 
> > See the respective commit messages for more details.
> > 
> > Thanks!
> > Andre
> > 
> > Andre Przywara (2):
> >   tools: mkenvimage: Fix reading from slow pipe
> >   tools: mkenvimage: Always consider non-regular files
> > 
> >  tools/mkenvimage.c | 71 ++++++++++++++++--------------------------------------
> >  1 file changed, 21 insertions(+), 50 deletions(-)  
> 
> These all look fine but in the interest of avoiding unintended
> consequences I'm going to pull these in after v2019.07 as they're
> long-standing bugs rather than regressions being fixed, thanks!

Thanks, that was my intention anyway.

So is the development model now "only send new changes/features in the merge window"? I was suspecting that with this new long stabilisation phase we prepare patches outside of the merge window?
Or was the "Fixes ..." in the subject line giving the wrong impression of my intention?

Cheers,
Andre.

[U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes

[Tom Rini]

On Mon, Jul 01, 2019 at 10:22:46AM +0100, Andre Przywara wrote:
> On Sun, 30 Jun 2019 10:40:44 -0400
> Tom Rini <trini@konsulko.com> wrote:
> 
> Hi Tom,
> 
> > On Sun, Jun 30, 2019 at 02:44:59AM +0100, Andre Przywara wrote:
> > 
> > > This fixes two issues I had when trying to create an envimage from a
> > > more complex pipe:
> > > - The read process stops when the read(2) syscall returns less bytes
> > >   than requested.
> > > - Specifying an input filename expects this to be a regular file.
> > > 
> > > See the respective commit messages for more details.
> > > 
> > > Thanks!
> > > Andre
> > > 
> > > Andre Przywara (2):
> > >   tools: mkenvimage: Fix reading from slow pipe
> > >   tools: mkenvimage: Always consider non-regular files
> > > 
> > >  tools/mkenvimage.c | 71 ++++++++++++++++--------------------------------------
> > >  1 file changed, 21 insertions(+), 50 deletions(-)  
> > 
> > These all look fine but in the interest of avoiding unintended
> > consequences I'm going to pull these in after v2019.07 as they're
> > long-standing bugs rather than regressions being fixed, thanks!
> 
> Thanks, that was my intention anyway.
> 
> So is the development model now "only send new changes/features in the
> merge window"? I was suspecting that with this new long stabilisation
> phase we prepare patches outside of the merge window?
> Or was the "Fixes ..." in the subject line giving the wrong impression
> of my intention?

New changes / features should still be posted when they're ready for
public review, to get the most possible review done to them.  I just
figured that since this was a series that was fixing a problem I should
give it a look and since I could see an argument in favor of merging
them now, say why I wasn't.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190701/1d9b6087/attachment.sig>