Skip to content

Commit 970a27c

Browse files
brsonbrson
authored
Fix clippy warnings etc. (#393)
* Fix various unsafe declarations As reported by clippy. These are all cases where a pointer argument is dereferenced by a declared safe function. The functions are changed to be unsafe functions. Though some of these functions are declared pub, they all seem to be related to internal binding matters and appear to be written correctly, just declared incorrectly, so there shouldn't be any external impact for callers that aren't using the crocksdb API directly. Signed-off-by: Brian Anderson <[email protected]> * Fix some clippy style lints Signed-off-by: Brian Anderson <[email protected]> * Fix mutability of crocksdb_load_latest_options errptr This value can be mutated on the C side. Caught by clippy. Signed-off-by: Brian Anderson <[email protected]> * Cleanup some unit-return functions Signed-off-by: Brian Anderson <[email protected]> * Disable a clippy lint Signed-off-by: Brian Anderson <[email protected]> * make a transmute more typesafe Signed-off-by: Brian Anderson <[email protected]> * Elide a lifetime Signed-off-by: Brian Anderson <[email protected]> * Remove unnecessary unsafe blocks Signed-off-by: Brian Anderson <[email protected]> * Use repr(C) structs of c_void for opaque types. These represent opaque RocksDB types. They are used behind pointers, but are also wrapped in other types in the higher-level bindings. These use the strategy for opaque C types described in the nomicon [1]: but with the exception that they contain c_void instead of [u8; 0], thus making them uninstantiable sized types instead of ZSTs. The c_void documentation publicly recommends using the ZST pattern from the nomicon, but in private documentation [2] warns about UB from dereferencing pointers to uninhabited types, which these bindings do. Additionally, these bindings wrap some these types directly (not through pointers) and it's impossible to repr(transparent) a ZST, without which the unsafe casts within are dubious. [1]: https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs [2]: https://doc.rust-lang.org/nightly/src/core/ffi.rs.html#28 Signed-off-by: Brian Anderson <[email protected]> * Apply repr(transparent) to various unsafe-cast wrappers These are wrappers of opaque FFI types, and pointers are being unsafely cast between the two. repr(transparent) is probably needed to make this well-defined. Signed-off-by: Brian Anderson <[email protected]> * Replace mem::transmute with pointer casts Slightly more typesafe, and more explicit about what's being cast. Signed-off-by: Brian Anderson <[email protected]> * Replace more mem::transmute with casts Signed-off-by: Brian Anderson <[email protected]> * Remove a no-op mem::transmute Signed-off-by: Brian Anderson <[email protected]> * Remove is_power_of_two std contains a branchless bitmagic version of this function Signed-off-by: Brian Anderson <[email protected]> * Add clippy makefile target Signed-off-by: Brian Anderson <[email protected]> * Add clippy to CI Signed-off-by: Brian Anderson <[email protected]> * Fix types in memcpy This code is copying from c_void pointers when it should be copying from byte pointers. Fortunately c_void and u8 have the same size, but I don't know if that is guaranteed. Signed-off-by: Brian Anderson <[email protected]> * Update src/rocksdb.rs Co-Authored-By: kennytm <[email protected]> Signed-off-by: Brian Anderson <[email protected]> * Install clippy on travis Signed-off-by: Brian Anderson <[email protected]> * Adjust clippy lints for CI toolchain Signed-off-by: Brian Anderson <[email protected]> * Make a typesafe cast Signed-off-by: Brian Anderson <[email protected]> * rustfmt Signed-off-by: Brian Anderson <[email protected]>
1 parent 3031723 commit 970a27c

File tree

12 files changed

+253
-213
lines changed

12 files changed

+253
-213
lines changed

‎.travis.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@ cache: false
2424
before_script:
2525
- rustup default nightly-2019-09-05
2626
- rustup component add rustfmt-preview
27+
- rustup component add clippy
2728

2829
script:
30+
- make clippy
2931
- cargo fmt --all -- --check
3032
- cargo build
3133
- cargo test --all

‎Makefile‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@ clean:
1414
@cargo clean
1515
@cd librocksdb_sys && cargo clean
1616

17+
# TODO it could be worth fixing some of these lints
18+
clippy:
19+
@cargo clippy --all -- \
20+
-D warnings \
21+
-A clippy::redundant_field_names -A clippy::single_match \
22+
-A clippy::assign_op_pattern -A clippy::new_without_default -A clippy::useless_let_if_seq \
23+
-A clippy::needless_return -A clippy::len_zero
24+
1725
update_titan:
1826
@if [ -n "${TITAN_REPO}" ]; then \
1927
git config --file=.gitmodules submodule.titan.url https://github.com/${TITAN_REPO}/titan.git; \
@@ -32,4 +40,4 @@ update_rocksdb:
3240
git config --file=.gitmodules submodule.rocksdb.branch ${ROCKSDB_BRANCH}; \
3341
fi
3442
@git submodule sync
35-
@git submodule update --remote librocksdb_sys/rocksdb
43+
@git submodule update --remote librocksdb_sys/rocksdb

‎librocksdb_sys/src/lib.rs‎

Lines changed: 146 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -22,60 +22,134 @@ use libc::{c_char, c_double, c_int, c_uchar, c_void, size_t};
2222
use std::ffi::CStr;
2323
use std::fmt;
2424

25-
pub enum Options {}
26-
pub enum ColumnFamilyDescriptor {}
27-
pub enum DBInstance {}
28-
pub enum DBWriteOptions {}
29-
pub enum DBReadOptions {}
30-
pub enum DBMergeOperator {}
31-
pub enum DBBlockBasedTableOptions {}
32-
pub enum DBMemoryAllocator {}
33-
pub enum DBLRUCacheOptions {}
34-
pub enum DBCache {}
35-
pub enum DBFilterPolicy {}
36-
pub enum DBSnapshot {}
37-
pub enum DBIterator {}
38-
pub enum DBCFHandle {}
39-
pub enum DBWriteBatch {}
40-
pub enum DBComparator {}
41-
pub enum DBFlushOptions {}
42-
pub enum DBCompactionFilter {}
43-
pub enum EnvOptions {}
44-
pub enum SstFileReader {}
45-
pub enum SstFileWriter {}
46-
pub enum ExternalSstFileInfo {}
47-
pub enum IngestExternalFileOptions {}
48-
pub enum DBBackupEngine {}
49-
pub enum DBRestoreOptions {}
50-
pub enum DBSliceTransform {}
51-
pub enum DBRateLimiter {}
52-
pub enum DBLogger {}
53-
pub enum DBCompactOptions {}
54-
pub enum DBFifoCompactionOptions {}
55-
pub enum DBPinnableSlice {}
56-
pub enum DBUserCollectedProperties {}
57-
pub enum DBUserCollectedPropertiesIterator {}
58-
pub enum DBTableProperties {}
59-
pub enum DBTablePropertiesCollection {}
60-
pub enum DBTablePropertiesCollectionIterator {}
61-
pub enum DBTablePropertiesCollector {}
62-
pub enum DBTablePropertiesCollectorFactory {}
63-
pub enum DBFlushJobInfo {}
64-
pub enum DBCompactionJobInfo {}
65-
pub enum DBIngestionInfo {}
66-
pub enum DBEventListener {}
67-
pub enum DBKeyVersions {}
68-
pub enum DBEnv {}
69-
pub enum DBSequentialFile {}
70-
pub enum DBColumnFamilyMetaData {}
71-
pub enum DBLevelMetaData {}
72-
pub enum DBSstFileMetaData {}
73-
pub enum DBCompactionOptions {}
74-
pub enum DBPerfContext {}
75-
pub enum DBIOStatsContext {}
76-
pub enum DBWriteStallInfo {}
77-
pub enum DBStatusPtr {}
78-
pub enum DBMapProperty {}
25+
// FFI-safe opaque types.
26+
//
27+
// These represent opaque RocksDB types. They are used behind pointers, but are
28+
// also wrapped in other types in the higher-level bindings.
29+
//
30+
// These use the strategy for opaque C types described in the nomicon [1]:
31+
// but with the exception that they contain c_void instead of [u8; 0], thus
32+
// making them uninstantiable sized types instead of ZSTs.
33+
//
34+
// The c_void documentation publicly recommends using the ZST pattern from the
35+
// nomicon, but in private documentation [2] warns about UB from dereferencing
36+
// pointers to uninhabited types, which these bindings do.
37+
//
38+
// Additionally, these bindings wrap some these types directly (not through
39+
// pointers) and it's impossible to repr(transparent) a ZST, without which the
40+
// unsafe casts within are dubious.
41+
//
42+
// [1]: https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs
43+
// [2]: https://doc.rust-lang.org/nightly/src/core/ffi.rs.html#28
44+
45+
#[repr(C)]
46+
pub struct Options(c_void);
47+
#[repr(C)]
48+
pub struct ColumnFamilyDescriptor(c_void);
49+
#[repr(C)]
50+
pub struct DBInstance(c_void);
51+
#[repr(C)]
52+
pub struct DBWriteOptions(c_void);
53+
#[repr(C)]
54+
pub struct DBReadOptions(c_void);
55+
#[repr(C)]
56+
pub struct DBMergeOperator(c_void);
57+
#[repr(C)]
58+
pub struct DBBlockBasedTableOptions(c_void);
59+
#[repr(C)]
60+
pub struct DBMemoryAllocator(c_void);
61+
#[repr(C)]
62+
pub struct DBLRUCacheOptions(c_void);
63+
#[repr(C)]
64+
pub struct DBCache(c_void);
65+
#[repr(C)]
66+
pub struct DBFilterPolicy(c_void);
67+
#[repr(C)]
68+
pub struct DBSnapshot(c_void);
69+
#[repr(C)]
70+
pub struct DBIterator(c_void);
71+
#[repr(C)]
72+
pub struct DBCFHandle(c_void);
73+
#[repr(C)]
74+
pub struct DBWriteBatch(c_void);
75+
#[repr(C)]
76+
pub struct DBComparator(c_void);
77+
#[repr(C)]
78+
pub struct DBFlushOptions(c_void);
79+
#[repr(C)]
80+
pub struct DBCompactionFilter(c_void);
81+
#[repr(C)]
82+
pub struct EnvOptions(c_void);
83+
#[repr(C)]
84+
pub struct SstFileReader(c_void);
85+
#[repr(C)]
86+
pub struct SstFileWriter(c_void);
87+
#[repr(C)]
88+
pub struct ExternalSstFileInfo(c_void);
89+
#[repr(C)]
90+
pub struct IngestExternalFileOptions(c_void);
91+
#[repr(C)]
92+
pub struct DBBackupEngine(c_void);
93+
#[repr(C)]
94+
pub struct DBRestoreOptions(c_void);
95+
#[repr(C)]
96+
pub struct DBSliceTransform(c_void);
97+
#[repr(C)]
98+
pub struct DBRateLimiter(c_void);
99+
#[repr(C)]
100+
pub struct DBLogger(c_void);
101+
#[repr(C)]
102+
pub struct DBCompactOptions(c_void);
103+
#[repr(C)]
104+
pub struct DBFifoCompactionOptions(c_void);
105+
#[repr(C)]
106+
pub struct DBPinnableSlice(c_void);
107+
#[repr(C)]
108+
pub struct DBUserCollectedProperties(c_void);
109+
#[repr(C)]
110+
pub struct DBUserCollectedPropertiesIterator(c_void);
111+
#[repr(C)]
112+
pub struct DBTableProperties(c_void);
113+
#[repr(C)]
114+
pub struct DBTablePropertiesCollection(c_void);
115+
#[repr(C)]
116+
pub struct DBTablePropertiesCollectionIterator(c_void);
117+
#[repr(C)]
118+
pub struct DBTablePropertiesCollector(c_void);
119+
#[repr(C)]
120+
pub struct DBTablePropertiesCollectorFactory(c_void);
121+
#[repr(C)]
122+
pub struct DBFlushJobInfo(c_void);
123+
#[repr(C)]
124+
pub struct DBCompactionJobInfo(c_void);
125+
#[repr(C)]
126+
pub struct DBIngestionInfo(c_void);
127+
#[repr(C)]
128+
pub struct DBEventListener(c_void);
129+
#[repr(C)]
130+
pub struct DBKeyVersions(c_void);
131+
#[repr(C)]
132+
pub struct DBEnv(c_void);
133+
#[repr(C)]
134+
pub struct DBSequentialFile(c_void);
135+
#[repr(C)]
136+
pub struct DBColumnFamilyMetaData(c_void);
137+
#[repr(C)]
138+
pub struct DBLevelMetaData(c_void);
139+
#[repr(C)]
140+
pub struct DBSstFileMetaData(c_void);
141+
#[repr(C)]
142+
pub struct DBCompactionOptions(c_void);
143+
#[repr(C)]
144+
pub struct DBPerfContext(c_void);
145+
#[repr(C)]
146+
pub struct DBIOStatsContext(c_void);
147+
#[repr(C)]
148+
pub struct DBWriteStallInfo(c_void);
149+
#[repr(C)]
150+
pub struct DBStatusPtr(c_void);
151+
#[repr(C)]
152+
pub struct DBMapProperty(c_void);
79153

80154
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
81155
#[repr(C)]
@@ -88,8 +162,10 @@ pub enum WriteStallCondition {
88162
mod generated;
89163
pub use generated::*;
90164

91-
pub enum DBTitanDBOptions {}
92-
pub enum DBTitanReadOptions {}
165+
#[repr(C)]
166+
pub struct DBTitanDBOptions(c_void);
167+
#[repr(C)]
168+
pub struct DBTitanReadOptions(c_void);
93169

94170
#[derive(Clone, Debug, Default)]
95171
#[repr(C)]
@@ -103,8 +179,8 @@ pub fn new_bloom_filter(bits: c_int) -> *mut DBFilterPolicy {
103179
unsafe { crocksdb_filterpolicy_create_bloom(bits) }
104180
}
105181

106-
pub fn new_lru_cache(opt: *mut DBLRUCacheOptions) -> *mut DBCache {
107-
unsafe { crocksdb_cache_create_lru(opt) }
182+
pub unsafe fn new_lru_cache(opt: *mut DBLRUCacheOptions) -> *mut DBCache {
183+
crocksdb_cache_create_lru(opt)
108184
}
109185

110186
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -302,12 +378,10 @@ pub enum DBBackgroundErrorReason {
302378
MemTable = 4,
303379
}
304380

305-
pub fn error_message(ptr: *mut c_char) -> String {
306-
let c_str = unsafe { CStr::from_ptr(ptr) };
381+
pub unsafe fn error_message(ptr: *mut c_char) -> String {
382+
let c_str = CStr::from_ptr(ptr);
307383
let s = format!("{}", c_str.to_string_lossy());
308-
unsafe {
309-
libc::free(ptr as *mut c_void);
310-
}
384+
libc::free(ptr as *mut c_void);
311385
s
312386
}
313387

@@ -634,7 +708,7 @@ extern "C" {
634708
cf_descs: *const *mut *mut ColumnFamilyDescriptor,
635709
cf_descs_len: *mut size_t,
636710
ignore_unknown_options: bool,
637-
errptr: *const *mut c_char,
711+
errptr: *mut *mut c_char,
638712
) -> bool;
639713
pub fn crocksdb_ratelimiter_create(
640714
rate_bytes_per_sec: i64,
@@ -863,8 +937,8 @@ extern "C" {
863937
);
864938
pub fn crocksdb_mergeoperator_create(
865939
state: *mut c_void,
866-
destroy: extern "C" fn(*mut c_void) -> (),
867-
full_merge: extern "C" fn(
940+
destroy: unsafe extern "C" fn(*mut c_void) -> (),
941+
full_merge: unsafe extern "C" fn(
868942
arg: *mut c_void,
869943
key: *const c_char,
870944
key_len: size_t,
@@ -876,7 +950,7 @@ extern "C" {
876950
success: *mut u8,
877951
new_value_length: *mut size_t,
878952
) -> *const c_char,
879-
partial_merge: extern "C" fn(
953+
partial_merge: unsafe extern "C" fn(
880954
arg: *mut c_void,
881955
key: *const c_char,
882956
key_len: size_t,
@@ -887,9 +961,9 @@ extern "C" {
887961
new_value_length: *mut size_t,
888962
) -> *const c_char,
889963
delete_value: Option<
890-
extern "C" fn(*mut c_void, value: *const c_char, value_len: *mut size_t) -> (),
964+
unsafe extern "C" fn(*mut c_void, value: *const c_char, value_len: *mut size_t) -> (),
891965
>,
892-
name_fn: extern "C" fn(*mut c_void) -> *const c_char,
966+
name_fn: unsafe extern "C" fn(*mut c_void) -> *const c_char,
893967
) -> *mut DBMergeOperator;
894968
pub fn crocksdb_mergeoperator_destroy(mo: *mut DBMergeOperator);
895969
pub fn crocksdb_options_set_merge_operator(options: *mut Options, mo: *mut DBMergeOperator);
@@ -1005,15 +1079,15 @@ extern "C" {
10051079
pub fn crocksdb_options_set_comparator(options: *mut Options, cb: *mut DBComparator);
10061080
pub fn crocksdb_comparator_create(
10071081
state: *mut c_void,
1008-
destroy: extern "C" fn(*mut c_void) -> (),
1009-
compare: extern "C" fn(
1082+
destroy: unsafe extern "C" fn(*mut c_void) -> (),
1083+
compare: unsafe extern "C" fn(
10101084
arg: *mut c_void,
10111085
a: *const c_char,
10121086
alen: size_t,
10131087
b: *const c_char,
10141088
blen: size_t,
10151089
) -> c_int,
1016-
name_fn: extern "C" fn(*mut c_void) -> *const c_char,
1090+
name_fn: unsafe extern "C" fn(*mut c_void) -> *const c_char,
10171091
) -> *mut DBComparator;
10181092
pub fn crocksdb_comparator_destroy(cmp: *mut DBComparator);
10191093

‎src/comparator.rs‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -15,38 +15,33 @@
1515

1616
use libc::{c_char, c_int, c_void, size_t};
1717
use std::ffi::CString;
18-
use std::mem;
1918
use std::slice;
2019

2120
pub struct ComparatorCallback {
2221
pub name: CString,
2322
pub f: fn(&[u8], &[u8]) -> i32,
2423
}
2524

26-
pub extern "C" fn destructor_callback(raw_cb: *mut c_void) {
25+
pub unsafe extern "C" fn destructor_callback(raw_cb: *mut c_void) {
2726
// turn this back into a local variable so rust will reclaim it
28-
let _: Box<ComparatorCallback> = unsafe { mem::transmute(raw_cb) };
27+
let _ = Box::from_raw(raw_cb as *mut ComparatorCallback);
2928
}
3029

31-
pub extern "C" fn name_callback(raw_cb: *mut c_void) -> *const c_char {
32-
unsafe {
33-
let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
34-
let ptr = cb.name.as_ptr();
35-
ptr as *const c_char
36-
}
30+
pub unsafe extern "C" fn name_callback(raw_cb: *mut c_void) -> *const c_char {
31+
let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
32+
let ptr = cb.name.as_ptr();
33+
ptr as *const c_char
3734
}
3835

39-
pub extern "C" fn compare_callback(
36+
pub unsafe extern "C" fn compare_callback(
4037
raw_cb: *mut c_void,
4138
a_raw: *const c_char,
4239
a_len: size_t,
4340
b_raw: *const c_char,
4441
b_len: size_t,
4542
) -> c_int {
46-
unsafe {
47-
let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
48-
let a: &[u8] = slice::from_raw_parts(a_raw as *const u8, a_len as usize);
49-
let b: &[u8] = slice::from_raw_parts(b_raw as *const u8, b_len as usize);
50-
(cb.f)(a, b)
51-
}
43+
let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
44+
let a: &[u8] = slice::from_raw_parts(a_raw as *const u8, a_len as usize);
45+
let b: &[u8] = slice::from_raw_parts(b_raw as *const u8, b_len as usize);
46+
(cb.f)(a, b)
5247
}

0 commit comments

Comments
 (0)