Re: Implement ScopedFD in terms of ScopedGeneric. (issue 191673003)

10 views
Skip to first unread message

viettr...@chromium.org

unread,
Mar 13, 2014, 6:37:10 PM3/13/14
to bre...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
On 2014/03/12 20:45:29, brettw wrote:
> Review comments so far, added some advice on when to use.


https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_launcher.cc
> File base/test/launcher/test_launcher.cc (right):


https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_launcher.cc#newcode269
> base/test/launcher/test_launcher.cc:269: output_file_fd.reset();
> On 2014/03/12 19:47:23, viettrungluu wrote:
> > Could you fix the indentation here as a drive-by?

> Done

> > (Actually, it seems to me that you could do this unconditionally.)

> I don't know what you mean.

> > (The Windows code also seems unusual to me -- wouldn't close imply
> flush?)

> Flush means flush to disk. I don't think Close() implies this.

Then why doesn't the POSIX version have an fsync?

https://codereview.chromium.org/191673003/

viettr...@chromium.org

unread,
Mar 13, 2014, 6:37:19 PM3/13/14
to bre...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org

https://codereview.chromium.org/191673003/diff/150043/content/browser/zygote_host/zygote_host_impl_linux.cc
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/191673003/diff/150043/content/browser/zygote_host/zygote_host_impl_linux.cc#newcode322
content/browser/zygote_host/zygote_host_impl_linux.cc:322:
std::vector<linked_ptr<base::ScopedFD> > autodelete_fds;
Side comment: I don't see why a ScopedVector isn't used instead.

https://codereview.chromium.org/191673003/diff/150043/ipc/ipc_channel_factory.cc
File ipc/ipc_channel_factory.cc (right):

