[U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes
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(-)
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);
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
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!
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'. */
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!
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!
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.
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.
participants (3)
-
Alexander Dahl -
Andre Przywara -
Tom Rini