The previous attempt of having EnvPosix use O_CLOEXEC (close-on-exec()) when opening file descriptors added tests that relied on procfs, which is Linux-specific. These tests failed on macOS. Unfortunately, the test failures were not caught due to a (since fixed) error in our Travis CI configuration.
This CL re-structures the tests to only rely on POSIX features. Since there is no POSIX-compliant way to get a file name/path out of a file descriptor, this CL breaks up the O_CLOEXEC test into multiple tests, where each Env method that creates an FD gets its own test. This is intended to make it easier to find and fix errors in Env implementations.
This CL also fixes the implementation of NewLogger() to use O_CLOEXEC on macOS. The current implementation passes "we" to fopen(), but the macOS standard C library does not implement the "e" flag yet.
PiperOrigin-RevId: 247088953
1) Convert iterator-based for loops to C++11 foreach loops.
2) Convert "void operator=" to "T& operator=".
3) Switch from copy operators from private to public deleted.
4) Switch from empty ctors / dtors to "= default" where appropriate.
PiperOrigin-RevId: 246679195
Use clang-format to correct formatting to be in agreement with the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html). Doing this simplifies the process of accepting changes. Also fixed a few warnings flagged by clang-tidy.
PiperOrigin-RevId: 246350737
There were a few members which were identified to have been left
uninitialized in some constructors. These were very likely to
have been set before being used, otherwise the ASan tests would
have caught them, but still good practice to have them
initialized. This addresses some items reported in issue #668.
PiperOrigin-RevId: 243370145
This change switches corruption_test, which previously used direct file
I/O to corrupt table files for open databases, to use InMemEnv. Using an
Env eliminates some platform dependencies thus simplifying the tests.
Also removed EnvWindowsTestHelper::RelaxFilePermissions(). This was
only added because the Windows Env opens files for exclusive access.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=239305329
This CL moves default values for
leveldb::{Options,ReadOptions,WriteOptions} from constructors to member
declarations, and removes now-redundant comments stating the defaults.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=239271242
This CL removes AtomicPointer from leveldb's port interface. Its usage is replaced with std::atomic<> from the C++11 standard library.
AtomicPointer was used to wrap flags, numbers, and pointers, so its instances are replaced with std::atomic<bool>, std::atomic<int>, std::atomic<size_t> and std::atomic<Node*>.
This CL does not revise the memory ordering. AtomicPointer's methods are replaced mechanically with their std::atomic equivalents, even when the underlying usage is incorrect. (Example: DBImpl::has_imm_ is written using release stores, even though it is always read using relaxed ordering.) Revising the memory ordering is left for future CLs.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=237865146
Fixes GitHub issue #657.
This CL also makes the Windows CI green.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=237255887
This change adds a native Windows port (port_windows.h) and a
Windows Env (WindowsEnv).
Note1: "small" is defined when including <Windows.h> so some
parameters were renamed to avoid conflict.
Note2: leveldb::Env defines the method: "DeleteFile" which is
also a constant defined when including <Windows.h>. The solution
was to ensure this macro is defined in env.h which forces
the function, when compiled, to be either DeleteFileA or
DeleteFileW when building for MBCS or UNICODE respectively.
This resolves#519 on GitHub.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=236364778
This prevents file descriptors from leaking to child processes.
When compiled for older (pre-2.6.23) kernels which lack support for
O_CLOEXEC there is no change in behavior. With newer kernels, child
processes will no longer inherit leveldb's file handles, which
reduces the changes of accidentally corrupting the database.
Fixes https://github.com/google/leveldb/issues/623
Apple doesn't follow POSIX specifications for fsync(). Instead, fsync() guarantees to flush the buffer cache to the device, which means the data will survive kernel panics, but may not survive power outages. Applications that need stronger guarantees (like databases) need to use fcntl(F_FULLFSYNC).
This CL switches PosixWritableFile::Sync() to get the stronger guarantees on Apple systems. The improved implementation follows the same principles as SQLite [1] and node.js [2].
Research for the fcntl() to fsync() fallback strategy:
Apple's released source code at https://opensource.apple.com/ shows at least three different error codes being returned when a filesystem does not support F_FULLFSYNC.
fcntl() is implemented in xnu-4903.221.2 in bsd/kern/kern_descrip.c, where it delegates to fcntl_nocancel(). The documentation for fcntl_nocancel() mentions error codes for some operations, but does not include F_FULLFSYNC. The F_FULLSYNC branch in fcntl_nocancel() calls VNOP_IOCTL(_, F_FULLSYNC, NULL, 0, _), whose return value sets the error
code.
VNOP_IOCTL() is implemented in bsd/vfs/kpi_vfs.c and calls the ioctl function in the vnode's operation vector. The per-filesystem function names follow the pattern _vnop_ioctl() for all the instances in opensource code: {hfs,msdosfs,nfs,ntfs,smbfs,webdav,zfs}_vnop_ioctl().
hfs-407.30.1, msdosfs-229.200.3, and nfs in xnu-4903.221.2 handle F_FULLFSYNC. ntfs-94.200.1 and smb-759.40.1 do not handle F_FULLFSYNC, and the default branch returns ENOSUP. webdav-380.200.1 also does not handle F_FULLFSYNC, but the default branch returns EINVAL. zfs-59 also does not handle F_FULLSYNC, and its default branch returns ENOTTY.
From a different angle, Apple's ntfs-94.200.1 includes utility code that uses fcntl(F_FULLFSYNC) and falls back to fsync() just like we do, supporting the hypothesis that there is no good way to detect lack of F_FULLFSYNC support. Also, Apple's fcntl() man page [3] does not mention a way to detect lack of F_FULLFSYNC support.
[1] https://www.sqlite.org/src/doc/trunk/src/os_unix.c
[2] https://github.com/libuv/libuv/blob/master/src/unix/fs.c
[3] https://developer.apple.com/library/archive/documentatiVon/System/Conceptual/ManPages_iPhoneOS/man2/fcntl.2.html
Tested:
https://travis-ci.org/pwnall/leveldb/builds/477318498
TAP global presubmit
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=228593729
The space in between the header and log message was mistakenly omitted
in a prior commit. Re-adding.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=228202737
General cleanup principles:
* Use override when applicable.
* Remove static when redundant (methods and globals in anonymous
namespaces).
* Use const on class members where possible.
* Standardize on "status" for Status local variables.
* Renames where clarity can be improved.
* Qualify standard library names with std:: when possible, to
distinguish from POSIX names.
* Qualify POSIX names with the global namespace (::) when possible, to
distinguish from standard library names.
This also refactors the background thread synchronization logic so that
it's statically analyzable.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=219212089
C++11 guarantees thread-safe initialization of static variables inside
functions. This is a more restricted form of std::call_once or
pthread_once_t (e.g., single call site), so the compiler might be able
to generate better code [1]. Equally important, having less
platform-dependent code in env_posix.cc makes it easier to port to other
platforms.
Due to the change above, this CL introduced a new approach for storing
the singleton PosixEnv instance returned by Env::Default(). The new
approach avoids a dynamic memory allocation, which eliminates the false
positive from LeakSanitizer reported in
https://github.com/google/leveldb/issues/539 and
https://github.com/google/leveldb/issues/113
[1] https://stackoverflow.com/a/27206650/
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=214293129
This commit replaces the use of pthreads in the POSIX port with std::thread
and port::Mutex + port::CondVar. This is intended to simplify porting
the env to a different platform.
The indirect use of pthreads in PosixLogger is replaced with
std:🧵:id(), based on an approach prototyped by @cmumfordx@.
The pthreads dependency in CMakeFiles is not removed, because some C++
standard library implementations must be linked against pthreads for
std::thread use. Figuring out this dependency is left for future work.
Switching away from pthreads also fixes
https://github.com/google/leveldb/issues/381
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212478311
This is not an API-breaking change, because it reduces the API that the
leveldb embedder must implement. The project will build just fine
against ports that still implement InitOnce.
C++11 guarantees thread-safe initialization of static variables inside
functions. This is a more restricted form of std::call_once or
pthread_once_t (e.g., single call site), so the compiler might be able
to generate better code [1]. Equally important, having less code in
port_example.h makes it easier to port to other platforms.
Due to the change above, this CL introduces a new approach for storing
the singleton BytewiseComparatorImpl instance returned by
BytewiseComparator(). The new approach avoids a dynamic memory
allocation, which eliminates the false positive from LeakSanitizer
reported in https://github.com/google/leveldb/issues/200
[1] https://stackoverflow.com/a/27206650/
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=212348004
This is separated from the general cleanup because of the logic changes
in SyncDirIfManifest().
General cleanup principles:
* Use override when applicable.
* Remove static when redundant (methods and globals in anonymous
namespaces).
* Use const on class members where possible.
* Standardize on "status" for Status local variables.
* Renames where clarity can be improved.
* Qualify standard library names with std:: when possible, to
distinguish from POSIX names.
* Qualify POSIX names with the global namespace (::) when possible, to
distinguish from standard library names.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211709673
General cleanup principles:
* Use override when applicable.
* Use const on class members where possible.
* Renames where clarity can be improved.
* Qualify standard library names with std:: when possible, to
distinguish from POSIX names.
* Qualify POSIX names with the global namespace (::) when possible, to
distinguish from standard library names.
This also revamps the logic for putting together a message into the
in-memory buffer before that is passed to fwrite(). While correct in
practice, the current implementation advances a char pointer past the
size of its buffer, which is technically undefined behavior.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211472570
Now that we require C++11, we can use std::atomic<int>, which has
primitives for most of the logic we need. As a bonus, the happy path for
Limiter::Acquire() and Limiter::Release() only performs one atomic
operation.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=211469518
The porting layer implements threading primitives: atomic pointers,
condition variables, mutexes, thread-safe initialization. These are all
specified in C++11, so the reference open source port implementation can
become platform-independent.
The porting layer will remain in place to allow the use of other
implementations with more features, such as the built-in deadlock
detection in abseil's Mutex.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=193245934
Commit a0008deb679480fd30e845d7e52421af72160c2c introduced
std::numeric_limits usage in logging.cc, but didn't #include <limits>
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=192840190
The old implementation caused odd crashes on ARM, which were fixed by
changing a local variable type. The main suspect is the use of a static
local variable. This CL replaces the static local variable with
constexpr, which still ensures the compiler sees the expressions as
constants.
The CL also replaces Slice operations in the functions' inner loop with
iterator-style pointer operations, which can help the compiler generate
less code.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=192832175
ConsumeDecimalNumber has fairly non-trivial logic, and a previous
version has crashed inexplicably on Android. Having some test coverage
will make it easier to tweak / simplify the function later on.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=192821751
After this CL, all classes with Mutex members should be covered by annotations. Exceptions are atomic members, which shouldn't need locking, and DBImpl members that cause errors when annotated, which will be tackled separately.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=190260865
This CL makes it easier to reason about thread safety by:
1) Adding Clang thread safety annotations according to comments.
2) Expanding a couple of variable names, without adding extra lines of code.
3) Adding const in a couple of places.
4) Replacing an always-non-null const pointer with a reference.
5) Fixing style warnings in the modified files.
This CL does not annotate the DBImpl members that claim to be protected
by the instance mutex, but are accessed without the mutex being held.
Those members (and their unprotected accesses) will be addressed in
future CLs.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=189354657
This CL removes unused headers included by util/testharness.h, adds
precise includes where the build breaks, and fixes style errors in the
edited files.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=189331061
External linkage is the default for function declarations in C++.
This also fixes ClangTidy errors generated by removing the "extern"
keyword as described above.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188730416
CL 170738066 introduced std::min and std::max to env_test.cc. These
require the <algorithm> header.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171024062
Deleting a PosixWritableFile without calling Close() leaks the file
descriptor. While the API description in include/leveldb/env.h does not
specify whether the caller is responsible for Close()ing the file before
deleting it, all other Env file implementations do release underlying
resources when destroyed, even if Close() is not called.
The leak shows up when running db_tests on Mac Travis, or on a vanilla
MacOS install.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170906843
This also removes std::unique_ptr introduced in CL 170738066, because
it's C++11-only, and the open source version still supports older
versions at the moment.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170876919
If the file already existed, we should have truncated it. This was not
detected by leveldb tests since leveldb code avoids reusing same files,
but there was code elsewhere that was directly using leveldb files and
relying on this behavior.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170769101
If leveldb::Options::block_cache is set to a cache of zero capacity
then it is possible for LRUHandle::next to be used without having been
set.
Conditional jump or move depends on uninitialised value(s):
leveldb::(anonymous namespace)::LRUHandle::key() const (cache.cc:58)
leveldb::(anonymous namespace)::LRUCache::Unref(leveldb::(anonymous namespace)::LRUHandle*) (cache.cc:234)
leveldb::(anonymous namespace)::LRUCache::Release(leveldb::Cache::Handle*) (cache.cc:266)
leveldb::(anonymous namespace)::ShardedLRUCache::Release(leveldb::Cache::Handle*) (cache.cc:375)
leveldb::CacheTest::Insert(int, int, int) (cache_test.cc:59)
This bug forced a commit reversion in Chromium. For more information see
https://bugs.chromium.org/p/chromium/issues/detail?id=761398#c4
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170749054
env_posix.cc and concurrent application calls to fflush(NULL).
The fix is to avoid using stdio in env_posix.cc but add our own
buffering where we need it.
Added a test to reproduce the bug.
Added a test for Env reads/writes.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170738066
An Android test was occasionally crashing with a SEGV in ConsumeDecimalNumber
Switching a local variable from an int to uint64_t eliminated these crashes.
Speculating this is either a compiler, runtime library, or emulator issue.
Switching this type to uint64_t also eliminates a compiler warning
about comparing an int with a uint64_t.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=166399695
BTRFS reorders rename and write operations, so it is possible that a filesystem crash and recovery results in a situation where the file pointed to by CURRENT does not exist. DB::Open currently reports an I/O error in this case. Reporting database corruption is a better hint to the caller, which can attempt to recover the database or erase it and start over.
This issue is not merely theoretical. It was reported as having showed up in the wild at https://github.com/google/leveldb/issues/195 and at https://crbug.com/738961. Also, asides from the BTRFS case described above, incorrect data in CURRENT seems like a possible corruption case that should be handled gracefully.
The Env API changes here can be considered backwards compatible, because an implementation that returns Status::IOError instead of Status::NotFound will still get the same functionality as before.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=161432630
