is_hdd: update for linux only #4293

p8p · 2018-08-22T13:42:07Z

First two commits have been discussed in #4278

p8p · 2018-08-22T13:54:52Z

tests/unit_tests/is_hdd.cpp

+  path = "C:\\";
+  EXPECT_TRUE(tools::is_hdd(path.c_str()));
+}
+TEST(is_hdd, win_os_ssd)


I don't know how to emulate ssd.
This test isn't general and requires environment setup.
It'll be deleted.

p8p · 2018-08-22T13:57:26Z

src/crypto/CMakeLists.txt

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


This is required for successful build of master branch.
It's not required for is_hdd() implementation itself.

p8p · 2018-08-22T14:02:40Z

CMakeLists.txt

@@ -566,11 +568,13 @@ else()
  add_c_flag_if_supported(-Wformat-security C_SECURITY_FLAGS)
  add_cxx_flag_if_supported(-Wformat-security CXX_SECURITY_FLAGS)

+  if(0)


This is required for successful build of master branch.
It's not required for is_hdd() implementation itself.
It's mentioned here #4290

p8p · 2018-08-22T14:05:14Z

tests/unit_tests/ringdb.cpp

@@ -47,7 +47,7 @@ static crypto::chacha_key generate_chacha_key()
 {
  crypto::chacha_key chacha_key;
  uint64_t password = crypto::rand<uint64_t>();
-  crypto::generate_chacha_key(std::string((const char*)&password, sizeof(password)), chacha_key);
+  crypto::generate_chacha_key(std::string((const char*)&password, sizeof(password)), chacha_key, 1);


This is required for successful build of tests of master branch.
It's not required for is_hdd() implementation itself.
It's mentioned here #4272

p8p · 2018-08-22T19:57:44Z

CMakeLists.txt

@@ -814,7 +818,7 @@ endif()
 include_directories(SYSTEM ${Boost_INCLUDE_DIRS})
 if(MINGW)
  set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -Wa,-mbig-obj")
-  set(EXTRA_LIBRARIES mswsock;ws2_32;iphlpapi)
+  set(EXTRA_LIBRARIES mswsock;ws2_32;iphlpapi;bcrypt)


This is required for successful build of tests of master branch.
It's not required for is_hdd() implementation itself.

Changed the default random_generator implementation to use
boost::uuid::random_generator is being used only in "tests/functional_tests/transactions_flow_test.cpp" but included in few other places.

moneromooo-monero · 2018-08-25T19:41:38Z

Why change this ? Is it somehow better ?

moneromooo-monero · 2018-08-25T20:13:07Z

Welll, the code's substantially smaller anyway, and it works for me, so I guess that's a good enough reason. Why did you take out the Windows part ?

p8p · 2018-08-26T16:05:59Z

@moneromooo-monero,

STORAGE_PROPERTY_QUERY q{
  .PropertyId = StorageDeviceSeekPenaltyProperty,
  .QueryType = PropertyStandardQuery
};
DEVICE_SEEK_PENALTY_DESCRIPTOR r;
DWORD r_size = 0;
BOOL success = DeviceIoControl(
  h,
  IOCTL_STORAGE_QUERY_PROPERTY,
  &q,
  sizeof(q),
  &r,
  sizeof(r),
  &r_size,
  nullptr
);

Windows part works on windows 7 installed on virtualbox emulator.
But it fails with GetLastError() == 31 on windows 10 and windows 8.1 installed on virtualbox emulator.
I don't have any other implementation for windows.

moneromooo-monero · 2018-08-27T08:51:09Z

Isn't this better than nothing then ?
Or is window 7 really old ?

p8p · 2018-08-27T11:38:30Z

I don't want submit that implementation.
Windows 7 is still supported.

Products Released	Lifecycle Start Date	Mainstream Support End Date	Extended Support End Date	Service Pack Support End Date	Notes
Windows 7 Service Pack 1	2/22/2011	1/13/2015	1/14/2020	--	--

https://support.microsoft.com/en-us/lifecycle/search/?c2=14019

vtnerd · 2018-08-22T17:15:22Z

src/common/is_hdd.cpp

+    }
+    std::string attr_path = prefix + "/queue/rotational";
+    std::ifstream f(attr_path, std::ios_base::in);
+    if(not f.is_open())


Why did you start flipping everything to the alternative tokens? I guess a new cpp file means you can slightly dictact new style policies, but this is certainly an interesting choice ... it doesn't bother me all that much (it is still readable) -> @moneromooo-monero what do you think?

It feels odd, but since it's now standard and not confusing, I have no problem with it.

vtnerd · 2018-08-22T17:19:51Z

src/common/is_hdd.cpp

