| Message ID | 20181024051500.12467-1-peng.fan@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [U-Boot] tools: imx8image: return SUCCESS when the required files not found | expand |
Hi Peng, On 24/10/18 07:09, Peng Fan wrote: > When the required files to build a bootable imx8 image are not > found, return EXIT_SUCCESS to avoid build CI system. > And if the files are missing, give a error message during the build. > Thanks for this - anyway, I am thinking about if this is the correct solution. IMHO the current implementation in mkimage *is* already correct. Files are missing, mkimage cannot be completed with success, we do not get an image suitable for booting - an error is raised. What else ? There is nothing wrong on this :-). However, we have to make CI happy - but mkimage could be called even outside a U-Boot build, and should be scriptable, that is it must report an error in case of failure. This is already provided. Instead of changing mkimage, I thought to not call mkimage at all. We could add a test in arch/arm/mach-imx/Makefile to check if files for imx8 are available, and if not, we print warnings and Makefile returns with success. CI / Travis should be happy again without introducing a buggy behavior into mkimage. What do you think ? Best regards, Stefano > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > tools/imx8image.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/tools/imx8image.c b/tools/imx8image.c > index e6b0a146b6..35409646f5 100644 > --- a/tools/imx8image.c > +++ b/tools/imx8image.c > @@ -279,9 +279,9 @@ static void check_file(struct stat *sbuf, char *filename) > int tmp_fd = open(filename, O_RDONLY | O_BINARY); > > if (tmp_fd < 0) { > - fprintf(stderr, "%s: Can't open: %s\n", > + fprintf(stderr, "*** %s: Can't open: %s ***\n", > filename, strerror(errno)); > - exit(EXIT_FAILURE); > + exit(EXIT_SUCCESS); > } > > if (fstat(tmp_fd, sbuf) < 0) { > @@ -311,9 +311,9 @@ static void copy_file_aligned(int ifd, const char *datafile, int offset, > > dfd = open(datafile, O_RDONLY | O_BINARY); > if (dfd < 0) { > - fprintf(stderr, "Can't open %s: %s\n", > + fprintf(stderr, "*** Can't open %s: %s ***\n", > datafile, strerror(errno)); > - exit(EXIT_FAILURE); > + exit(EXIT_SUCCESS); > } > > if (fstat(dfd, &sbuf) < 0) { > @@ -365,9 +365,9 @@ static void copy_file (int ifd, const char *datafile, int pad, int offset) > > dfd = open(datafile, O_RDONLY | O_BINARY); > if (dfd < 0) { > - fprintf(stderr, "Can't open %s: %s\n", > + fprintf(stderr, "*** Can't open %s: %s ***\n", > datafile, strerror(errno)); > - exit(EXIT_FAILURE); > + exit(EXIT_SUCCESS); > } > > if (fstat(dfd, &sbuf) < 0) { > @@ -648,8 +648,8 @@ static int get_container_image_start_pos(image_t *image_stack, uint32_t align) > if (img_sp->option == APPEND) { > fd = fopen(img_sp->filename, "r"); > if (!fd) { > - fprintf(stderr, "Fail open first container file %s\n", img_sp->filename); > - exit(EXIT_FAILURE); > + fprintf(stderr, "*** Fail open first container file %s ***\n", img_sp->filename); > + exit(EXIT_SUCCESS); > } > > ret = fread(&header, sizeof(header), 1, fd); >
Hi Stefano, > -----Original Message----- > From: Stefano Babic [mailto:sbabic@denx.de] > Sent: 2018年10月24日 15:31 > To: Peng Fan <peng.fan@nxp.com>; sbabic@denx.de > Cc: u-boot@lists.denx.de > Subject: Re: [PATCH] tools: imx8image: return SUCCESS when the required files > not found > > Hi Peng, > > On 24/10/18 07:09, Peng Fan wrote: > > When the required files to build a bootable imx8 image are not found, > > return EXIT_SUCCESS to avoid build CI system. > > And if the files are missing, give a error message during the build. > > > > Thanks for this - anyway, I am thinking about if this is the correct solution. IMHO > the current implementation in mkimage *is* already correct. Files are missing, > mkimage cannot be completed with success, we do not get an image suitable for > booting - an error is raised. What else ? There is nothing wrong on this :-). > > However, we have to make CI happy - but mkimage could be called even outside > a U-Boot build, and should be scriptable, that is it must report an error in case of > failure. This is already provided. > > Instead of changing mkimage, I thought to not call mkimage at all. We could add > a test in arch/arm/mach-imx/Makefile to check if files for > imx8 are available, and if not, we print warnings and Makefile returns with > success. CI / Travis should be happy again without introducing a buggy behavior > into mkimage. What do you think ? Just send out a new patch, please check whether it is ok. I also kicked a CI build, still running, https://travis-ci.org/MrVan/u-boot/builds Thanks, Peng. > > Best regards, > Stefano > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > tools/imx8image.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/tools/imx8image.c b/tools/imx8image.c index > > e6b0a146b6..35409646f5 100644 > > --- a/tools/imx8image.c > > +++ b/tools/imx8image.c > > @@ -279,9 +279,9 @@ static void check_file(struct stat *sbuf, char > *filename) > > int tmp_fd = open(filename, O_RDONLY | O_BINARY); > > > > if (tmp_fd < 0) { > > - fprintf(stderr, "%s: Can't open: %s\n", > > + fprintf(stderr, "*** %s: Can't open: %s ***\n", > > filename, strerror(errno)); > > - exit(EXIT_FAILURE); > > + exit(EXIT_SUCCESS); > > } > > > > if (fstat(tmp_fd, sbuf) < 0) { > > @@ -311,9 +311,9 @@ static void copy_file_aligned(int ifd, const char > > *datafile, int offset, > > > > dfd = open(datafile, O_RDONLY | O_BINARY); > > if (dfd < 0) { > > - fprintf(stderr, "Can't open %s: %s\n", > > + fprintf(stderr, "*** Can't open %s: %s ***\n", > > datafile, strerror(errno)); > > - exit(EXIT_FAILURE); > > + exit(EXIT_SUCCESS); > > } > > > > if (fstat(dfd, &sbuf) < 0) { > > @@ -365,9 +365,9 @@ static void copy_file (int ifd, const char > > *datafile, int pad, int offset) > > > > dfd = open(datafile, O_RDONLY | O_BINARY); > > if (dfd < 0) { > > - fprintf(stderr, "Can't open %s: %s\n", > > + fprintf(stderr, "*** Can't open %s: %s ***\n", > > datafile, strerror(errno)); > > - exit(EXIT_FAILURE); > > + exit(EXIT_SUCCESS); > > } > > > > if (fstat(dfd, &sbuf) < 0) { > > @@ -648,8 +648,8 @@ static int get_container_image_start_pos(image_t > *image_stack, uint32_t align) > > if (img_sp->option == APPEND) { > > fd = fopen(img_sp->filename, "r"); > > if (!fd) { > > - fprintf(stderr, "Fail open first container file %s\n", > img_sp->filename); > > - exit(EXIT_FAILURE); > > + fprintf(stderr, "*** Fail open first container file %s ***\n", > img_sp->filename); > > + exit(EXIT_SUCCESS); > > } > > > > ret = fread(&header, sizeof(header), 1, fd); > > > > > -- > ================================================================ > ===== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ================================================================ > =====
Hi Peng, On 24/10/18 11:51, Peng Fan wrote: > Hi Stefano, > >> -----Original Message----- >> From: Stefano Babic [mailto:sbabic@denx.de] >> Sent: 2018年10月24日 15:31 >> To: Peng Fan <peng.fan@nxp.com>; sbabic@denx.de >> Cc: u-boot@lists.denx.de >> Subject: Re: [PATCH] tools: imx8image: return SUCCESS when the required files >> not found >> >> Hi Peng, >> >> On 24/10/18 07:09, Peng Fan wrote: >>> When the required files to build a bootable imx8 image are not found, >>> return EXIT_SUCCESS to avoid build CI system. >>> And if the files are missing, give a error message during the build. >>> >> >> Thanks for this - anyway, I am thinking about if this is the correct solution. IMHO >> the current implementation in mkimage *is* already correct. Files are missing, >> mkimage cannot be completed with success, we do not get an image suitable for >> booting - an error is raised. What else ? There is nothing wrong on this :-). >> >> However, we have to make CI happy - but mkimage could be called even outside >> a U-Boot build, and should be scriptable, that is it must report an error in case of >> failure. This is already provided. >> >> Instead of changing mkimage, I thought to not call mkimage at all. We could add >> a test in arch/arm/mach-imx/Makefile to check if files for >> imx8 are available, and if not, we print warnings and Makefile returns with >> success. CI / Travis should be happy again without introducing a buggy behavior >> into mkimage. What do you think ? > > Just send out a new patch, please check whether it is ok. Thanks - it is what I mind ;-) > I also kicked a CI build, still running, https://travis-ci.org/MrVan/u-boot/builds Well, easy for me :-) I just wait your build is over, and then I merge and send a new PR to Tom ! Best regards, Stefano > > Thanks, > Peng. > >> >> Best regards, >> Stefano >> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>> --- >>> tools/imx8image.c | 16 ++++++++-------- >>> 1 file changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/tools/imx8image.c b/tools/imx8image.c index >>> e6b0a146b6..35409646f5 100644 >>> --- a/tools/imx8image.c >>> +++ b/tools/imx8image.c >>> @@ -279,9 +279,9 @@ static void check_file(struct stat *sbuf, char >> *filename) >>> int tmp_fd = open(filename, O_RDONLY | O_BINARY); >>> >>> if (tmp_fd < 0) { >>> - fprintf(stderr, "%s: Can't open: %s\n", >>> + fprintf(stderr, "*** %s: Can't open: %s ***\n", >>> filename, strerror(errno)); >>> - exit(EXIT_FAILURE); >>> + exit(EXIT_SUCCESS); >>> } >>> >>> if (fstat(tmp_fd, sbuf) < 0) { >>> @@ -311,9 +311,9 @@ static void copy_file_aligned(int ifd, const char >>> *datafile, int offset, >>> >>> dfd = open(datafile, O_RDONLY | O_BINARY); >>> if (dfd < 0) { >>> - fprintf(stderr, "Can't open %s: %s\n", >>> + fprintf(stderr, "*** Can't open %s: %s ***\n", >>> datafile, strerror(errno)); >>> - exit(EXIT_FAILURE); >>> + exit(EXIT_SUCCESS); >>> } >>> >>> if (fstat(dfd, &sbuf) < 0) { >>> @@ -365,9 +365,9 @@ static void copy_file (int ifd, const char >>> *datafile, int pad, int offset) >>> >>> dfd = open(datafile, O_RDONLY | O_BINARY); >>> if (dfd < 0) { >>> - fprintf(stderr, "Can't open %s: %s\n", >>> + fprintf(stderr, "*** Can't open %s: %s ***\n", >>> datafile, strerror(errno)); >>> - exit(EXIT_FAILURE); >>> + exit(EXIT_SUCCESS); >>> } >>> >>> if (fstat(dfd, &sbuf) < 0) { >>> @@ -648,8 +648,8 @@ static int get_container_image_start_pos(image_t >> *image_stack, uint32_t align) >>> if (img_sp->option == APPEND) { >>> fd = fopen(img_sp->filename, "r"); >>> if (!fd) { >>> - fprintf(stderr, "Fail open first container file %s\n", >> img_sp->filename); >>> - exit(EXIT_FAILURE); >>> + fprintf(stderr, "*** Fail open first container file %s ***\n", >> img_sp->filename); >>> + exit(EXIT_SUCCESS); >>> } >>> >>> ret = fread(&header, sizeof(header), 1, fd); >>> >> >> >> -- >> ================================================================ >> ===== >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de >> ================================================================ >> =====
Dear Peng Fan, In message <20181024051500.12467-1-peng.fan@nxp.com> you wrote: > When the required files to build a bootable imx8 image are not > found, return EXIT_SUCCESS to avoid build CI system. > And if the files are missing, give a error message during the build. Full NAK. Naked-by: Wolfgang Denk <wd@denx.de> > if (tmp_fd < 0) { > - fprintf(stderr, "%s: Can't open: %s\n", > + fprintf(stderr, "*** %s: Can't open: %s ***\n", > filename, strerror(errno)); > - exit(EXIT_FAILURE); > + exit(EXIT_SUCCESS); An error like this must ALWAYS be reported by a proper return / exit code o fthe funtion / program, other wise it is impossible to use such tools in any automatic scripts, pipelines etc. Anything else is just fundamentally broken. I am shocked to see such a patch. Wolfgang Denk
diff --git a/tools/imx8image.c b/tools/imx8image.c index e6b0a146b6..35409646f5 100644 --- a/tools/imx8image.c +++ b/tools/imx8image.c @@ -279,9 +279,9 @@ static void check_file(struct stat *sbuf, char *filename) int tmp_fd = open(filename, O_RDONLY | O_BINARY); if (tmp_fd < 0) { - fprintf(stderr, "%s: Can't open: %s\n", + fprintf(stderr, "*** %s: Can't open: %s ***\n", filename, strerror(errno)); - exit(EXIT_FAILURE); + exit(EXIT_SUCCESS); } if (fstat(tmp_fd, sbuf) < 0) { @@ -311,9 +311,9 @@ static void copy_file_aligned(int ifd, const char *datafile, int offset, dfd = open(datafile, O_RDONLY | O_BINARY); if (dfd < 0) { - fprintf(stderr, "Can't open %s: %s\n", + fprintf(stderr, "*** Can't open %s: %s ***\n", datafile, strerror(errno)); - exit(EXIT_FAILURE); + exit(EXIT_SUCCESS); } if (fstat(dfd, &sbuf) < 0) { @@ -365,9 +365,9 @@ static void copy_file (int ifd, const char *datafile, int pad, int offset) dfd = open(datafile, O_RDONLY | O_BINARY); if (dfd < 0) { - fprintf(stderr, "Can't open %s: %s\n", + fprintf(stderr, "*** Can't open %s: %s ***\n", datafile, strerror(errno)); - exit(EXIT_FAILURE); + exit(EXIT_SUCCESS); } if (fstat(dfd, &sbuf) < 0) { @@ -648,8 +648,8 @@ static int get_container_image_start_pos(image_t *image_stack, uint32_t align) if (img_sp->option == APPEND) { fd = fopen(img_sp->filename, "r"); if (!fd) { - fprintf(stderr, "Fail open first container file %s\n", img_sp->filename); - exit(EXIT_FAILURE); + fprintf(stderr, "*** Fail open first container file %s ***\n", img_sp->filename); + exit(EXIT_SUCCESS); } ret = fread(&header, sizeof(header), 1, fd);
When the required files to build a bootable imx8 image are not found, return EXIT_SUCCESS to avoid build CI system. And if the files are missing, give a error message during the build. Signed-off-by: Peng Fan <peng.fan@nxp.com> --- tools/imx8image.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)