Added encoding for paths #7

grbIzl · 2017-06-15T15:36:42Z

Encoding added. For Windows path local codepage will be used. For posix it will be usual utf-8 string
Test with non unicode path added

rphmeier · 2017-06-15T15:47:28Z

Cargo.toml

@@ -26,3 +26,4 @@ path = "test/test.rs"
 [dependencies]
 libc = "0.2"
 rocksdb-sys = { path = "rocksdb-sys", version = "0.3.0" }
+local-encoding = "*"


should pin to specific version

rphmeier · 2017-06-15T15:47:45Z

src/lib.rs

@@ -13,6 +13,7 @@
 // limitations under the License.
 //
 extern crate rocksdb_sys;
+extern crate local_encoding;


style: newline between links and pub exports

rphmeier · 2017-06-15T15:48:32Z

src/rocksdb.rs

@@ -276,7 +278,8 @@ impl DB {
    	if cfs.len() != cf_opts.len() {
 			return Err(format!("Mismatching number of CF options"));
 		}
-        let cpath = match CString::new(path.as_bytes()) {
+        let encoded_path = Encoding::ANSI.to_bytes(path);
+        let cpath = match CString::new(encoded_path.unwrap()) {


unwrap() is an assertion. Favor expect which lets you provide a proof for why the assertion will never fail

(our goal is zero panics in Parity code, so all panickers should be backed by a "proof" of why they won't fail)

rphmeier · 2017-06-15T15:49:04Z

src/rocksdb.rs

@@ -378,7 +381,9 @@ impl DB {
    }

    pub fn destroy(opts: &Options, path: &str) -> Result<(), String> {
-        let cpath = CString::new(path.as_bytes()).unwrap();
+        let encoded_path = Encoding::ANSI.to_bytes(path);
+        let cpath = CString::new(encoded_path.unwrap()).unwrap();


rphmeier · 2017-06-15T15:49:09Z

src/rocksdb.rs

@@ -395,7 +400,9 @@ impl DB {
    }

    pub fn repair(opts: &Options, path: &str) -> Result<(), String> {
-        let cpath = CString::new(path.as_bytes()).unwrap();
+        let encoded_path = Encoding::ANSI.to_bytes(path);
+        let cpath = CString::new(encoded_path.unwrap()).unwrap();


rphmeier · 2017-06-15T15:49:15Z

src/rocksdb.rs

@@ -518,7 +525,8 @@ impl DB {
                     name: &str,
                     opts: &Options)
                     -> Result<Column, String> {
-        let cname = match CString::new(name.as_bytes()) {
+        let encoded_name = Encoding::ANSI.to_bytes(name);
+        let cname = match CString::new(encoded_name.unwrap()) {


grbIzl · 2017-06-16T11:21:49Z

Review comments fixed

rphmeier · 2017-06-16T12:16:53Z

src/rocksdb.rs

@@ -276,7 +278,16 @@ impl DB {
    	if cfs.len() != cf_opts.len() {
 			return Err(format!("Mismatching number of CF options"));
 		}
-        let cpath = match CString::new(path.as_bytes()) {
+        let encoded_path = match Encoding::ANSI.to_bytes(path) {


idiomatic rust tip: you can use map_err and the ? operator to condense this to
let encoded_path = Encoding::ANSI.to_bytes(path).map_err(|_| "error msg".to_string())?

rphmeier · 2017-06-16T12:17:49Z

LGTM, thanks

Added encoding for paths

71a8b2f

grbIzl mentioned this pull request Jun 15, 2017

App crashes using non-ascii path on windows openethereum/parity-ethereum#2215

Closed

rphmeier reviewed Jun 15, 2017

View reviewed changes

Removed unwrap. Better handling of resut added

aecb6a7

rphmeier reviewed Jun 16, 2017

View reviewed changes

arkpar merged commit 4364cae into master Jun 22, 2017

arkpar deleted the Fix-2215 branch June 22, 2017 12:35

grbIzl mentioned this pull request Jun 22, 2017

Latest changes from Rust RocksDB binding merged openethereum/parity-ethereum#5905

Merged

apolkingg8 mentioned this pull request Feb 27, 2018

"IO error: Failed to create dir: Invalid argument" when having non-latin characters in folder name (only on windows) Level/rocksdb#40

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added encoding for paths #7

Added encoding for paths #7

grbIzl commented Jun 15, 2017

rphmeier Jun 15, 2017

rphmeier Jun 15, 2017

rphmeier Jun 15, 2017

rphmeier Jun 15, 2017

rphmeier Jun 15, 2017

rphmeier Jun 15, 2017

rphmeier Jun 15, 2017

grbIzl commented Jun 16, 2017

rphmeier Jun 16, 2017

rphmeier commented Jun 16, 2017

Added encoding for paths #7

Added encoding for paths #7

Conversation

grbIzl commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grbIzl commented Jun 16, 2017

Choose a reason for hiding this comment

rphmeier commented Jun 16, 2017