* [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes
@ 2019-06-30 1:44 Andre Przywara
2019-06-30 1:45 ` [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe Andre Przywara
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andre Przywara @ 2019-06-30 1:44 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe
2019-06-30 1:44 [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Andre Przywara
@ 2019-06-30 1:45 ` Andre Przywara
2019-07-01 6:31 ` Alexander Dahl
2019-07-18 23:57 ` Tom Rini
2019-06-30 1:45 ` [U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files Andre Przywara
2019-06-30 14:40 ` [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Tom Rini
2 siblings, 2 replies; 9+ messages in thread
From: Andre Przywara @ 2019-06-30 1:45 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files
2019-06-30 1:44 [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Andre Przywara
2019-06-30 1:45 ` [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe Andre Przywara
@ 2019-06-30 1:45 ` Andre Przywara
2019-07-18 23:57 ` Tom Rini
2019-06-30 14:40 ` [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Tom Rini
2 siblings, 1 reply; 9+ messages in thread
From: Andre Przywara @ 2019-06-30 1:45 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes
2019-06-30 1:44 [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Andre Przywara
2019-06-30 1:45 ` [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe Andre Przywara
2019-06-30 1:45 ` [U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files Andre Przywara
@ 2019-06-30 14:40 ` Tom Rini
2019-07-01 9:22 ` Andre Przywara
2 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2019-06-30 14:40 UTC (permalink / raw
To: u-boot
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe
2019-06-30 1:45 ` [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe Andre Przywara
@ 2019-07-01 6:31 ` Alexander Dahl
2019-07-18 23:57 ` Tom Rini
1 sibling, 0 replies; 9+ messages in thread
From: Alexander Dahl @ 2019-07-01 6:31 UTC (permalink / raw
To: u-boot
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes
2019-06-30 14:40 ` [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Tom Rini
@ 2019-07-01 9:22 ` Andre Przywara
2019-07-01 13:23 ` Tom Rini
0 siblings, 1 reply; 9+ messages in thread
From: Andre Przywara @ 2019-07-01 9:22 UTC (permalink / raw
To: u-boot
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes
2019-07-01 9:22 ` Andre Przywara
@ 2019-07-01 13:23 ` Tom Rini
0 siblings, 0 replies; 9+ messages in thread
From: Tom Rini @ 2019-07-01 13:23 UTC (permalink / raw
To: u-boot
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe
2019-06-30 1:45 ` [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe Andre Przywara
2019-07-01 6:31 ` Alexander Dahl
@ 2019-07-18 23:57 ` Tom Rini
1 sibling, 0 replies; 9+ messages in thread
From: Tom Rini @ 2019-07-18 23:57 UTC (permalink / raw
To: u-boot
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files
2019-06-30 1:45 ` [U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files Andre Przywara
@ 2019-07-18 23:57 ` Tom Rini
0 siblings, 0 replies; 9+ messages in thread
From: Tom Rini @ 2019-07-18 23:57 UTC (permalink / raw
To: u-boot
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-18 23:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-30 1:44 [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Andre Przywara
2019-06-30 1:45 ` [U-Boot] [PATCH 1/2] tools: mkenvimage: Fix reading from slow pipe Andre Przywara
2019-07-01 6:31 ` Alexander Dahl
2019-07-18 23:57 ` Tom Rini
2019-06-30 1:45 ` [U-Boot] [PATCH 2/2] tools: mkenvimage: Always consider non-regular files Andre Przywara
2019-07-18 23:57 ` Tom Rini
2019-06-30 14:40 ` [U-Boot] [PATCH 0/2] tools: mkenvimage: Fixes for reading from pipes Tom Rini
2019-07-01 9:22 ` Andre Przywara
2019-07-01 13:23 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.