search

ManageIQ / more_core_extensions

MoreCoreExtensions are a set of core extensions beyond those provided by ActiveSupport.
MIT License
5 stars 23 forks source link

Add Windows time methods that convert to and from FILETIME #93

Closed djberg96 closed 4 years ago

djberg96 commented 4 years ago

This PR adds two custom methods to the Time class: wtime_to_time and the inverse time_to_wtime. These convert Windows FILETIME values into a Ruby time value.

The original idea was to port this straight from the NtUtil module in gems pending: https://github.com/ManageIQ/manageiq-gems-pending/blob/master/lib/gems/pending/util/win32/nt_util.rb as part of the cleanup effort at https://github.com/ManageIQ/manageiq-gems-pending/issues/231.

However, closer inspection appears to reveal a problem with the NtUtil code. Namely, the value 11_644_495_200 doesn't line up with what we're finding online. We are not sure where that number came from, but the original commit message suggests that it "may be slightly off". So, I've changed it.

This code essentially matches the code currently in the win32-registry library from the stdlib: https://github.com/ruby/ruby/blob/c5eb24349a4535948514fe765c3ddb0628d81004/ext/win32/lib/win32/registry.rb#L402-L404, which in turn matches up with what we do find online.

As to why we don't just use win32-registry directly, it's because it's not available on non-windows systems since it also makes dllload calls that wouldn't work except on Windows.

For info on FILETIME: https://support.microsoft.com/en-us/help/188768/info-working-with-the-filetime-structure

miq-bot commented 4 years ago

Checked commits https://github.com/djberg96/more_core_extensions/compare/796290c55a56ba8d4169d07dc3e344b60b1cdd58~...017a1f35f45647600bae61c45466c474161fa601 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint 4 files checked, 0 offenses detected Everything looks fine. :star:

chessbyte commented 4 years ago

should we rename these methods to make things clearer? maybe something verbose like windows_filetime_to_ruby_time and ruby_time_to_windows_filetime

djberg96 commented 4 years ago

@chessbyte I'm generally not a fan of "ruby" in method names. We know it's Ruby. :)

How about windows_filetime_to_time and time_to_windows_filetime?

Fryguy commented 4 years ago

However, closer inspection appears to reveal a problem with the NtUtil code. Namely, the value 11_644_495_200 doesn't line up with what we're finding online. We are not sure where that number came from, but the original commit message suggests that it "may be slightly off". So, I've changed it.

I would feel much more comfortable with a change like this if we could verify through smartstate running against a real system (and different versions of Windows systems at that).

As to why we don't just use win32-registry directly

Because smartstate doesn't actually run on a live filesystem. It's interpreting the raw bytes of the registry directly, hence the need to have a way to convert the raw time values. That being said, I'd be ok with using the helper methods in win32-registry.

FWIW, I searched back and it seems that code has been in place since 2007

djberg96 commented 4 years ago

Because smartstate doesn't actually run on a live filesystem. It's interpreting the raw bytes of the registry directly, hence the need to have a way to convert the raw time values. That being said, I'd be ok with using the helper methods in win32-registry.

As far as I can tell you cannot require 'win32/registry' on a non-windows system. It loads on my Windows 7 VM, but fails to load on my Mac.

FWIW, I searched back and it seems that code has been in place since 2007.

I'm afraid it's probably been wrong since then.

Fryguy commented 4 years ago

I'm afraid it's probably been wrong since then.

If that's the case, I'd much rather see it fixed as a proper bug fix on it's own in gems pending (which is, in turn, backportable), than changed in the same PR as an extraction.

kbrock commented 4 years ago

Can you verify that this works with ActiveSupport::TimeWithTimezone

We may need to do nothing, or we may need to add another include clause

maybe include it if the class is defined?

guess load order may be an issue as we would want this loaded after rails.

djberg96 commented 4 years ago

Well, it would definitely break the WimParser specs, but whether or not the specs are correct I'm not sure.

djberg96 commented 4 years ago

Closing for now, as this might not be compatible with what's in gems-pending atm.