+      bool success = qi::parse(
+        p.cbegin(),
+        p.cend(),
+        (qi::lit("\\Device\\") >> qi::raw[*(qi::char_ - "\\")]),


Good catch on the missing qi::raw calling from my last review.

I don't see that code in the patch now, and don't recall seeing it before. Is this commenting on an old rev ?

Yes, I meant to only click the approve button and didn't realize I had unpublished comments.

vtnerd · 2018-08-22T17:27:25Z

src/common/is_hdd.cpp

+      }
+      if(q.value())
+      {
+        return true;


I had a comment about whether this is correct behavior in my last review. In particular, if a path is backed by a RAID-0 setup, you want every check to pass. This would be "correct" for RAID-1 or higher, but I think the conservative approach is better since this is merely a warning system and not a forcibly-do-this check.

vtnerd · 2018-08-22T17:40:12Z

src/common/is_hdd.cpp

+    std::ifstream f(attr_path, std::ios_base::in);
+    if(not f.is_open())
+    {
+      attr_path = prefix + "/../queue/rotational";


Is this for older kernels? This would be looking for a file at path /sys/dev/block/queue/rotational. I see no such file on my local systems.

vtnerd · 2018-08-22T17:42:51Z

src/common/is_hdd.cpp

+#include <boost/optional.hpp>
+namespace tools
+{
+#if defined(__GLIBC__)


This /sys check seems specific to Linux, and not Posix or GNU. Is it possible to have glibc (GNU) outside of Linux? I feel like it is possible, so a check for a Linux macro might be more appropriate.

vtnerd · 2018-09-03T17:21:17Z

Sorry, I had unpublished comments apparently. You can just ignore them now ...

9d65399 is_hdd update (p8p)

p8p commented Aug 22, 2018

View reviewed changes

p8p force-pushed the is_hdd branch from 0bf3da1 to c9d369f Compare August 22, 2018 14:00

p8p commented Aug 22, 2018

View reviewed changes

p8p force-pushed the is_hdd branch from c9d369f to 7598208 Compare August 22, 2018 16:51

p8p commented Aug 22, 2018

View reviewed changes

p8p force-pushed the is_hdd branch from 7598208 to ccfb81a Compare August 23, 2018 08:10

is_hdd update

9d65399

p8p force-pushed the is_hdd branch from ccfb81a to 9d65399 Compare August 25, 2018 11:55

p8p changed the title ~~is_hdd: update for linux and windows~~ is_hdd: update for linux only Aug 25, 2018

p8p closed this Aug 25, 2018

p8p reopened this Aug 25, 2018

moneromooo-monero approved these changes Aug 27, 2018

View reviewed changes

vtnerd approved these changes Sep 3, 2018

View reviewed changes

luigi1111 merged commit 9d65399 into monero-project:master Sep 10, 2018

luigi1111 added a commit that referenced this pull request Sep 10, 2018

Merge pull request #4293

ab85b92

9d65399 is_hdd update (p8p)

p8p deleted the is_hdd branch September 11, 2018 05:56

stoffu mentioned this pull request Dec 23, 2019

Upstream merge 5 aeonix/aeon#167

Merged

stoffu mentioned this pull request Apr 23, 2020

Aeon upstream 6 aeonix/aeon#175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

is_hdd: update for linux only #4293

is_hdd: update for linux only #4293

p8p commented Aug 22, 2018

p8p Aug 22, 2018

p8p Aug 22, 2018

p8p Aug 22, 2018

p8p Aug 22, 2018 •

edited

Loading

p8p Aug 22, 2018

moneromooo-monero commented Aug 25, 2018

moneromooo-monero commented Aug 25, 2018

p8p commented Aug 26, 2018

moneromooo-monero commented Aug 27, 2018 •

edited

Loading

p8p commented Aug 27, 2018

vtnerd Aug 22, 2018

moneromooo-monero Sep 3, 2018

vtnerd Aug 22, 2018

moneromooo-monero Sep 3, 2018

vtnerd Sep 7, 2018

vtnerd Aug 22, 2018

vtnerd Aug 22, 2018

vtnerd Aug 22, 2018

vtnerd commented Sep 3, 2018

is_hdd: update for linux only #4293

is_hdd: update for linux only #4293

Conversation

p8p commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p8p Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Aug 25, 2018

moneromooo-monero commented Aug 25, 2018

p8p commented Aug 26, 2018

moneromooo-monero commented Aug 27, 2018 • edited Loading

p8p commented Aug 27, 2018

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 commented Sep 3, 2018

p8p Aug 22, 2018 •

edited

Loading

moneromooo-monero commented Aug 27, 2018 •

edited

Loading