is_hdd update for windows and linux #4278

moneromooo-monero · 2018-08-18T09:43:48Z

Add is_hdd_sysfs(), is_hdd_win_ioctl(), fl2lv(), lv2pv(),
win_device_has_seek_penalny()
Add tests test_a.is_hdd*
Modify is_hdd()
Added bcrypt, sodium

Contributed by crCr62U0_ who does not have nor want a github account.

Add is_hdd_sysfs(), is_hdd_win_ioctl(), fl2lv(), lv2pv(), win_device_has_seek_penalny() Add tests test_a.is_hdd* Modify is_hdd() Added bcrypt, sodium

moneromooo-monero

Please make commit message first line informative (like this PR's title for example).

Also, since you change the is_hdd code completely, I think it might be a good occasion to move this into a separate file as it's becoming a bit crowded here.

moneromooo-monero · 2018-08-18T09:45:01Z

src/common/util.cpp

@@ -74,6 +75,11 @@ using namespace epee;
 #include <boost/asio.hpp>
 #include <openssl/sha.h>

+#ifdef WIN32


I think we had problems with WIN32 before, and _WIN32 was the "best" one to use.

moneromooo-monero · 2018-08-18T09:46:31Z

src/common/util.cpp

+        return false;
+      }
+    }
+    uint16_t val = 0xdead;


Probably best to make it a unsigned short if using %hu below.

moneromooo-monero · 2018-08-18T09:48:42Z

src/common/util.cpp

-    std::string sys_path = "/sys/block/" + device + "/queue/rotational";
-    FILE *f = fopen(sys_path.c_str(), "r");
-    if (!f)
+  bool win_device_has_seek_penalny(const char *pv, bool &result)


moneromooo-monero · 2018-08-18T09:49:05Z

src/common/util.cpp

+        );
+      if(success and r_size == sizeof(r))
+      {
+        result = r.IncursSeekPenalty;


That path seems to leak h.

moneromooo-monero · 2018-08-18T09:49:46Z

src/common/util.cpp

+      return false;
+    }
+    bool a_result = 0;
+    bool a_success = 0;


moneromooo-monero · 2018-08-18T09:50:19Z

src/common/util.cpp

+    #elif defined(__GLIBC__)
+    return is_hdd_sysfs(path, result);
+    #else
+    return 0;


moneromooo-monero · 2018-08-18T09:50:35Z

src/common/util.cpp

+    }
+    else
+    {
+        return 0;


moneromooo-monero · 2018-08-18T09:50:53Z

tests/unit_tests/CMakeLists.txt

@@ -72,6 +72,7 @@ set(unit_tests_sources
  ringct.cpp
  output_selection.cpp
  vercmp.cpp
+  test_a.cpp


Rename to something sane

vtnerd

A first pass at a review.

vtnerd · 2018-08-19T17:34:45Z

src/common/util.cpp

@@ -733,63 +739,225 @@ std::string get_nix_version_display_string()
 #endif
  }

-  bool is_hdd(const char *path)
+#if defined(__GLIBC__)
+  bool is_hdd_sysfs(const char *file_path, bool &result)


Somewhat quirky, but boost::optional<bool> might be better suited for this. There is also boost::tribool, but usage of that can be tricky since the library author unfortunately decided to overload operator&& and operator|| which ruins default semantics.

vtnerd · 2018-08-19T17:37:34Z

src/common/util.cpp

+      return false;
+    }
+    std::string attr_path = prefix + "/queue/rotational";
+    FILE *f = fopen(attr_path.c_str(), "r");


Why not std::ifstream here? Streams were already in use a few lines above anyway. The primary advantage is that you don't have to worry about the unsigned short / %hu stuff in scanf.

vtnerd · 2018-08-19T17:44:10Z

src/common/util.cpp

