Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

djberg96
Copy link
Contributor

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 ManageIQ/manageiq-gems-pending#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
Copy link
Member

miq-bot commented Aug 13, 2020

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. ⭐

@chessbyte
Copy link
Member

chessbyte commented Aug 13, 2020

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
Copy link
Contributor Author

@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
Copy link
Member

Fryguy commented Aug 17, 2020

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
Copy link
Contributor Author

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
Copy link
Member

Fryguy commented Aug 17, 2020

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
Copy link
Member

kbrock commented Aug 18, 2020

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
Copy link
Contributor Author

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

@djberg96
Copy link
Contributor Author

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

@djberg96 djberg96 closed this Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants