| Message ID | 20200605080127.10156-2-mylene.josserand@collabora.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix mkimage error message | expand |
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 <mylene.josserand@collabora.com> > --- > 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
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 <mylene.josserand@collabora.com> >> --- >> 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
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 <mylene.josserand@collabora.com> >>> --- >>> 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 <walter.lozano@collabora.com> > >> >>> exit (EXIT_FAILURE); >>> } >> Regards, Walter
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 <mylene.josserand@collabora.com> >>>> --- >>>> 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 <walter.lozano@collabora.com> Thanks! > >> >>> >>>> exit (EXIT_FAILURE); >>>> } >>> > Regards, > > Walter > Best regards, Mylène
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); }
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 <mylene.josserand@collabora.com> --- tools/mkimage.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)