+    if(h != INVALID_HANDLE_VALUE)
+    {
+      char p[MAX_PATH + 1];
+      DWORD r_size = GetFinalPathNameByHandleA(


If this throws (it shouldn't really), then CloseHandle is never called. There is a struct close_handle in this cpp file already that can be used as a helper:

std::unique_ptr<void, close_handle> h{ CreateFile(...) };

vtnerd · 2018-08-19T17:47:40Z

src/crypto/CMakeLists.txt

@@ -79,6 +79,7 @@ target_link_libraries(cncrypto
    epee
    ${Boost_SYSTEM_LIBRARY}
  PRIVATE
+    sodium


Is this meant for another patch?

vtnerd · 2018-08-19T18:47:04Z

src/common/util.cpp

-        std::string dev_path = std::string("/dev/block/") + de->d_name;
-        char resolved[PATH_MAX];
-        if (realpath(dev_path.c_str(), resolved) && !strncmp(resolved, "/dev/", 5))
+        std::regex r("^\\\\Device\\\\([^\\\\]+).*$");


Ok, let me walk through this logic, because this should be easy for a boost::spirit usage. It will require more compile-time, but will be much faster since the expression expands into a specific parser at compile-time, instead of a generic NFA runtime implementation that std::regex is likely using.

It looks like you want to grab all characters after \\Device\\ until EOF or next \\. Is that correct? If so the boost::spirit equivalent is:

#include <boost/range/iterator_range.hpp> #include <boost/spirit/include/qi_char_.hpp> #include <boost/spirit/include/qi_difference.hpp> #include <boost/spirit/include/qi_kleene.hpp> #include <boost/spirit/include/qi_lit.hpp> #include <boost/spirit/include/qi_sequence.hpp> namespace qi = boost::spirit::qi boost::iterator_range<const char> match{}; if (qi::parse(p, p + strlen(p), (qi::lit("\\Device\\") >> *(qi::char_ - "\\")), match)) { ... }

This simply says match \\Device\\ then match zero or more char_acters unless the next two characters are \\. Those char_acters are then stored in the boost::iterator_range<const char*>. The entire expression doesn't allocate any memory, even when a match is found.

... thoughts ?

vtnerd · 2018-08-19T21:47:22Z

src/common/util.cpp

+    HANDLE h = INVALID_HANDLE_VALUE;
+    std::string lv_path = "\\\\?\\";
+    lv_path += lv;
+    h = CreateFile(


Another std::unique_ptr<void, close_handle> case.

vtnerd · 2018-08-19T21:49:30Z

src/common/util.cpp

+      {
+        for(uint32_t i = 0; i < r.NumberOfDiskExtents; ++i)
+        {
+          std::ostringstream ss;


Pop this before the loop, then do a .str(std::string{}) every iteration. Some easy performance gains (constructing an std::ostream requires a global synchronization on the current std::locale object). There might be a better way to reset the state of this type if the manual was inspecting further ...

vtnerd · 2018-08-19T22:00:48Z

src/common/util.cpp

+    {
+        bool q_result;
+        bool q_success = win_device_has_seek_penalny(v.c_str(), q_result);
+        a_success |= q_success;


This is damn near obfuscation. Some early returns are possible. If boost::optional<bool> suggestion is used:

for (auto const& v : pvs) { const auto success = win_device_has_seek_penalty(v.c_str()); if (!success) return boost::none; if (success.value()) return true; } return false;

Should this be inverted though? If any of the backing physical volumes isn't a local HDD, then LMDB may have a bad time.

vtnerd · 2018-08-19T22:07:28Z

src/common/util.cpp

+    HANDLE h = INVALID_HANDLE_VALUE;
+    std::string pv_path = "\\\\?\\";
+    pv_path += pv;
+    h = CreateFile(


Another std::unique_ptr<void, close_handle>.

vtnerd · 2018-08-19T22:46:35Z

src/common/util.cpp

+    }
+    bool a_result = 0;
+    bool a_success = 0;
+    for(auto &v: pvs)


auto const& v

p8p · 2018-08-20T05:56:11Z

I don't have write access to push updates.

moneromooo-monero · 2018-08-20T08:55:59Z

If you are the author, then you can open a new bug for it. If you are not, then I suggest you ping the author in #monero-dev (nick in the first post) about it. I PRed because the author did not want to make a github account.

p8p · 2018-08-20T12:08:46Z

The final aim is to upload the next commit with all fixes requested in comments.
It was possible to ask moneromoo-monero to create pull request with the first commit.
What way should be used for the next commit?
It seems that p8p account (newly created github) has been flagged therefore I'm forbidden to use all github features.
I'd like to send updates without using github account because of this useless censorship.
The way to use someone in #monero-dev as a proxy to upload updates is too slow and not efficient.

1. Move is_hdd() declaration from util.h into is_hdd.h include "common/is_hdd.h" into *.cpp that use is_hdd() 2. Move the following functions from util.cpp into is_hdd.cpp is_hdd_sysfs() fp2lv() lv2pv() win_device_has_seek_penalty() is_hdd_win_ioctl() is_hdd() 3. Fix the following things use _WIN32 instead of WIN32 use unsigned short instead of uint16_t use penalty instead of penalny add CloseHandle(h) in all conditional branches to avoid resource leaking use either 0/1 or true/false to maintain consistency rename tests/unit_tests/test_a.cpp

moneromooo-monero · 2018-08-22T13:59:44Z

+duplicate

Update

c5a5c9c

Add is_hdd_sysfs(), is_hdd_win_ioctl(), fl2lv(), lv2pv(), win_device_has_seek_penalny() Add tests test_a.is_hdd* Modify is_hdd() Added bcrypt, sodium

moneromooo-monero commented Aug 18, 2018

View reviewed changes

vtnerd reviewed Aug 19, 2018

View reviewed changes

p8p mentioned this pull request Aug 22, 2018

is_hdd: update for linux only #4293

Merged

moneromooo-monero closed this Aug 22, 2018

issue-helper added the duplicate label Aug 22, 2018

stoffu mentioned this pull request Jun 18, 2019

Various wallet improvements (nuke TransferOriginal, unlock_time, etc) aeonix/aeon#116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

is_hdd update for windows and linux #4278

is_hdd update for windows and linux #4278

moneromooo-monero commented Aug 18, 2018

moneromooo-monero left a comment

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

moneromooo-monero Aug 18, 2018

vtnerd left a comment

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

vtnerd Aug 19, 2018

p8p commented Aug 20, 2018

moneromooo-monero commented Aug 20, 2018

p8p commented Aug 20, 2018

moneromooo-monero commented Aug 22, 2018

is_hdd update for windows and linux #4278

is_hdd update for windows and linux #4278

Conversation

moneromooo-monero commented Aug 18, 2018

moneromooo-monero left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd left a 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

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

p8p commented Aug 20, 2018

moneromooo-monero commented Aug 20, 2018

p8p commented Aug 20, 2018

moneromooo-monero commented Aug 22, 2018