2018-01-08 19:17 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>:
>
> On Sun, 31 Dec 2017, Masahiro Yamada wrote:
>
>> 2017-12-22 4:10 GMT+09:00 Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>:
>>>
>>> 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
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
