From 85ac1a320a7082391e26e76792e968a5f48bcca0 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Tue, 3 Jan 2017 18:17:12 -0800 Subject: [PATCH] Fix rocksdb::Status::getState Summary: This fixes the Java API for Status#getState use in Native code and also simplifies the implementation of rocksdb::Status::getState. Closes https://github.com/facebook/rocksdb/issues/1688 Closes https://github.com/facebook/rocksdb/pull/1714 Differential Revision: D4364181 Pulled By: yiwu-arbug fbshipit-source-id: 8e073b4 --- db/db_test.cc | 13 ++++++++ include/rocksdb/status.h | 1 + java/rocksjni/portal.h | 4 +-- .../test/java/org/rocksdb/RocksDBTest.java | 16 ++++++++++ util/status.cc | 32 +++++++++---------- 5 files changed, 47 insertions(+), 19 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index f01b508ae..d033a0a2e 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -197,6 +197,19 @@ TEST_F(DBTest, MemEnvTest) { } #endif // ROCKSDB_LITE +TEST_F(DBTest, OpenWhenOpen) { + Options options = CurrentOptions(); + options.env = env_; + rocksdb::DB* db2 = nullptr; + rocksdb::Status s = DB::Open(options, dbname_, &db2); + + ASSERT_EQ(Status::Code::kIOError, s.code()); + ASSERT_EQ(Status::SubCode::kNone, s.subcode()); + ASSERT_TRUE(strncmp("lock ", s.getState(), 5) == 0); + + delete db2; +} + TEST_F(DBTest, WriteEmptyBatch) { Options options = CurrentOptions(); options.env = env_; diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h index 2c5090b4f..078d5c1b4 100644 --- a/include/rocksdb/status.h +++ b/include/rocksdb/status.h @@ -75,6 +75,7 @@ class Status { SubCode subcode() const { return subcode_; } + // Returns a C style string indicating the message of the Status const char* getState() const { return state_; } // Return a success status. diff --git a/java/rocksjni/portal.h b/java/rocksjni/portal.h index dc6dcf507..d4408f497 100644 --- a/java/rocksjni/portal.h +++ b/java/rocksjni/portal.h @@ -120,8 +120,8 @@ class StatusJni : public RocksDBNativeClass { jstring jstate = nullptr; if (status.getState() != nullptr) { - std::string s(status.getState()); - jstate = env->NewStringUTF(s.c_str()); + const char* const state = status.getState(); + jstate = env->NewStringUTF(state); } return env->NewObject(getJClass(env), mid, toJavaStatusCode(status.code()), toJavaStatusSubCode(status.subcode()), jstate); diff --git a/java/src/test/java/org/rocksdb/RocksDBTest.java b/java/src/test/java/org/rocksdb/RocksDBTest.java index 0f32c6cbd..7ae0da6e2 100644 --- a/java/src/test/java/org/rocksdb/RocksDBTest.java +++ b/java/src/test/java/org/rocksdb/RocksDBTest.java @@ -12,6 +12,7 @@ import org.junit.rules.TemporaryFolder; import java.util.*; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.fail; public class RocksDBTest { @@ -42,6 +43,21 @@ public class RocksDBTest { } } + @Test + public void openWhenOpen() throws RocksDBException { + final String dbPath = dbFolder.getRoot().getAbsolutePath(); + + try (final RocksDB db1 = RocksDB.open(dbPath)) { + try (final RocksDB db2 = RocksDB.open(dbPath)) { + fail("Should have thrown an exception when opening the same db twice"); + } catch (final RocksDBException e) { + assertThat(e.getStatus().getCode()).isEqualTo(Status.Code.IOError); + assertThat(e.getStatus().getSubCode()).isEqualTo(Status.SubCode.None); + assertThat(e.getStatus().getState()).startsWith("lock "); + } + } + } + @Test public void put() throws RocksDBException { try (final RocksDB db = RocksDB.open(dbFolder.getRoot().getAbsolutePath()); diff --git a/util/status.cc b/util/status.cc index c17006ea7..9c1ad8a08 100644 --- a/util/status.cc +++ b/util/status.cc @@ -7,17 +7,17 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. +#include "rocksdb/status.h" #include +#include #include "port/port.h" -#include "rocksdb/status.h" namespace rocksdb { const char* Status::CopyState(const char* state) { - uint32_t size; - memcpy(&size, state, sizeof(size)); - char* result = new char[size + 4]; - memcpy(result, state, size + 4); + char* const result = + new char[std::strlen(state) + 1]; // +1 for the null terminator + std::strcpy(result, state); return result; } @@ -25,17 +25,17 @@ Status::Status(Code _code, SubCode _subcode, const Slice& msg, const Slice& msg2 : code_(_code), subcode_(_subcode) { assert(code_ != kOk); assert(subcode_ != kMaxSubCode); - const uint32_t len1 = static_cast(msg.size()); - const uint32_t len2 = static_cast(msg2.size()); - const uint32_t size = len1 + (len2 ? (2 + len2) : 0); - char* result = new char[size + 4]; - memcpy(result, &size, sizeof(size)); - memcpy(result + 4, msg.data(), len1); + const size_t len1 = msg.size(); + const size_t len2 = msg2.size(); + const size_t size = len1 + (len2 ? (2 + len2) : 0); + char* const result = new char[size + 1]; // +1 for null terminator + memcpy(result, msg.data(), len1); if (len2) { - result[4 + len1] = ':'; - result[5 + len1] = ' '; - memcpy(result + 6 + len1, msg2.data(), len2); + result[len1] = ':'; + result[len1 + 1] = ' '; + memcpy(result + len1 + 2, msg2.data(), len2); } + result[size] = '\0'; // null terminator for C style string state_ = result; } @@ -98,9 +98,7 @@ std::string Status::ToString() const { } if (state_ != nullptr) { - uint32_t length; - memcpy(&length, state_, sizeof(length)); - result.append(state_ + 4, length); + result.append(state_); } return result; }