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 <andersrb@gmail.com>

* Fix some clippy style lints
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* 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 <andersrb@gmail.com>

* Cleanup some unit-return functions
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Disable a clippy lint
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* make a transmute more typesafe
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Elide a lifetime
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Remove unnecessary unsafe blocks
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* 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#28Signed-off-by: Brian Anderson <andersrb@gmail.com>

* 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 <andersrb@gmail.com>

* Replace mem::transmute with pointer casts

Slightly more typesafe, and more explicit about what's being cast.
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Replace more mem::transmute with casts
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Remove a no-op mem::transmute
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Remove is_power_of_two

std contains a branchless bitmagic version of this function
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Add clippy makefile target
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Add clippy to CI
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* 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 <andersrb@gmail.com>

* Update src/rocksdb.rs
Co-Authored-By: kennytm <kennytm@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Install clippy on travis
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Adjust clippy lints for CI toolchain
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Make a typesafe cast
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* rustfmt
Signed-off-by: Brian Anderson <andersrb@gmail.com>

.travis.yml

@@ -24,8 +24,10 @@ cache: false
 before_script:
   - rustup default nightly-2019-09-05
   - rustup component add rustfmt-preview
+  - rustup component add clippy
 
 script:
+  - make clippy
   - cargo fmt --all -- --check
   - cargo build
   - cargo test --all

Makefile

@@ -14,6 +14,14 @@ clean:
 	@cargo clean
 	@cd librocksdb_sys && cargo clean
 
+# TODO it could be worth fixing some of these lints
+clippy:
+	@cargo clippy --all -- \
+	-D warnings \
+	-A clippy::redundant_field_names -A clippy::single_match \
+	-A clippy::assign_op_pattern -A clippy::new_without_default -A clippy::useless_let_if_seq \
+	-A clippy::needless_return -A clippy::len_zero
+
 update_titan:
 	@if [ -n "${TITAN_REPO}" ]; then \
 		git config --file=.gitmodules submodule.titan.url https://github.com/${TITAN_REPO}/titan.git; \
@@ -32,4 +40,4 @@ update_rocksdb:
 		git config --file=.gitmodules submodule.rocksdb.branch ${ROCKSDB_BRANCH}; \
 	fi
 	@git submodule sync
-	@git submodule update --remote librocksdb_sys/rocksdb
\ No newline at end of file
+	@git submodule update --remote librocksdb_sys/rocksdb

librocksdb_sys/src/lib.rs

@@ -22,60 +22,134 @@ use libc::{c_char, c_double, c_int, c_uchar, c_void, size_t};
 use std::ffi::CStr;
 use std::fmt;
 
