| Message ID | 1513883429-9527-1-git-send-email-lukas.bulwahn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Sun, Dec 31, 2017 at 01:51:33AM +0900, Masahiro Yamada wrote: > 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.) > What case for a legitimately empty source file would there be ? the files being checked are assumed to be mandatory for the given configuration right ? if so how can a configuration depend on an empty file ? would that not point to a config problem and rather be fixed there ? or am I misunderstanding this here ? > Having an empty source file is rare, but possible. > putting a printf in the empty case handling and running a set of randconfigs did not reveal such an event (10 runs). thx! hofrat > > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-01 0:45 GMT+09:00 Nicholas Mc Guire <der.herr@hofr.at>: > On Sun, Dec 31, 2017 at 01:51:33AM +0900, Masahiro Yamada wrote: >> 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.) >> > > What case for a legitimately empty source file would there be ? > the files being checked are assumed to be mandatory for the > given configuration right ? if so how can a configuration depend > on an empty file ? would that not point to a config problem and > rather be fixed there ? or am I misunderstanding this here ? > >> Having an empty source file is rare, but possible. >> > > putting a printf in the empty case handling and running a set of > randconfigs did not reveal such an event (10 runs). Remove the comment line in scripts/mod/empty.c > thx! > hofrat > >> >> >> >> >> >> > 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 01, 2018 at 03:41:10PM +0900, Masahiro Yamada wrote: > 2018-01-01 0:45 GMT+09:00 Nicholas Mc Guire <der.herr@hofr.at>: > > On Sun, Dec 31, 2017 at 01:51:33AM +0900, Masahiro Yamada wrote: > >> 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.) > >> > > > > What case for a legitimately empty source file would there be ? > > the files being checked are assumed to be mandatory for the > > given configuration right ? if so how can a configuration depend > > on an empty file ? would that not point to a config problem and > > rather be fixed there ? or am I misunderstanding this here ? > > > >> Having an empty source file is rare, but possible. > >> > > > > putting a printf in the empty case handling and running a set of > > randconfigs did not reveal such an event (10 runs). > > > Remove the comment line in > scripts/mod/empty.c > > And would that file be acceptable if it were actually empty The point is what would be the rational for a file that is mandatory but empty - my assumption is that this file would not be acceptable if it were actually empty simply because it would be impossible to figure out what it is good for and readability/documentation is a key, if not the key, issue in a softare element the complexity of the linux kernel. The argument for the exit(2) here would be that an empty file would indicate that something is going wrong with high probablity and jumping that event could result in broken builds. thx! hofrat > > > thx! > > hofrat > > > >> > >> > >> > >> > >> > >> > 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 > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Best Regards > Masahiro Yamada -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > And would that file be acceptable if it were actually empty There have been cases with empty files in the source tree. And it is not the job of fixdep to do anything special about them. fixdep shall do what it is requested to do, and not more than that. Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 31 Dec 2017, Masahiro Yamada wrote: > 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>: >> 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. > > Now that I looked at the code for creating the v3 patch, I am confused about the error messages in scripts/basic/fixdep.c, lines 275 - 285, in do_config_file(): fd = open(filename, O_RDONLY); if (fd < 0) { fprintf(stderr, "fixdep: error opening config file: "); perror(filename); exit(2); } if (fstat(fd, &st) < 0) { fprintf(stderr, "fixdep: error fstat'ing config file: "); perror(filename); exit(2); } These error messages suggest that filename (and the file handler fd) refers to a config file, but you say that filename and fd refer to source files. Looking at parse_dep_file() and how it invokes do_config_file(), I think you are right that it does refer to source files. If you confirm that this is correct, I would change `config file` to `source file` in the error messages of do_config_file(). What is best to do here, provide a separate patch or add it to the existing patch? Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-08 19:17 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>: > > On Sun, 31 Dec 2017, Masahiro Yamada wrote: > >> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@gmail.com>: >>> >>> 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. >> >> > > Now that I looked at the code for creating the v3 patch, I am confused about > the error messages in scripts/basic/fixdep.c, lines 275 - 285, in > do_config_file(): Not only the error messages. Function names are confusing too. parse_config_file(), do_config_file these are not for parsing configuration file, but for parsing files that contain CONFIG options > > fd = open(filename, O_RDONLY); > if (fd < 0) { > fprintf(stderr, "fixdep: error opening config file: "); > perror(filename); > exit(2); > } > if (fstat(fd, &st) < 0) { > fprintf(stderr, "fixdep: error fstat'ing config file: "); > perror(filename); > exit(2); > } > > These error messages suggest that filename (and the file handler fd) refers > to a config file, but you say that filename and fd refer to source files. > > Looking at parse_dep_file() and how it invokes do_config_file(), I think you > are right that it does refer to source files. This depends on the definition of "source file". In general, we think "source files" refer to files processed by compiler, like *c, *.h, *.S, *.dts, etc. In the fixdep context, "source file" has narrower meaning. You will notice this from the comments in parse_dep_file(), "Do not list the source file as dependency, ..." > If you confirm that this is correct, I would change `config file` to `source > file` in the error messages of do_config_file(). Maybe a bad idea. For the reason above, "source file" is also confusing in my opinion. Just "file" should be fine. But I do not need a patch just for removing "config". I'd like to refactor the code in a bigger picture. do_config_file() and print_deps() can be merged together, since they do similar things. After merged, load_file() will refer "file". https://patchwork.kernel.org/patch/10151165/ https://patchwork.kernel.org/patch/10151157/ I had already had those cleanups in my mind but I was holding it back to avoid conflicts with your patch. > What is best to do here, provide a separate patch or add it to the existing > patch? > > Lukas
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); } 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);
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"). 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") 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(-)