From 8d5b1e4c6c6c6f06cfd2587af2061c2b80b4039e Mon Sep 17 00:00:00 2001 From: Tyler Neely Date: Sun, 12 Jul 2015 00:55:30 -0700 Subject: [PATCH] Fix a few memory leaks. Add valgrind feature for conditionally building a testable binary. Get rid of unstable vector usage. --- Cargo.toml | 4 ++++ README.md | 4 ++-- src/ffi.rs | 14 ++++++++------ src/lib.rs | 1 - src/main.rs | 32 +++++++++++++++++++++++++++++--- src/merge_operator.rs | 4 ++-- src/rocksdb.rs | 15 ++++++++++----- 7 files changed, 55 insertions(+), 19 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c2b4597..c7c119e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,3 +5,7 @@ description = "A Rust wrapper for Facebook's RocksDB embeddable database." version = "0.0.5" authors = ["Tyler Neely "] license = "Apache-2.0" + +[features] +default=[] +valgrind=[] diff --git a/README.md b/README.md index 75c4a3b..77a4ba3 100644 --- a/README.md +++ b/README.md @@ -45,9 +45,9 @@ use rocksdb::{RocksDBOptions, RocksDB, MergeOperands}; fn concat_merge(new_key: &[u8], existing_val: Option<&[u8]>, operands: &mut MergeOperands) -> Vec { let mut result: Vec = Vec::with_capacity(operands.size_hint().0); - existing_val.map(|v| { result.push_all(v) }); + existing_val.map(|v| { result.extend(v) }); for op in operands { - result.push_all(op); + result.extend(op); } result } diff --git a/src/ffi.rs b/src/ffi.rs index 1fc2dfe..4dfc518 100644 --- a/src/ffi.rs +++ b/src/ffi.rs @@ -180,12 +180,14 @@ extern { err: *mut i8 ) -> RocksDBInstance; pub fn rocksdb_writeoptions_create() -> RocksDBWriteOptions; + pub fn rocksdb_writeoptions_destroy(writeopts: RocksDBWriteOptions); pub fn rocksdb_put(db: RocksDBInstance, writeopts: RocksDBWriteOptions, k: *const u8, kLen: size_t, v: *const u8, vLen: size_t, err: *mut i8); pub fn rocksdb_readoptions_create() -> RocksDBReadOptions; + pub fn rocksdb_readoptions_destroy(readopts: RocksDBReadOptions); pub fn rocksdb_readoptions_set_snapshot(read_opts: RocksDBReadOptions, snapshot: RocksDBSnapshot); pub fn rocksdb_get(db: RocksDBInstance, @@ -338,22 +340,22 @@ fn internal() { assert!(err.is_null()); let writeopts = rocksdb_writeoptions_create(); - let RocksDBWriteOptions(write_opt_ptr) = writeopts; - assert!(!write_opt_ptr.is_null()); + assert!(!writeopts.0.is_null()); let key = b"name\x00"; let val = b"spacejam\x00"; - rocksdb_put(db, writeopts, key.as_ptr(), 4, val.as_ptr(), 8, err); + rocksdb_put(db, writeopts.clone(), key.as_ptr(), 4, val.as_ptr(), 8, err); + rocksdb_writeoptions_destroy(writeopts); assert!(err.is_null()); let readopts = rocksdb_readoptions_create(); - let RocksDBReadOptions(read_opts_ptr) = readopts; - assert!(!read_opts_ptr.is_null()); + assert!(!readopts.0.is_null()); let val_len: size_t = 0; let val_len_ptr = &val_len as *const size_t; - rocksdb_get(db, readopts, key.as_ptr(), 4, val_len_ptr, err); + rocksdb_get(db, readopts.clone(), key.as_ptr(), 4, val_len_ptr, err); + rocksdb_readoptions_destroy(readopts); assert!(err.is_null()); rocksdb_close(db); rocksdb_destroy_db(opts, cpath_ptr, err); diff --git a/src/lib.rs b/src/lib.rs index 2d2fe95..6510a7c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,6 @@ #![feature(unique)] #![feature(path_ext)] #![feature(raw)] -#![feature(vec_push_all)] pub use ffi as rocksdb_ffi; pub use ffi::{ diff --git a/src/main.rs b/src/main.rs index 7e18a46..2bcdc43 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14,13 +14,13 @@ limitations under the License. */ #![feature(test)] -#![feature(vec_push_all)] extern crate rocksdb; extern crate test; use rocksdb::{RocksDBOptions, RocksDB, MergeOperands, new_bloom_filter, Writable}; use rocksdb::RocksDBCompactionStyle::RocksDBUniversalCompaction; +#[cfg(not(feature = "valgrind"))] fn main() { let path = "/tmp/rust-rocksdb"; let db = RocksDB::open_default(path).unwrap(); @@ -46,11 +46,11 @@ fn concat_merge(new_key: &[u8], existing_val: Option<&[u8]>, mut operands: &mut MergeOperands) -> Vec { let mut result: Vec = Vec::with_capacity(operands.size_hint().0); match existing_val { - Some(v) => result.push_all(v), + Some(v) => result.extend(v), None => (), } for op in operands { - result.push_all(op); + result.extend(op); } result } @@ -82,6 +82,32 @@ fn custom_merge() { RocksDB::destroy(opts, path).is_ok(); } +#[cfg(feature = "valgrind")] +fn main() { + let path = "_rust_rocksdb_valgrind"; + let opts = RocksDBOptions::new(); + opts.create_if_missing(true); + opts.add_merge_operator("test operator", concat_merge); + let db = RocksDB::open(opts, path).unwrap(); + loop { + db.put(b"k1", b"a"); + db.merge(b"k1", b"b"); + db.merge(b"k1", b"c"); + db.merge(b"k1", b"d"); + db.merge(b"k1", b"efg"); + db.merge(b"k1", b"h"); + db.get(b"k1").map( |value| { + match value.to_utf8() { + Some(v) => (), + None => panic!("value corrupted"), + } + }) + .on_absent( || { panic!("value not found") }) + .on_error( |e| { panic!("error retrieving value: {}", e) }); + db.delete(b"k1"); + } +} + #[cfg(test)] mod tests { diff --git a/src/merge_operator.rs b/src/merge_operator.rs index f9f3741..19fc813 100644 --- a/src/merge_operator.rs +++ b/src/merge_operator.rs @@ -153,11 +153,11 @@ fn test_provided_merge(new_key: &[u8], existing_val: Option<&[u8]>, let nops = operands.size_hint().0; let mut result: Vec = Vec::with_capacity(nops); match existing_val { - Some(v) => result.push_all(v), + Some(v) => result.extend(v), None => (), } for op in operands { - result.push_all(op); + result.extend(op); } result } diff --git a/src/rocksdb.rs b/src/rocksdb.rs index c0e4c49..2e2d68d 100644 --- a/src/rocksdb.rs +++ b/src/rocksdb.rs @@ -132,7 +132,8 @@ impl RocksDB { unsafe { let writeopts = rocksdb_ffi::rocksdb_writeoptions_create(); let err = 0 as *mut i8; - rocksdb_ffi::rocksdb_write(self.inner, writeopts, batch.inner, err); + rocksdb_ffi::rocksdb_write(self.inner, writeopts.clone(), batch.inner, err); + rocksdb_ffi::rocksdb_writeoptions_destroy(writeopts); if !err.is_null() { return Err(error_message(err)); } @@ -153,8 +154,9 @@ impl RocksDB { let val_len: size_t = 0; let val_len_ptr = &val_len as *const size_t; let err = 0 as *mut i8; - let val = rocksdb_ffi::rocksdb_get(self.inner, readopts, + let val = rocksdb_ffi::rocksdb_get(self.inner, readopts.clone(), key.as_ptr(), key.len() as size_t, val_len_ptr, err) as *mut u8; + rocksdb_ffi::rocksdb_readoptions_destroy(readopts); if !err.is_null() { return RocksDBResult::Error(error_message(err)); } @@ -177,9 +179,10 @@ impl Writable for RocksDB { unsafe { let writeopts = rocksdb_ffi::rocksdb_writeoptions_create(); let err = 0 as *mut i8; - rocksdb_ffi::rocksdb_put(self.inner, writeopts, key.as_ptr(), + rocksdb_ffi::rocksdb_put(self.inner, writeopts.clone(), key.as_ptr(), key.len() as size_t, value.as_ptr(), value.len() as size_t, err); + rocksdb_ffi::rocksdb_writeoptions_destroy(writeopts); if !err.is_null() { return Err(error_message(err)); } @@ -191,9 +194,10 @@ impl Writable for RocksDB { unsafe { let writeopts = rocksdb_ffi::rocksdb_writeoptions_create(); let err = 0 as *mut i8; - rocksdb_ffi::rocksdb_merge(self.inner, writeopts, key.as_ptr(), + rocksdb_ffi::rocksdb_merge(self.inner, writeopts.clone(), key.as_ptr(), key.len() as size_t, value.as_ptr(), value.len() as size_t, err); + rocksdb_ffi::rocksdb_writeoptions_destroy(writeopts); if !err.is_null() { return Err(error_message(err)); } @@ -205,8 +209,9 @@ impl Writable for RocksDB { unsafe { let writeopts = rocksdb_ffi::rocksdb_writeoptions_create(); let err = 0 as *mut i8; - rocksdb_ffi::rocksdb_delete(self.inner, writeopts, key.as_ptr(), + rocksdb_ffi::rocksdb_delete(self.inner, writeopts.clone(), key.as_ptr(), key.len() as size_t, err); + rocksdb_ffi::rocksdb_writeoptions_destroy(writeopts); if !err.is_null() { return Err(error_message(err)); }