-pub enum Options {}
-pub enum ColumnFamilyDescriptor {}
-pub enum DBInstance {}
-pub enum DBWriteOptions {}
-pub enum DBReadOptions {}
-pub enum DBMergeOperator {}
-pub enum DBBlockBasedTableOptions {}
-pub enum DBMemoryAllocator {}
-pub enum DBLRUCacheOptions {}
-pub enum DBCache {}
-pub enum DBFilterPolicy {}
-pub enum DBSnapshot {}
-pub enum DBIterator {}
-pub enum DBCFHandle {}
-pub enum DBWriteBatch {}
-pub enum DBComparator {}
-pub enum DBFlushOptions {}
-pub enum DBCompactionFilter {}
-pub enum EnvOptions {}
-pub enum SstFileReader {}
-pub enum SstFileWriter {}
-pub enum ExternalSstFileInfo {}
-pub enum IngestExternalFileOptions {}
-pub enum DBBackupEngine {}
-pub enum DBRestoreOptions {}
-pub enum DBSliceTransform {}
-pub enum DBRateLimiter {}
-pub enum DBLogger {}
-pub enum DBCompactOptions {}
-pub enum DBFifoCompactionOptions {}
-pub enum DBPinnableSlice {}
-pub enum DBUserCollectedProperties {}
-pub enum DBUserCollectedPropertiesIterator {}
-pub enum DBTableProperties {}
-pub enum DBTablePropertiesCollection {}
-pub enum DBTablePropertiesCollectionIterator {}
-pub enum DBTablePropertiesCollector {}
-pub enum DBTablePropertiesCollectorFactory {}
-pub enum DBFlushJobInfo {}
-pub enum DBCompactionJobInfo {}
-pub enum DBIngestionInfo {}
-pub enum DBEventListener {}
-pub enum DBKeyVersions {}
-pub enum DBEnv {}
-pub enum DBSequentialFile {}
-pub enum DBColumnFamilyMetaData {}
-pub enum DBLevelMetaData {}
-pub enum DBSstFileMetaData {}
-pub enum DBCompactionOptions {}
-pub enum DBPerfContext {}
-pub enum DBIOStatsContext {}
-pub enum DBWriteStallInfo {}
-pub enum DBStatusPtr {}
-pub enum DBMapProperty {}
+// FFI-safe 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
+
+#[repr(C)]
+pub struct Options(c_void);
+#[repr(C)]
+pub struct ColumnFamilyDescriptor(c_void);
+#[repr(C)]
+pub struct DBInstance(c_void);
+#[repr(C)]
+pub struct DBWriteOptions(c_void);
+#[repr(C)]
+pub struct DBReadOptions(c_void);
+#[repr(C)]
+pub struct DBMergeOperator(c_void);
+#[repr(C)]
+pub struct DBBlockBasedTableOptions(c_void);
+#[repr(C)]
+pub struct DBMemoryAllocator(c_void);
+#[repr(C)]
+pub struct DBLRUCacheOptions(c_void);
+#[repr(C)]
+pub struct DBCache(c_void);
+#[repr(C)]
+pub struct DBFilterPolicy(c_void);
+#[repr(C)]
+pub struct DBSnapshot(c_void);
+#[repr(C)]
+pub struct DBIterator(c_void);
+#[repr(C)]
+pub struct DBCFHandle(c_void);
+#[repr(C)]
+pub struct DBWriteBatch(c_void);
+#[repr(C)]
+pub struct DBComparator(c_void);
+#[repr(C)]
+pub struct DBFlushOptions(c_void);
+#[repr(C)]
+pub struct DBCompactionFilter(c_void);
+#[repr(C)]
+pub struct EnvOptions(c_void);
+#[repr(C)]
+pub struct SstFileReader(c_void);
+#[repr(C)]
+pub struct SstFileWriter(c_void);
+#[repr(C)]
+pub struct ExternalSstFileInfo(c_void);
+#[repr(C)]
+pub struct IngestExternalFileOptions(c_void);
+#[repr(C)]
+pub struct DBBackupEngine(c_void);
+#[repr(C)]
+pub struct DBRestoreOptions(c_void);
+#[repr(C)]
+pub struct DBSliceTransform(c_void);
+#[repr(C)]
+pub struct DBRateLimiter(c_void);
+#[repr(C)]
+pub struct DBLogger(c_void);
+#[repr(C)]
+pub struct DBCompactOptions(c_void);
+#[repr(C)]
+pub struct DBFifoCompactionOptions(c_void);
+#[repr(C)]
+pub struct DBPinnableSlice(c_void);
+#[repr(C)]
+pub struct DBUserCollectedProperties(c_void);
+#[repr(C)]
+pub struct DBUserCollectedPropertiesIterator(c_void);
+#[repr(C)]
+pub struct DBTableProperties(c_void);
+#[repr(C)]
+pub struct DBTablePropertiesCollection(c_void);
+#[repr(C)]
+pub struct DBTablePropertiesCollectionIterator(c_void);
+#[repr(C)]
+pub struct DBTablePropertiesCollector(c_void);
+#[repr(C)]
+pub struct DBTablePropertiesCollectorFactory(c_void);
+#[repr(C)]
+pub struct DBFlushJobInfo(c_void);
+#[repr(C)]
+pub struct DBCompactionJobInfo(c_void);
+#[repr(C)]
+pub struct DBIngestionInfo(c_void);
+#[repr(C)]
+pub struct DBEventListener(c_void);
+#[repr(C)]
+pub struct DBKeyVersions(c_void);
+#[repr(C)]
+pub struct DBEnv(c_void);
+#[repr(C)]
+pub struct DBSequentialFile(c_void);
+#[repr(C)]
+pub struct DBColumnFamilyMetaData(c_void);
+#[repr(C)]
+pub struct DBLevelMetaData(c_void);
+#[repr(C)]
+pub struct DBSstFileMetaData(c_void);
+#[repr(C)]
+pub struct DBCompactionOptions(c_void);
+#[repr(C)]
+pub struct DBPerfContext(c_void);
+#[repr(C)]
+pub struct DBIOStatsContext(c_void);
+#[repr(C)]
+pub struct DBWriteStallInfo(c_void);
+#[repr(C)]
+pub struct DBStatusPtr(c_void);
+#[repr(C)]
+pub struct DBMapProperty(c_void);
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 #[repr(C)]
@@ -88,8 +162,10 @@ pub enum WriteStallCondition {
 mod generated;
 pub use generated::*;
 
-pub enum DBTitanDBOptions {}
-pub enum DBTitanReadOptions {}
+#[repr(C)]
+pub struct DBTitanDBOptions(c_void);
+#[repr(C)]
+pub struct DBTitanReadOptions(c_void);
 
 #[derive(Clone, Debug, Default)]
 #[repr(C)]
@@ -103,8 +179,8 @@ pub fn new_bloom_filter(bits: c_int) -> *mut DBFilterPolicy {
     unsafe { crocksdb_filterpolicy_create_bloom(bits) }
 }
 
-pub fn new_lru_cache(opt: *mut DBLRUCacheOptions) -> *mut DBCache {
-    unsafe { crocksdb_cache_create_lru(opt) }
+pub unsafe fn new_lru_cache(opt: *mut DBLRUCacheOptions) -> *mut DBCache {
+    crocksdb_cache_create_lru(opt)
 }
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -302,12 +378,10 @@ pub enum DBBackgroundErrorReason {
     MemTable = 4,
 }
 
-pub fn error_message(ptr: *mut c_char) -> String {
-    let c_str = unsafe { CStr::from_ptr(ptr) };
+pub unsafe fn error_message(ptr: *mut c_char) -> String {
+    let c_str = CStr::from_ptr(ptr);
     let s = format!("{}", c_str.to_string_lossy());
-    unsafe {
-        libc::free(ptr as *mut c_void);
-    }
+    libc::free(ptr as *mut c_void);
     s
 }
 
@@ -634,7 +708,7 @@ extern "C" {
         cf_descs: *const *mut *mut ColumnFamilyDescriptor,
         cf_descs_len: *mut size_t,
         ignore_unknown_options: bool,
-        errptr: *const *mut c_char,
+        errptr: *mut *mut c_char,
     ) -> bool;
     pub fn crocksdb_ratelimiter_create(
         rate_bytes_per_sec: i64,
@@ -863,8 +937,8 @@ extern "C" {
     );
     pub fn crocksdb_mergeoperator_create(
         state: *mut c_void,
-        destroy: extern "C" fn(*mut c_void) -> (),
-        full_merge: extern "C" fn(
+        destroy: unsafe extern "C" fn(*mut c_void) -> (),
+        full_merge: unsafe extern "C" fn(
             arg: *mut c_void,
             key: *const c_char,
             key_len: size_t,
@@ -876,7 +950,7 @@ extern "C" {
             success: *mut u8,
             new_value_length: *mut size_t,
         ) -> *const c_char,
-        partial_merge: extern "C" fn(
+        partial_merge: unsafe extern "C" fn(
             arg: *mut c_void,
             key: *const c_char,
             key_len: size_t,
@@ -887,9 +961,9 @@ extern "C" {
             new_value_length: *mut size_t,
         ) -> *const c_char,
         delete_value: Option<
-            extern "C" fn(*mut c_void, value: *const c_char, value_len: *mut size_t) -> (),
+            unsafe extern "C" fn(*mut c_void, value: *const c_char, value_len: *mut size_t) -> (),
         >,
-        name_fn: extern "C" fn(*mut c_void) -> *const c_char,
+        name_fn: unsafe extern "C" fn(*mut c_void) -> *const c_char,
     ) -> *mut DBMergeOperator;
     pub fn crocksdb_mergeoperator_destroy(mo: *mut DBMergeOperator);
     pub fn crocksdb_options_set_merge_operator(options: *mut Options, mo: *mut DBMergeOperator);
@@ -1005,15 +1079,15 @@ extern "C" {
     pub fn crocksdb_options_set_comparator(options: *mut Options, cb: *mut DBComparator);
     pub fn crocksdb_comparator_create(
         state: *mut c_void,
-        destroy: extern "C" fn(*mut c_void) -> (),
-        compare: extern "C" fn(
+        destroy: unsafe extern "C" fn(*mut c_void) -> (),
+        compare: unsafe extern "C" fn(
             arg: *mut c_void,
             a: *const c_char,
             alen: size_t,
             b: *const c_char,
             blen: size_t,
         ) -> c_int,
-        name_fn: extern "C" fn(*mut c_void) -> *const c_char,
+        name_fn: unsafe extern "C" fn(*mut c_void) -> *const c_char,
     ) -> *mut DBComparator;
     pub fn crocksdb_comparator_destroy(cmp: *mut DBComparator);
 

src/comparator.rs

@@ -15,7 +15,6 @@
 
 use libc::{c_char, c_int, c_void, size_t};
 use std::ffi::CString;
-use std::mem;
 use std::slice;
 
 pub struct ComparatorCallback {
@@ -23,30 +22,26 @@ pub struct ComparatorCallback {
     pub f: fn(&[u8], &[u8]) -> i32,
 }
 
-pub extern "C" fn destructor_callback(raw_cb: *mut c_void) {
+pub unsafe extern "C" fn destructor_callback(raw_cb: *mut c_void) {
     // turn this back into a local variable so rust will reclaim it
-    let _: Box<ComparatorCallback> = unsafe { mem::transmute(raw_cb) };
+    let _ = Box::from_raw(raw_cb as *mut ComparatorCallback);
 }
 
-pub extern "C" fn name_callback(raw_cb: *mut c_void) -> *const c_char {
-    unsafe {
-        let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
-        let ptr = cb.name.as_ptr();
-        ptr as *const c_char
-    }
+pub unsafe extern "C" fn name_callback(raw_cb: *mut c_void) -> *const c_char {
+    let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
+    let ptr = cb.name.as_ptr();
+    ptr as *const c_char
 }
 
-pub extern "C" fn compare_callback(
+pub unsafe extern "C" fn compare_callback(
     raw_cb: *mut c_void,
     a_raw: *const c_char,
     a_len: size_t,
     b_raw: *const c_char,
     b_len: size_t,
 ) -> c_int {
-    unsafe {
-        let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
-        let a: &[u8] = slice::from_raw_parts(a_raw as *const u8, a_len as usize);
-        let b: &[u8] = slice::from_raw_parts(b_raw as *const u8, b_len as usize);
-        (cb.f)(a, b)
-    }
+    let cb: &mut ComparatorCallback = &mut *(raw_cb as *mut ComparatorCallback);
+    let a: &[u8] = slice::from_raw_parts(a_raw as *const u8, a_len as usize);
+    let b: &[u8] = slice::from_raw_parts(b_raw as *const u8, b_len as usize);
+    (cb.f)(a, b)
 }

src/event_listener.rs

@@ -18,7 +18,7 @@ use crocksdb_ffi::{
 };
 use libc::c_void;
 use std::path::Path;
-use std::{mem, slice, str};
+use std::{slice, str};
 use {TableProperties, TablePropertiesCollectionView};
 
 macro_rules! fetch_str {
@@ -30,6 +30,7 @@ macro_rules! fetch_str {
     })
 }
 
+#[repr(transparent)]
 pub struct FlushJobInfo(DBFlushJobInfo);
 
 impl FlushJobInfo {
@@ -58,6 +59,7 @@ impl FlushJobInfo {
     }
 }
 
+#[repr(transparent)]
 pub struct CompactionJobInfo(DBCompactionJobInfo);
 
 impl CompactionJobInfo {
@@ -128,6 +130,7 @@ impl CompactionJobInfo {
     }
 }
 
+#[repr(transparent)]
 pub struct IngestionInfo(DBIngestionInfo);
 
 impl IngestionInfo {
@@ -152,6 +155,7 @@ impl IngestionInfo {
     }
 }
 
+#[repr(transparent)]
 pub struct WriteStallInfo(DBWriteStallInfo);
 
 impl WriteStallInfo {
@@ -199,7 +203,7 @@ extern "C" fn on_flush_completed(
     let (ctx, info) = unsafe {
         (
             &*(ctx as *mut Box<dyn EventListener>),
-            mem::transmute(&*info),
+            &*(info as *const FlushJobInfo),
         )
     };
     ctx.on_flush_completed(info);
@@ -213,7 +217,7 @@ extern "C" fn on_compaction_completed(
     let (ctx, info) = unsafe {
         (
             &*(ctx as *mut Box<dyn EventListener>),
-            mem::transmute(&*info),
+            &*(info as *const CompactionJobInfo),
         )
     };
     ctx.on_compaction_completed(info);
@@ -227,7 +231,7 @@ extern "C" fn on_external_file_ingested(
     let (ctx, info) = unsafe {
         (
             &*(ctx as *mut Box<dyn EventListener>),
-            mem::transmute(&*info),
+            &*(info as *const IngestionInfo),
         )
     };
     ctx.on_external_file_ingested(info);
@@ -254,7 +258,7 @@ extern "C" fn on_stall_conditions_changed(ctx: *mut c_void, info: *const DBWrite
     let (ctx, info) = unsafe {
         (
             &*(ctx as *mut Box<dyn EventListener>),
-            mem::transmute(&*info),
+            &*(info as *const WriteStallInfo),
         )
     };
     ctx.on_stall_conditions_changed(info);

src/lib.rs

@@ -68,4 +68,3 @@ mod table_properties;
 mod table_properties_collector;
 mod table_properties_collector_factory;
 mod titan;
-mod util;

src/merge_operator.rs

@@ -26,20 +26,18 @@ pub struct MergeOperatorCallback {
     pub merge_fn: MergeFn,
 }
 
-pub extern "C" fn destructor_callback(raw_cb: *mut c_void) {
+pub unsafe extern "C" fn destructor_callback(raw_cb: *mut c_void) {
     // turn this back into a local variable so rust will reclaim it
-    let _: Box<MergeOperatorCallback> = unsafe { mem::transmute(raw_cb) };
+    let _ = Box::from_raw(raw_cb as *mut MergeOperatorCallback);
 }
 
-pub extern "C" fn name_callback(raw_cb: *mut c_void) -> *const c_char {
-    unsafe {
-        let cb: &mut MergeOperatorCallback = &mut *(raw_cb as *mut MergeOperatorCallback);
-        let ptr = cb.name.as_ptr();
-        ptr as *const c_char
-    }
+pub unsafe extern "C" fn name_callback(raw_cb: *mut c_void) -> *const c_char {
+    let cb: &mut MergeOperatorCallback = &mut *(raw_cb as *mut MergeOperatorCallback);
+    let ptr = cb.name.as_ptr();
+    ptr as *const c_char
 }
 
-pub extern "C" fn full_merge_callback(
+pub unsafe extern "C" fn full_merge_callback(
     raw_cb: *mut c_void,
     raw_key: *const c_char,
     key_len: size_t,
@@ -51,25 +49,24 @@ pub extern "C" fn full_merge_callback(
     success: *mut u8,
     new_value_length: *mut size_t,
 ) -> *const c_char {
-    unsafe {
-        let cb: &mut MergeOperatorCallback = &mut *(raw_cb as *mut MergeOperatorCallback);
-        let operands = &mut MergeOperands::new(operands_list, operands_list_len, num_operands);
-        let key: &[u8] = slice::from_raw_parts(raw_key as *const u8, key_len as usize);
-        let oldval: &[u8] =
-            slice::from_raw_parts(existing_value as *const u8, existing_value_len as usize);
-        let mut result = (cb.merge_fn)(key, Some(oldval), operands);
-        result.shrink_to_fit();
-        // TODO(tan) investigate zero-copy techniques to improve performance
-        let buf = libc::malloc(result.len() as size_t);
-        assert!(!buf.is_null());
-        *new_value_length = result.len() as size_t;
-        *success = 1 as u8;
-        ptr::copy(result.as_ptr() as *mut c_void, &mut *buf, result.len());
-        buf as *const c_char
-    }
+    let cb: &mut MergeOperatorCallback = &mut *(raw_cb as *mut MergeOperatorCallback);
+    let operands = &mut MergeOperands::new(operands_list, operands_list_len, num_operands);
+    let key: &[u8] = slice::from_raw_parts(raw_key as *const u8, key_len as usize);
+    let oldval: &[u8] =
+        slice::from_raw_parts(existing_value as *const u8, existing_value_len as usize);
+    let mut result = (cb.merge_fn)(key, Some(oldval), operands);
+    result.shrink_to_fit();
+    // TODO(tan) investigate zero-copy techniques to improve performance
+    let buf = libc::malloc(result.len() as size_t);
+    let buf = buf as *mut u8;
+    assert!(!buf.is_null());
+    *new_value_length = result.len() as size_t;
+    *success = 1 as u8;
+    ptr::copy(result.as_ptr(), &mut *buf, result.len());
+    buf as *const c_char
 }
 
-pub extern "C" fn partial_merge_callback(
+pub unsafe extern "C" fn partial_merge_callback(
     raw_cb: *mut c_void,
     raw_key: *const c_char,
     key_len: size_t,
@@ -79,20 +76,19 @@ pub extern "C" fn partial_merge_callback(
     success: *mut u8,
     new_value_length: *mut size_t,
 ) -> *const c_char {
-    unsafe {
-        let cb: &mut MergeOperatorCallback = &mut *(raw_cb as *mut MergeOperatorCallback);
-        let operands = &mut MergeOperands::new(operands_list, operands_list_len, num_operands);
-        let key: &[u8] = slice::from_raw_parts(raw_key as *const u8, key_len as usize);
-        let mut result = (cb.merge_fn)(key, None, operands);
-        result.shrink_to_fit();
-        // TODO(tan) investigate zero-copy techniques to improve performance
-        let buf = libc::malloc(result.len() as size_t);
-        assert!(!buf.is_null());
-        *new_value_length = result.len() as size_t;
-        *success = 1 as u8;
-        ptr::copy(result.as_ptr() as *mut c_void, &mut *buf, result.len());
-        buf as *const c_char
-    }
+    let cb: &mut MergeOperatorCallback = &mut *(raw_cb as *mut MergeOperatorCallback);
+    let operands = &mut MergeOperands::new(operands_list, operands_list_len, num_operands);
+    let key: &[u8] = slice::from_raw_parts(raw_key as *const u8, key_len as usize);
+    let mut result = (cb.merge_fn)(key, None, operands);
+    result.shrink_to_fit();
+    // TODO(tan) investigate zero-copy techniques to improve performance
+    let buf = libc::malloc(result.len() as size_t);
+    let buf = buf as *mut u8;
+    assert!(!buf.is_null());
+    *new_value_length = result.len() as size_t;
+    *success = 1 as u8;
+    ptr::copy(result.as_ptr(), &mut *buf, result.len());
+    buf as *const c_char
 }
 
 pub struct MergeOperands {
@@ -133,10 +129,10 @@ impl<'a> Iterator for &'a mut MergeOperands {
                 let len = *len_ptr as usize;
                 let ptr = base + (spacing * self.cursor);
                 self.cursor += 1;
-                Some(mem::transmute(slice::from_raw_parts(
+                Some(slice::from_raw_parts(
                     *(ptr as *const *const u8) as *const u8,
                     len,
-                )))
+                ))
             }
         }
     }

src/rocksdb.rs

@@ -39,7 +39,6 @@ use std::str::from_utf8;
 use std::sync::Arc;
 use std::{fs, ptr, slice};
 use table_properties::{TableProperties, TablePropertiesCollection};
-use util::is_power_of_two;
 
 pub struct CFHandle {
     inner: *mut DBCFHandle,
@@ -83,7 +82,7 @@ fn split_descriptors<'a>(
 
 fn build_cstring_list(str_list: &[&str]) -> Vec<CString> {
     str_list
-        .into_iter()
+        .iter()
         .map(|s| CString::new(s.as_bytes()).unwrap())
         .collect()
 }
@@ -262,6 +261,7 @@ impl<D> DBIterator<D> {
         self.valid()
     }
 
+    #[allow(clippy::should_implement_trait)]
     pub fn next(&mut self) -> bool {
         unsafe {
             crocksdb_ffi::crocksdb_iter_next(self.inner);
@@ -557,11 +557,7 @@ impl DB {
             .map(|x| x.titan_inner as *const crocksdb_ffi::DBTitanDBOptions)
             .collect();
 
-        let readonly = if error_if_log_file_exist.is_some() {
-            true
-        } else {
-            false
-        };
+        let readonly = error_if_log_file_exist.is_some();
 
         let with_ttl = if ttls_vec.len() > 0 {
             if ttls_vec.len() == cf_names.len() {
@@ -2121,11 +2117,9 @@ impl SstFileReader {
         let path =
             CString::new(name.to_owned()).map_err(|e| format!("invalid path {}: {:?}", name, e))?;
         unsafe {
-            Ok(ffi_try!(crocksdb_sstfilereader_open(
-                self.inner,
-                path.as_ptr()
-            )))
+            ffi_try!(crocksdb_sstfilereader_open(self.inner, path.as_ptr()));
         }
+        Ok(())
     }
 
     pub fn iter(&self) -> DBIterator<&Self> {
@@ -2188,7 +2182,8 @@ impl SstFileReader {
     }
 
     pub fn verify_checksum(&self) -> Result<(), String> {
-        unsafe { Ok(ffi_try!(crocksdb_sstfilereader_verify_checksum(self.inner))) }
+        unsafe { ffi_try!(crocksdb_sstfilereader_verify_checksum(self.inner)) };
+        Ok(())
     }
 }
 
@@ -2240,11 +2235,9 @@ impl SstFileWriter {
             Ok(p) => p,
         };
         unsafe {
-            Ok(ffi_try!(crocksdb_sstfilewriter_open(
-                self.inner,
-                path.as_ptr()
-            )))
+            ffi_try!(crocksdb_sstfilewriter_open(self.inner, path.as_ptr()));
         }
+        Ok(())
     }
 
     /// Add key, value to currently opened file
@@ -2425,7 +2418,7 @@ impl Env {
     // The recommanded block size are 1024, 512 and 256.
     pub fn new_ctr_encrypted_env(base_env: Arc<Env>, ciphertext: &[u8]) -> Result<Env, String> {
         let len = ciphertext.len();
-        if len > 2048 || !is_power_of_two(len) {
+        if len > 2048 || !len.is_power_of_two() {
             return Err(
                 "ciphertext length must be less or equal to 2048, and must be power of 2"
                     .to_owned(),
@@ -2434,7 +2427,7 @@ impl Env {
         let env = unsafe {
             crocksdb_ffi::crocksdb_ctr_encrypted_env_create(
                 base_env.inner,
-                mem::transmute(&ciphertext[0]),
+                ciphertext.as_ptr() as *const c_char,
                 len,
             )
         };
@@ -2542,8 +2535,11 @@ pub struct Cache {
 
 impl Cache {
     pub fn new_lru_cache(opt: LRUCacheOptions) -> Cache {
-        Cache {
-            inner: crocksdb_ffi::new_lru_cache(opt.inner),
+        // This is ok because LRUCacheOptions always contains a valid pointer
+        unsafe {
+            Cache {
+                inner: crocksdb_ffi::new_lru_cache(opt.inner),
+            }
         }
     }
 }
@@ -2608,14 +2604,14 @@ pub fn load_latest_options(
     let dbpath = CString::new(dbpath.as_bytes()).map_err(|_| ERR_CONVERT_PATH.to_owned())?;
     let db_options = DBOptions::new();
     unsafe {
-        let mut raw_cf_descs: *mut *mut crocksdb_ffi::ColumnFamilyDescriptor = ptr::null_mut();
+        let raw_cf_descs: *mut *mut crocksdb_ffi::ColumnFamilyDescriptor = ptr::null_mut();
         let mut cf_descs_len: size_t = 0;
 
         let ok = ffi_try!(crocksdb_load_latest_options(
             dbpath.as_ptr(),
             env.inner,
             db_options.inner,
-            &mut raw_cf_descs,
+            &raw_cf_descs,
             &mut cf_descs_len,
             ignore_unknown_options
         ));
@@ -2624,7 +2620,7 @@ pub fn load_latest_options(
         }
         let cf_descs_list = slice::from_raw_parts(raw_cf_descs, cf_descs_len);
         let cf_descs = cf_descs_list
-            .into_iter()
+            .iter()
             .map(|raw_cf_desc| CColumnFamilyDescriptor::from_raw(*raw_cf_desc))
             .collect();
 
@@ -2634,7 +2630,7 @@ pub fn load_latest_options(
     }
 }
 
-pub fn run_ldb_tool(ldb_args: &Vec<String>, opts: &DBOptions) {
+pub fn run_ldb_tool(ldb_args: &[String], opts: &DBOptions) {
     unsafe {
         let ldb_args_cstrs: Vec<_> = ldb_args
             .iter()

src/rocksdb_options.rs

@@ -31,7 +31,6 @@ use rocksdb::Env;
 use rocksdb::{Cache, MemoryAllocator};
 use slice_transform::{new_slice_transform, SliceTransform};
 use std::ffi::{CStr, CString};
-use std::mem;
 use std::path::Path;
 use std::ptr;
 use std::sync::Arc;
@@ -454,9 +453,10 @@ impl ReadOptions {
     pub fn set_table_filter(&mut self, filter: Box<dyn TableFilter>) {
         unsafe {
             let f = Box::into_raw(Box::new(filter));
+            let f = f as *mut c_void;
             crocksdb_ffi::crocksdb_readoptions_set_table_filter(
                 self.inner,
-                mem::transmute(f),
+                f,
                 table_filter,
                 destroy_table_filter,
             );
@@ -1276,10 +1276,11 @@ impl ColumnFamilyOptions {
             name: CString::new(name.as_bytes()).unwrap(),
             merge_fn: merge_fn,
         });
+        let cb = Box::into_raw(cb) as *mut c_void;
 
         unsafe {
             let mo = crocksdb_ffi::crocksdb_mergeoperator_create(
-                mem::transmute(cb),
+                cb,
                 merge_operator::destructor_callback,
                 full_merge_callback,
                 partial_merge_callback,
@@ -1295,10 +1296,11 @@ impl ColumnFamilyOptions {
             name: CString::new(name.as_bytes()).unwrap(),
             f: compare_fn,
         });
+        let cb = Box::into_raw(cb) as *mut c_void;
 
         unsafe {
             let cmp = crocksdb_ffi::crocksdb_comparator_create(
-                mem::transmute(cb),
+                cb,
                 comparator::destructor_callback,
                 compare_callback,
                 comparator::name_callback,
@@ -1340,7 +1342,8 @@ impl ColumnFamilyOptions {
     pub fn set_max_bytes_for_level_multiplier(&mut self, mul: i32) {
         unsafe {
             crocksdb_ffi::crocksdb_options_set_max_bytes_for_level_multiplier(
-                self.inner, mul as f64,
+                self.inner,
+                f64::from(mul),
             );
         }
     }
@@ -1674,7 +1677,7 @@ impl CColumnFamilyDescriptor {
         CColumnFamilyDescriptor { inner }
     }
 
-    pub fn name<'a>(&'a self) -> &'a str {
+    pub fn name(&self) -> &str {
         unsafe {
             let raw_cf_name = crocksdb_ffi::crocksdb_name_from_column_family_descriptor(self.inner);
             CStr::from_ptr(raw_cf_name).to_str().unwrap()

src/table_filter.rs

@@ -13,7 +13,6 @@
 
 use crocksdb_ffi::DBTableProperties;
 use libc::{c_int, c_void};
-use std::mem;
 use table_properties::TableProperties;
 
 pub trait TableFilter {
@@ -28,7 +27,8 @@ pub trait TableFilter {
 pub extern "C" fn table_filter(ctx: *mut c_void, props: *const DBTableProperties) -> c_int {
     unsafe {
         let filter = &*(ctx as *mut Box<dyn TableFilter>);
-        filter.table_filter(mem::transmute(&*props)) as c_int
+        let props = &*(props as *const TableProperties);
+        filter.table_filter(props) as c_int
     }
 }
 

src/table_properties.rs

@@ -18,16 +18,16 @@ use crocksdb_ffi::{
 use libc::size_t;
 use std::marker::PhantomData;
 use std::ops::{Deref, Index};
-use std::{mem, slice, str};
+use std::{slice, str};
 
+#[repr(transparent)]
 pub struct TablePropertiesCollectionView(DBTablePropertiesCollection);
 
 impl TablePropertiesCollectionView {
     pub unsafe fn from_ptr<'a>(
         collection: *const DBTablePropertiesCollection,
     ) -> &'a TablePropertiesCollectionView {
-        let c = &*collection;
-        mem::transmute(c)
+        &*(collection as *const TablePropertiesCollectionView)
     }
 
     pub fn iter(&self) -> TablePropertiesCollectionIter {
@@ -130,14 +130,14 @@ impl Deref for TablePropertiesCollection {
     }
 }
 
+#[repr(transparent)]
 pub struct TableProperties {
     inner: DBTableProperties,
 }
 
 impl TableProperties {
     pub unsafe fn from_ptr<'a>(ptr: *const DBTableProperties) -> &'a TableProperties {
-        let res = &*ptr;
-        mem::transmute(res)
+        &*(ptr as *const TableProperties)
     }
 
     fn get_u64(&self, prop: DBTableProperty) -> u64 {
@@ -229,14 +229,14 @@ impl TableProperties {
     }
 }
 
+#[repr(transparent)]
 pub struct UserCollectedProperties {
     inner: DBUserCollectedProperties,
 }
 
 impl UserCollectedProperties {
     unsafe fn from_ptr<'a>(ptr: *const DBUserCollectedProperties) -> &'a UserCollectedProperties {
-        let prop = &*ptr;
-        mem::transmute(prop)
+        &*(ptr as *const UserCollectedProperties)
     }
 
     pub fn get<Q: AsRef<[u8]>>(&self, index: Q) -> Option<&[u8]> {

src/util.rs

-// Copyright 2018 PingCAP, Inc.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-pub fn is_power_of_two(mut n: usize) -> bool {
-    if n == 0 {
-        return false;
-    }
-    while n % 2 == 0 {
-        n = n / 2;
-    }
-    n == 1
-}
-
-#[cfg(test)]
-mod test {
-    use super::*;
-
-    #[test]
-    fn test_is_power_of_two() {
-        assert_eq!(is_power_of_two(0), false);
-        assert_eq!(is_power_of_two(1), true);
-        assert_eq!(is_power_of_two(2), true);
-        assert_eq!(is_power_of_two(4), true);
-        assert_eq!(is_power_of_two(8), true);
-        assert_eq!(is_power_of_two(5), false);
-    }
-}