https://codereview.chromium.org/191673003/diff/150043/ipc/ipc_channel_factory.cc#newcode56
ipc/ipc_channel_factory.cc:56: if (new_fd < 0) {
Maybe move the scoped_fd to here, and do |if (!scoped_fd.is_valid())|?

https://codereview.chromium.org/191673003/diff/150043/ipc/ipc_channel_factory.cc#newcode65
ipc/ipc_channel_factory.cc:65: if (!IsPeerAuthorized(new_fd))
And use scoped_fd.get() here.

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_server/local_test_server_posix.cc
File net/test/spawned_test_server/local_test_server_posix.cc (right):

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_server/local_test_server_posix.cc#newcode151
net/test/spawned_test_server/local_test_server_posix.cc:151:
base::ScopedFD our_fd(child_fd_.release());
Why are you changing the name from "child" to "our"?

https://codereview.chromium.org/191673003/diff/150043/sandbox/linux/services/scoped_process_unittest.cc
File sandbox/linux/services/scoped_process_unittest.cc (right):

https://codereview.chromium.org/191673003/diff/150043/sandbox/linux/services/scoped_process_unittest.cc#newcode83
sandbox/linux/services/scoped_process_unittest.cc:83:
write_end_closer.reset();
Hrm, this changes "IGNORE_EINTR" to "HANDLE_EINTR". Dunno if that's
actually significant.

https://codereview.chromium.org/191673003/

viettr...@chromium.org

unread,
Mar 13, 2014, 6:38:13 PM3/13/14
to bre...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
Also, LGTM w/the noted things addressed/looked
at/considered/ignored/aborted/retried/failed.

https://codereview.chromium.org/191673003/

bre...@chromium.org

unread,
Mar 13, 2014, 6:42:10 PM3/13/14
to viettr...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_server/local_test_server_posix.cc
File net/test/spawned_test_server/local_test_server_posix.cc (right):

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_server/local_test_server_posix.cc#newcode151
net/test/spawned_test_server/local_test_server_posix.cc:151:
base::ScopedFD our_fd(child_fd_.release());
On 2014/03/13 22:37:20, viettrungluu wrote:
> Why are you changing the name from "child" to "our"?

I didn't want to have two variables named child_fd and child_fd_.

https://codereview.chromium.org/191673003/

bre...@chromium.org

unread,
Mar 14, 2014, 1:21:27 AM3/14/14
to viettr...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
Committed patchset #11 manually as r257001.

https://codereview.chromium.org/191673003/

tha...@chromium.org

unread,
Mar 14, 2014, 1:33:22 AM3/14/14
to bre...@chromium.org, viettr...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
A revert of this CL has been created in
https://codereview.chromium.org/197873014/ by tha...@chromium.org.

The reason for reverting is: Doesn't build on android:

FAILED: /mnt/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/base/memory/base.discardable_memory_allocator_android.o.d
-DV8_DEPRECATION_WARNINGS -DBLINK_SCALE_FILTERS_AT_RECORD_TIME
-D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD
-DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DUSE_PROPRIETARY_CODECS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NEW_GAMEPAD_API=1 -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_EGLIMAGE=1
-DENABLE_AUTOFILL_DIALOG=1 -DCLD_VERSION=1 -DENABLE_PRINTING=1
-DENABLE_MANAGED_USERS=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DBASE_IMPLEMENTATION -DANDROID -D__GNU_SOURCE=1
-DUSE_STLPORT=1 -D_STLP_USE_PTR_SPECIALIZATIONS=1 '-DCHROME_BUILD_ID=""'
-DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1
-D_DEBUG -Igen/base
-I../../third_party/android_tools/ndk/sources/android/cpufeatures -I../..
-fstack-protector --param=ssp-buffer-size=4 -Werror -fno-exceptions
-fno-strict-aliasing -Wall -Wno-unused-parameter
-Wno-missing-field-initializers
-fvisibility=hidden -pipe -fPIC -Wheader-hygiene -Wno-char-subscripts
-Wno-unneeded-internal-declaration -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-reserved-user-defined-literal
-Wno-deprecated-register -Xclang -load -Xclang
/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-url-directory
-fcolor-diagnostics
-Wexit-time-destructors -march=armv7-a -mfpu=vfpv3-d16 -mfloat-abi=softfp
-mthumb -no-integrated-as
-B/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86_64/bin
-ffunction-sections -funwind-tables -g -fstack-protector -fno-short-enums
-Wa,--noexecstack -D__compiler_offsetof=__builtin_offsetof
-Dnan=__builtin_nan
-target arm-linux-androideabi -mllvm -arm-enable-ehabi
--sysroot=/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-I/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-Os -g -fomit-frame-pointer -fdata-sections -ffunction-sections
-funwind-tables
-g0 -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden
-Wsign-compare
-std=gnu++11 -Wno-implicit-exception-spec-mismatch -Wno-abi -c
../../base/memory/discardable_memory_allocator_android.cc -o
obj/base/memory/base.discardable_memory_allocator_android.o
../../base/memory/discardable_memory_allocator_android.cc:84:25:error: no
matching function for call to 'mmap'
void* const address = mmap(
^~~~
/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/sys/mman.h:47:15:
note: candidate function not viable: no known conversion
from 'base::ScopedFD'
(aka 'ScopedGeneric<int, internal::ScopedFDCloseTraits>') to 'int' for 5th
argument
extern void* mmap(void *, size_t, int, int, int, off_t);
^
1 error generated..

https://codereview.chromium.org/191673003/

bre...@chromium.org

unread,
Mar 14, 2014, 3:37:01 PM3/14/14
to viettr...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
Committed patchset #13 manually as r257179.

https://codereview.chromium.org/191673003/

joc...@chromium.org

unread,
Mar 15, 2014, 9:33:58 AM3/15/14
to bre...@chromium.org, viettr...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
A revert of this CL has been created in
https://codereview.chromium.org/201203002/ by joc...@chromium.org.

The reason for reverting is: Doesn't correctly link

/mnt/data/b/build/slave/Chromium_Linux_Codesearch/build/src/third_party/gold/gold64:
warning: hidden symbol 'base::internal::ScopedFDCloseTraits::Free(int)' in
obj/base/files/nacl_helper.scoped_file.o is referenced by DSO lib/libipc.so.

https://codereview.chromium.org/191673003/

a...@chromium.org

unread,
Mar 18, 2014, 2:52:05 AM3/18/14
to bre...@chromium.org, viettr...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
LGTM


https://codereview.chromium.org/191673003/diff/220001/chrome/browser/chromeos/system/automatic_reboot_manager.cc
File chrome/browser/chromeos/system/automatic_reboot_manager.cc (right):

https://codereview.chromium.org/191673003/diff/220001/chrome/browser/chromeos/system/automatic_reboot_manager.cc#newcode68
chrome/browser/chromeos/system/automatic_reboot_manager.cc:68: while
((length = read(fd.get(), buffer, sizeof(buffer))) > 0)
This read should be wrapped in HANDLE_EINTR.

https://codereview.chromium.org/191673003/diff/220001/chrome/test/chromedriver/chrome_launcher.cc
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/191673003/diff/220001/chrome/test/chromedriver/chrome_launcher.cc#newcode258
chrome/test/chromedriver/chrome_launcher.cc:258:
devnull.reset(open("/dev/null", O_WRONLY));
this open should be wrapped in HANDLE_EINTR.

https://codereview.chromium.org/191673003/diff/220001/content/common/sandbox_mac_system_access_unittest.mm
File content/common/sandbox_mac_system_access_unittest.mm (right):

https://codereview.chromium.org/191673003/diff/220001/content/common/sandbox_mac_system_access_unittest.mm#newcode90
content/common/sandbox_mac_system_access_unittest.mm:90: base::ScopedFD
fdes(open("/etc/passwd", O_RDONLY));
This, line 115 and line 108 should also have HANDLE_EINTR.

https://codereview.chromium.org/191673003/diff/220001/sandbox/linux/services/yama.cc
File sandbox/linux/services/yama.cc (right):

https://codereview.chromium.org/191673003/diff/220001/sandbox/linux/services/yama.cc#newcode82
sandbox/linux/services/yama.cc:82: base::ScopedFD
yama_scope(open(kPtraceScopePath, O_RDONLY));
HANDLE_EINTR

https://codereview.chromium.org/191673003/diff/220001/sandbox/linux/services/yama.cc#newcode92
sandbox/linux/services/yama.cc:92: ssize_t num_read =
read(yama_scope.get(), &yama_scope_value, 1);
HANDLE_EINTR

https://codereview.chromium.org/191673003/diff/220001/tools/android/memdump/memdump.cc
File tools/android/memdump/memdump.cc (right):

https://codereview.chromium.org/191673003/diff/220001/tools/android/memdump/memdump.cc#newcode440
tools/android/memdump/memdump.cc:440: base::ScopedFD pagemap_fd(open(
HANDLE_EINTR

https://codereview.chromium.org/191673003/diff/220001/tools/android/memdump/memdump.cc#newcode492
tools/android/memdump/memdump.cc:492: base::ScopedFD
page_count_fd(open("/proc/kpagecount", O_RDONLY));
HANDLE_EINTR and on line 493.

https://codereview.chromium.org/191673003/

bre...@chromium.org

unread,
Mar 18, 2014, 4:21:54 PM3/18/14
to viettr...@chromium.org, a...@chromium.org, chromium...@chromium.org, nkostyl...@chromium.org, gavinp...@chromium.org, stevenj...@chromium.org, extension...@chromium.org, cbentze...@chromium.org, j...@chromium.org, joi+watc...@chromium.org, dari...@chromium.org, yfriedm...@chromium.org, chromium-a...@chromium.org, erikwrig...@chromium.org, fischma...@chromium.org, craigd...@chromium.org, feature-me...@chromium.org, bulach...@chromium.org, oshima...@chromium.org, ilev...@chromium.org, klundbe...@chromium.org, a...@chromium.org, tfa...@chromium.org, mcasas...@chromium.org, robert...@chromium.org, davemoo...@chromium.org, st...@chromium.org, wjia+...@chromium.org, jln+...@chromium.org
Thanks, HANDLE_EINTR suggestions implemented on
https://codereview.chromium.org/203213005/

https://codereview.chromium.org/191673003/
Reply all
Reply to author
Forward
0 new messages