Re: [PATCH v1 1/1] mkimage: Fix error message if write less data then expected
Hello, On 7/7/20 3:40 PM, Walter Lozano wrote: Hi Mylene, On 7/7/20 05:25, Mylene Josserand wrote: Hi, On 6/9/20 8:49 PM, Walter Lozano wrote: Hi Mylene, Thanks for you patch. On 5/6/20 05:01, Mylène Josserand wrote: Add a new error message in case the size of data written are shorter than the one expected. Currently, it will lead to the following error message: "mkimage: Write error on uImage: Success" This is not explicit when the error is because the device doesn't have enough space. Let's use a "No space left on the device" error message in that case. Signed-off-by: Mylène Josserand --- tools/mkimage.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/mkimage.c b/tools/mkimage.c index d2cd1917874..ef0cc889a92 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -674,7 +674,7 @@ copy_file (int ifd, const char *datafile, int pad) int zero = 0; uint8_t zeros[4096]; int offset = 0; - int size; + int size, ret; struct image_type_params *tparams = imagetool_get_type(params.type); memset(zeros, 0, sizeof(zeros)); @@ -730,9 +730,15 @@ copy_file (int ifd, const char *datafile, int pad) } size = sbuf.st_size - offset; - if (write(ifd, ptr + offset, size) != size) { - fprintf (stderr, "%s: Write error on %s: %s\n", - params.cmdname, params.imagefile, strerror(errno)); + + ret = write(ifd, ptr + offset, size); + if (ret != size) { + if (ret < 0) + fprintf (stderr, "%s: Write error on %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + else if (ret < size) + fprintf (stderr, "%s: No space left on the device\n", + params.cmdname); Thanks for improving the error message, it is much more clear now. The only question I have is if "no space left on the device" is the only possible cause for this situation, for sure it is the most probable. Thanks for your review. Indeed, it is the most probable I guess. Yes, I agree. However, in order to be more accurate, and probably avoid future patches to correct a misleading message I would improve the error message with something like "Fewer bytes written than expected, probably no space left on the device" or something similar. What do you think? Yes, why not. I thought that "No space left" is a more common error message but let's change that with more details about how many written/expected bytes. Besides that Reviewed-by: Walter Lozano Thanks! exit (EXIT_FAILURE); } Regards, Walter Best regards, Mylène
Re: [PATCH v1 1/1] mkimage: Fix error message if write less data then expected
Hi Mylene, On 7/7/20 05:25, Mylene Josserand wrote: Hi, On 6/9/20 8:49 PM, Walter Lozano wrote: Hi Mylene, Thanks for you patch. On 5/6/20 05:01, Mylène Josserand wrote: Add a new error message in case the size of data written are shorter than the one expected. Currently, it will lead to the following error message: "mkimage: Write error on uImage: Success" This is not explicit when the error is because the device doesn't have enough space. Let's use a "No space left on the device" error message in that case. Signed-off-by: Mylène Josserand --- tools/mkimage.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/mkimage.c b/tools/mkimage.c index d2cd1917874..ef0cc889a92 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -674,7 +674,7 @@ copy_file (int ifd, const char *datafile, int pad) int zero = 0; uint8_t zeros[4096]; int offset = 0; - int size; + int size, ret; struct image_type_params *tparams = imagetool_get_type(params.type); memset(zeros, 0, sizeof(zeros)); @@ -730,9 +730,15 @@ copy_file (int ifd, const char *datafile, int pad) } size = sbuf.st_size - offset; - if (write(ifd, ptr + offset, size) != size) { - fprintf (stderr, "%s: Write error on %s: %s\n", - params.cmdname, params.imagefile, strerror(errno)); + + ret = write(ifd, ptr + offset, size); + if (ret != size) { + if (ret < 0) + fprintf (stderr, "%s: Write error on %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + else if (ret < size) + fprintf (stderr, "%s: No space left on the device\n", + params.cmdname); Thanks for improving the error message, it is much more clear now. The only question I have is if "no space left on the device" is the only possible cause for this situation, for sure it is the most probable. Thanks for your review. Indeed, it is the most probable I guess. Yes, I agree. However, in order to be more accurate, and probably avoid future patches to correct a misleading message I would improve the error message with something like "Fewer bytes written than expected, probably no space left on the device" or something similar. What do you think? Besides that Reviewed-by: Walter Lozano exit (EXIT_FAILURE); } Regards, Walter
Re: [PATCH v1 1/1] mkimage: Fix error message if write less data then expected
Hi, On 6/9/20 8:49 PM, Walter Lozano wrote: Hi Mylene, Thanks for you patch. On 5/6/20 05:01, Mylène Josserand wrote: Add a new error message in case the size of data written are shorter than the one expected. Currently, it will lead to the following error message: "mkimage: Write error on uImage: Success" This is not explicit when the error is because the device doesn't have enough space. Let's use a "No space left on the device" error message in that case. Signed-off-by: Mylène Josserand --- tools/mkimage.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/mkimage.c b/tools/mkimage.c index d2cd1917874..ef0cc889a92 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -674,7 +674,7 @@ copy_file (int ifd, const char *datafile, int pad) int zero = 0; uint8_t zeros[4096]; int offset = 0; - int size; + int size, ret; struct image_type_params *tparams = imagetool_get_type(params.type); memset(zeros, 0, sizeof(zeros)); @@ -730,9 +730,15 @@ copy_file (int ifd, const char *datafile, int pad) } size = sbuf.st_size - offset; - if (write(ifd, ptr + offset, size) != size) { - fprintf (stderr, "%s: Write error on %s: %s\n", - params.cmdname, params.imagefile, strerror(errno)); + + ret = write(ifd, ptr + offset, size); + if (ret != size) { + if (ret < 0) + fprintf (stderr, "%s: Write error on %s: %s\n", + params.cmdname, params.imagefile, strerror(errno)); + else if (ret < size) + fprintf (stderr, "%s: No space left on the device\n", + params.cmdname); Thanks for improving the error message, it is much more clear now. The only question I have is if "no space left on the device" is the only possible cause for this situation, for sure it is the most probable. Thanks for your review. Indeed, it is the most probable I guess. exit (EXIT_FAILURE); } Regards, Walter Best regards, Mylène
Re: [PATCH v1 1/1] mkimage: Fix error message if write less data then expected
Hi Mylene, Thanks for you patch. On 5/6/20 05:01, Mylène Josserand wrote: Add a new error message in case the size of data written are shorter than the one expected. Currently, it will lead to the following error message: "mkimage: Write error on uImage: Success" This is not explicit when the error is because the device doesn't have enough space. Let's use a "No space left on the device" error message in that case. Signed-off-by: Mylène Josserand --- tools/mkimage.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/mkimage.c b/tools/mkimage.c index d2cd1917874..ef0cc889a92 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -674,7 +674,7 @@ copy_file (int ifd, const char *datafile, int pad) int zero = 0; uint8_t zeros[4096]; int offset = 0; - int size; + int size, ret; struct image_type_params *tparams = imagetool_get_type(params.type); memset(zeros, 0, sizeof(zeros)); @@ -730,9 +730,15 @@ copy_file (int ifd, const char *datafile, int pad) } size = sbuf.st_size - offset; - if (write(ifd, ptr + offset, size) != size) { - fprintf (stderr, "%s: Write error on %s: %s\n", - params.cmdname, params.imagefile, strerror(errno)); + + ret = write(ifd, ptr + offset, size); + if (ret != size) { + if (ret < 0) + fprintf (stderr, "%s: Write error on %s: %s\n", +params.cmdname, params.imagefile, strerror(errno)); + else if (ret < size) + fprintf (stderr, "%s: No space left on the device\n", +params.cmdname); Thanks for improving the error message, it is much more clear now. The only question I have is if "no space left on the device" is the only possible cause for this situation, for sure it is the most probable. exit (EXIT_FAILURE); } Regards, Walter
[PATCH v1 1/1] mkimage: Fix error message if write less data then expected
Add a new error message in case the size of data written are shorter than the one expected. Currently, it will lead to the following error message: "mkimage: Write error on uImage: Success" This is not explicit when the error is because the device doesn't have enough space. Let's use a "No space left on the device" error message in that case. Signed-off-by: Mylène Josserand --- tools/mkimage.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tools/mkimage.c b/tools/mkimage.c index d2cd1917874..ef0cc889a92 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -674,7 +674,7 @@ copy_file (int ifd, const char *datafile, int pad) int zero = 0; uint8_t zeros[4096]; int offset = 0; - int size; + int size, ret; struct image_type_params *tparams = imagetool_get_type(params.type); memset(zeros, 0, sizeof(zeros)); @@ -730,9 +730,15 @@ copy_file (int ifd, const char *datafile, int pad) } size = sbuf.st_size - offset; - if (write(ifd, ptr + offset, size) != size) { - fprintf (stderr, "%s: Write error on %s: %s\n", - params.cmdname, params.imagefile, strerror(errno)); + + ret = write(ifd, ptr + offset, size); + if (ret != size) { + if (ret < 0) + fprintf (stderr, "%s: Write error on %s: %s\n", +params.cmdname, params.imagefile, strerror(errno)); + else if (ret < size) + fprintf (stderr, "%s: No space left on the device\n", +params.cmdname); exit (EXIT_FAILURE); } -- 2.26.2