Messages in this thread | | | From | Masahiro Yamada <> | Date | Sun, 31 Dec 2017 01:51:33 +0900 | Subject | Re: [PATCH v2] fixdep: exit with error code in error branches of do_config_file() |
| |
2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>: > do_config_file() should exit with an error code, and not return if it fails > as then the error in do_config_file() would go unnoticed in the current > code and allow the build to continue. The exit with error code will make > the build fail in those very exceptional cases. If this occurs, this > actually indicates a deeper problem in the execution of the kernel build > process. > > Now, that the function exists, we do not explicitly free memory and close > the file handlers in do_config_file(), as this is covered by exit(). > > This issue in the fixdep script was introduced with its initial > implementation back in 2002 by the original author Kai Germaschewski with > this commit 04bd72170653 ("kbuild: Make dependencies at compile time").
"04bd72170653" is just confusing.
If you really want to mention this hash, please clearly say it is in the history repository outside of this.
> This issue was identified during the review of a previous patch that > intended to address a memory leak detected by a static analysis tool. > > Link: https://lkml.org/lkml/2017/12/14/736 > > Fixes: 04bd72170653 ("kbuild: Make dependencies at compile time")
Please drop this pointless Fixes tag because that commit is not reachable from this patch.
> Suggested-by: Nicholas Mc Guire <der.herr@hofr.at> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> > --- > compile tested on top of next-20171220 with clang and gcc > Change in v2: > - no code change; only include proper Fixes tag and explain it > > scripts/basic/fixdep.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c > index bbf62cb..4274610 100644 > --- a/scripts/basic/fixdep.c > +++ b/scripts/basic/fixdep.c > @@ -284,19 +284,18 @@ static void do_config_file(const char *filename) > exit(2); > } > if (st.st_size == 0) { > - close(fd); > - return; > + fprintf(stderr, "fixdep: error empty file config file: "); > + perror(filename); > + exit(2); > }
No. This is correct as-is.
do_config_file() does not parse .cmd files but parse source files (.c .h .S etc.)
Having an empty source file is rare, but possible.
> map = malloc(st.st_size + 1); > if (!map) { > perror("fixdep: malloc"); > - close(fd); > - return; > + exit(2); > } > if (read(fd, map, st.st_size) != st.st_size) { > perror("fixdep: read"); > - close(fd); > - return; > + exit(2); > } > map[st.st_size] = '\0'; > close(fd); > -- > 2.7.4 >
These two changes are OK.
-- Best Regards Masahiro Yamada
| |