Skip to content

common/win32,dokan: include bcrypt.h for NTSTATUS #47217

Merged
tchaikov merged 3 commits intoceph:mainfrom
tchaikov:wip-ntstatus
Jul 25, 2022
Merged

common/win32,dokan: include bcrypt.h for NTSTATUS #47217
tchaikov merged 3 commits intoceph:mainfrom
tchaikov:wip-ntstatus

Conversation

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Jul 21, 2022 •
edited
Loading

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@tchaikov tchaikov mentioned this pull request Jul 21, 2022
Merged
14 tasks
@tchaikov tchaikov added the win32 Specifix changes for the windows platform label Jul 21, 2022
@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 21, 2022

https://shaman.ceph.com/builds/ceph/wip-ntstatus/5623bc3c3ac4d86ce1a37c7a11c76ca615060970/

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 21, 2022 

[2022-07-21T23:52:36.000Z] [googletest] unittest_encoding.exe failed. Error: Command returned non-zero code(1): "cmd /c 'C:\ceph\unittest_encoding.exe --gtest_output=xml:C:\workspace\test_results\unittest_encoding_results.xml  > C:\workspace\test_results\unittest_encoding_results.log 2>&1'".

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 21, 2022

jenkins test windows

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 22, 2022 

[ RUN      ] EncodingException.Macros
../src/test/encoding.cc:341: Failure
Expected equality of these values:
  string(expected_what[i])
    Which is: "void lame_decoder(int) no longer understand old encoding version 100 < 200: Malformed input"
  string(e.what())
    Which is: "void lame_decoder(int) no longer understand old encoding version 100 < 200: Malformed input [buffer:3]"
[  FAILED  ] EncodingException.Macros (0 ms)

@tchaikov tchaikov marked this pull request as draft July 22, 2022 00:21
@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 22, 2022

needs to address the test failure first.

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 22, 2022

jenkins test make check

@tchaikov tchaikov marked this pull request as ready for review July 22, 2022 13:09
@github-actions github-actions bot added the tests label Jul 22, 2022
@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 22, 2022

@adamemerson i fixed the test. could you please take another look?

@tchaikov tchaikov force-pushed the wip-ntstatus branch 2 times, most recently from 1e2b086 to 6389ee0 Compare July 22, 2022 13:27
adamemerson
adamemerson approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@adamemerson adamemerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@tchaikov tchaikov force-pushed the wip-ntstatus branch 2 times, most recently from 3815da6 to cb9f789 Compare July 22, 2022 15:03
tchaikov added 3 commits July 23, 2022 00:14
@tchaikov
common/win32,dokan: include bcrypt.h for NTSTATUS
1a09e52 
to avoid the conflicting declaration of NTSTATUS from bcrypt.h and our
own typedef. as after switching to boost 1.79, we would have following compiling
failure:

In file included from ../src/dokan/options.cc:14:
../src/dokan/ceph_dokan.h:16:15: error: conflicting declaration 'typedef DWORD NTSTATUS'
   16 | typedef DWORD NTSTATUS;
      |               ^~~~~~~~
In file included from ../build.deps/mingw/boost/include/boost/asio/impl/connect_pipe.ipp:29,
                 from ../build.deps/mingw/boost/include/boost/asio/connect_pipe.hpp:79,
                 from ../build.deps/mingw/boost/include/boost/asio.hpp:64,
                 from ../src/include/win32/winsock_wrapper.h:20,
                 from <command-line>:
/usr/share/mingw-w64/include/bcrypt.h:27:16: note: previous declaration as 'typedef LONG NTSTATUS'
   27 |   typedef LONG NTSTATUS,*PNTSTATUS;
      |                ^~~~~~~~

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov
test/encoding: verify that e.what() starts with expected str
0d9eec3 
boost changes the way how it prints boost::system::system_error in
boost 1.79 -- it appends the stringified error_category at end of
exception::what(), and our buffer::malformed_input is a subclass
of boost::system::system_error.

so we cannot just compare the return value of what() with the
expected string, to be more future proof, let's check if i
starts with the expected string instead.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov
test/encoding: refactor EncodingException::Macros
b3151a9 
so it is more compacted

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 22, 2022

https://shaman.ceph.com/builds/ceph/wip-ntstatus-kefu/b3151a9c373b472c7e88080205fc256a1304fe9d/

@djgalloway
Copy link
Contributor

djgalloway commented Jul 22, 2022

@adamemerson is this still good after the force pushes?

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 25, 2022

changelog

  • move #include <bcrypt.h> up before #include <dokan.h> in src/dokan/ceph_dokan.h to avoid undefined type FTBFS.

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 25, 2022

@adamemerson @djgalloway i am going to merge this change, as the latest force pushes was trivial changes. if anything looks insane, i will fix it ASAP.

@tchaikov tchaikov merged commit 413969b into ceph:main Jul 25, 2022
@tchaikov tchaikov deleted the wip-ntstatus branch July 25, 2022 22:56
@tchaikov tchaikov mentioned this pull request Sep 3, 2022
Closed
14 tasks
@idryomov idryomov mentioned this pull request Sep 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamemerson adamemerson adamemerson approved these changes

@petrutlucian94 petrutlucian94 Awaiting requested review from petrutlucian94

Assignees

No one assigned

Labels

build/ops common tests win32 Specifix changes for the windows platform

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tchaikov @djgalloway @adamemerson