Don't HANDLE_EINTR(close). Either IGNORE_EINTR(close) or just close.

Don't HANDLE_EINTR(close). Either IGNORE_EINTR(close) or just close.

It is incorrect to wrap close in HANDLE_EINTR on Linux. Correctness is
generally undefined on Mac, but as of r223369, it is incorrect in Chrome on
Mac.

To avoid new offenders, a PRESUBMIT check ensures that HANDLE_EINTR is not
used with close, and that IGNORE_EINTR is only used with close. Unnecessary
#includes of eintr_wrapper.h are also removed.

base/posix/einter_wrapper.h, PRESUBMIT.py, and ppapi/tests/test_broker.cc
contain non-mechanical changes. Variable naming within the latter is updated
per r178174. Missing #includes for <errno.h> in
content/zygote/zygote_main_linux.cc and tools/android/common/daemon.cc were
manually added. Mechanical changes were generated by running:

sed -E -i '' \
    -e 's/((=|if|return|CHECK|EXPECT|ASSERT).*)HANDLE(_EINTR\(.*close)/\1IGNORE\3/' \
    -e 's/(ignore_result|void ?)\(HANDLE_EINTR\((.*close\(.*)\)\)/\2/' \
    -e 's/(\(void\) ?)?HANDLE_EINTR\((.*close\(.*)\)/\2/' \
    $(git grep -El 'HANDLE_EINTR.*close')

sed -E -i '' -e '/#include.*eintr_wrapper\.h"/d' \
    $(grep -EL '(HANDLE|IGNORE)_EINTR' \
        $(git grep -El '#include.*eintr_wrapper\.h"'))

BUG=269623
R=agl@chromium.org, jln@chromium.org
TBR=OWNERS

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238390

[agl, 2013-12-02 22:46:33]

lgtm

[Mark Mentovai, 2013-12-02 22:54:58]

+darin (tree-wide OWNER), will TBR once the try jobs are green.

[jln (very slow on Chromium), 2013-12-02 23:50:13]

We had this discussion a while back when figuring out what to do for sandbox/
Since the code is monothreaded in sandbox/ HANDLE_EINTR() seemed more "ok".

I think the case is also pretty hairy on Linux.

sys_close will call into fs/open.c:filp_close(). From there, the kernel will
call into the flush() f_op of the underlying file system.

Markus and myself have been puzzled in the past by what some filesystems where
doing (int heir flush f_op), especially FUSE backed ones, or things such as
ecryptfs (which our users are using).

But it's still true that the vast majority of EINTR are just ERESTARTSYS etc.
being replaced by the kernel by EINTR in an ugly "workaround".

So I would agree that this change is the right thing to do, but unfortunately,
something may break.

[jln (very slow on Chromium), 2013-12-02 23:50:30]

https://codereview.chromium.org/100253002/diff/30001/base/posix/eintr_wrapper.h
File base/posix/eintr_wrapper.h (right):

https://codereview.chromium.org/100253002/diff/30001/base/posix/eintr_wrapper...
base/posix/eintr_wrapper.h:54: eintr_wrapper_result = 0; \
Shouldn't you patch errno as well ?

[Mark Mentovai, 2013-12-03 03:30:51]

https://codereview.chromium.org/100253002/diff/30001/base/posix/eintr_wrapper.h
File base/posix/eintr_wrapper.h (right):

https://codereview.chromium.org/100253002/diff/30001/base/posix/eintr_wrapper...
base/posix/eintr_wrapper.h:54: eintr_wrapper_result = 0; \
jln wrote:
> Shouldn't you patch errno as well ?

Why? And how? Reset it to its original value? I don’t think that’s necessary,
since nobody should be examining errno after close returns 0 (success), or after
any other system call indicates success. I avoided it because it’s extra code
that gets inlined in over a hundred places with no tangible benefit.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/errno.html:

> The value of errno shall be defined only after a call to a function
> for which it is explicitly stated to be set and until it is changed by
> the next function call or if the application assigns it a value. The
> value of errno should only be examined when it is indicated to be
> valid by a function's return value.

and

> The setting of errno after a successful call to a function is
> unspecified unless the description of that function specifies that
> errno shall not be modified.

The description of “close” doesn’t say that errno shall not be modified.

On Linux, “man 3 errno”:

> Its value is significant only when the return value of the call
> indicated an error (i.e., -1 from most system calls; -1 or NULL from
> most library functions)

[jln (very slow on Chromium), 2013-12-03 05:46:47]

ok, lgtm

[Mark Mentovai, 2013-12-03 14:11:06]

Committed patchset #4 manually as r238390.

[darin (slow to review), 2013-12-03 18:40:57]

rubber stamp LGTM