Bug 237477 – Add null check for path in makeAllDirectories

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 453859 [details]
Patch

[Chris Dumez]

Comment on attachment 453859 [details]
Patch

Your test is failing on api-gtk.

[Darin Adler]

Comment on attachment 453859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453859&action=review

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:280
> +        LOG_ERROR("File failed to access. Error message: %s", safeStrerror(errno).data());

I think "file failed to access" is a vague message to put in a log, and not how I would describe a failure of the access function. If we are going to log this, then the message should make some mention of the fact that were were attempting to create directories, perhaps. Also wondering why we decided to log about the failure of this access call but not about the failures of the other access and mkdir calls below.

Generally we want these file system abstraction functions like makeAllDirectories to not log directly to the console, so callers can make calls they know will or might fail without unconditionally cluttering logs. I understand that this conflicts with the desire to know what’s happening when diagnosing a bug. I’m not sure there is an easy answer. Generally speaking it would not be good if all file system calls logged what happened directly to the console, and certainly that’s not how the underlying POSIX functions behave.

[Sihui Liu]

Comment on attachment 453859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453859&action=review

>> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:280
>> +        LOG_ERROR("File failed to access. Error message: %s", safeStrerror(errno).data());
> 
> I think "file failed to access" is a vague message to put in a log, and not how I would describe a failure of the access function. If we are going to log this, then the message should make some mention of the fact that were were attempting to create directories, perhaps. Also wondering why we decided to log about the failure of this access call but not about the failures of the other access and mkdir calls below.
> 
> Generally we want these file system abstraction functions like makeAllDirectories to not log directly to the console, so callers can make calls they know will or might fail without unconditionally cluttering logs. I understand that this conflicts with the desire to know what’s happening when diagnosing a bug. I’m not sure there is an easy answer. Generally speaking it would not be good if all file system calls logged what happened directly to the console, and certainly that’s not how the underlying POSIX functions behave.

I added the log here because I realized that !access() does not necessarily mean file does not exist; it could be something more serious (like the fullPath is null case).
(I planned to add early return on errno != ENOENT, instead of error log, but I am not sure if that would exclude some valid cases...)

I think it's fair that we don't want to indroduce unnecessary logs for file system calls. Since there's no known issue here so far, I will remove this line.

[Sihui Liu]

Created attachment 453881 [details]
Patch

[EWS]

Committed r290862 (248093@main): <https://commits.webkit.org/248093@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453881 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89848974>