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

Fix wrong config defines in boost/windows.cfg #6492

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

ChristophStrehle
Copy link
Contributor

@ChristophStrehle ChristophStrehle commented Jun 7, 2024

Fix defines for boost test macros

The _MESSAGE macros were causing an unusedScopedObject warning because
the created string was not used. Casting the message to void fixed the
warnings.
Add missing BOOST_AUTO_TEST_CASE_TEMPLATE macro accidentally removed
with d1b3670

Fix wrong defined in windows.cfg

INVALID_HANDLE_VALUE is defined as -1 in the windows sdk
INVALID_SOCKET is defined as ~0 in the windwos sdk

Add missing generic rights

@orbitcowboy
Copy link
Collaborator

Thanks! Please add some test cases at test/cfg as well

test/cfg/windows.cpp Show resolved Hide resolved
test/cfg/boost.cpp Show resolved Hide resolved
@chrchr-github
Copy link
Collaborator

Please rebase, there was a CI failure.

<define name="INVALID_HANDLE_VALUE" value="0"/>
<define name="INVALID_SOCKET" value="0"/>
<define name="INVALID_HANDLE_VALUE" value="-1"/>
<define name="INVALID_SOCKET" value="~0"/>
Copy link
Owner

Choose a reason for hiding this comment

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

should INVALID_SOCKET also be -1 ? ~0 is -1 if 2-complement is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-1 would do the same thing as ~0. I decided to use ~0 as INVALID_SOCKET is defined like this in winsock.h, probably because SOCKET is unsigned. Should I rather use -1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there actual documentation of the value of INVALID_SOCKET? IIRC depending on the license we cannot simply copy contents from existing headers as that would be a violation - MinGW (or MSYS?) got into hot water about this a long time ago - I cannot find anything about it though.

https://learn.microsoft.com/en-us/windows/win32/winsock/socket-data-type-2 seems to imply it is -1 or negative.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might have actually been Cygwin and might be related to this: https://www.cygwin.com/faq.html#faq.programming.win32-headers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use the headers of MinGW as source but that licensing seems to be quite convoluted with a mixture of various ones depending on the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even Microsoft cannot copyright the value of a constant, and we are not copying/redistributing their headers or implementing their API anyway.
What's more concerning is that we define SOCKET as just int, whereas it should be __int64 or equivalent on 64bit platforms. But that's probably out of scope for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I can't find any documentation on the value of INVALID_SOCKET and I can't find a mirror of the windows headers that I could refer to.
I came up with the ~0 because I looked this up in the WinSock2.h on my Windows machine. I did not copy paste anything as this would introduce yet another warning because of the C-style cast used there to cast the ~0 into a SOCKET. The SOCKET itself is defined as a UINT_PTR and this is defined in BaseTsd.h as unsigned __int64 for 64 bit or as unsigned int for 32 bit.

Here:
https://learn.microsoft.com/en-us/windows/win32/winsock/handling-winsock-errors
they mention a value of 0xffff which is probably not correct on a 64 bit machine.

Here I found a winsock.h in the Microsoft repo on GitHub:
https://github.com/microsoft/service-fabric/blob/bc5c2bd546e98812509330273ae1bc6502276f87/src/prod/src/pal/src/winsock2.h#L56
there the INVALID_SOCKET is the same as on my machine but there SOCKET is only an int.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure how to handle this. I personally only added stuff which was explicitly documented but I cannot speak for other submissions. I just jumped on this because you explicitly mentioned the original header as a source.

Maybe this is just an issue of re-licensing so maybe the configuration files simply need to be put into the public domain instead of GPL. Or it is even a complete non-issue as we are not using it in code.

CC @danmar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@firewave thanks for the hint, I have to admit that I hadn't paid enough attention to the license but I don't think we have a licensing issue here. I did not copy any code from any Microsoft header into the code that is redistributed here. I only used the original headers form the sdk as a source of information for the values and for my commit message.

What I used can be found under the MIT license or the BSD license in the Microsoft GitHub repos as well:
https://github.com/microsoft/service-fabric/blob/bc5c2bd546e98812509330273ae1bc6502276f87/src/prod/src/pal/src/winsock2.h#L56
https://github.com/microsoft/mu_plus/blob/2d6746b8fe04a9be3998b3e1a682a4cbffcd7345/XmlSupportPkg/Library/XmlTreeLib/fasterxml/fasterxml.h#L949

So I'm pretty sure that this was fine. However, I have now removed the explicit mention from the commit message and form the comments above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great. Thanks for looking into this.

As this was quite a while ago (probably 15+ years - maybe even longer) it is likely the license has changed. Also it is no longer the Microsoft of old (at least in relation to Linux/open-source).

test/cfg/boost.cpp Show resolved Hide resolved
@chrchr-github chrchr-github changed the title Fix wrong config defines Fix wrong config defines in bost/windows.cfg Jun 12, 2024
@chrchr-github chrchr-github changed the title Fix wrong config defines in bost/windows.cfg Fix wrong config defines in boost/windows.cfg Jun 12, 2024
The _MESSAGE macros were causing an unusedScopedObject warning because
the created string was not used. Casting the message to void fixed the
warnings.

Add missing `BOOST_AUTO_TEST_CASE_TEMPLATE` macro accidentally removed
with d1b3670
INVALID_HANDLE_VALUE is defined as `-1` in the windows sdk
INVALID_SOCKET is defined as `~0` in the windwos sdk

Add missing generic rights
Add one test for the `BOOST_<level>_MESSAGE` macros
Add one test for the `BOOST_AUTO_TEST_CASE_TEMPLATE`
There should be no Cppcheck warning when checking for 0 and
`INVALID_HANDLE_VALUE` or `INVALID_SOCKET`
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

lgtm

@firewave @chrchr-github do you feel we can merge this?

@firewave firewave merged commit 9044a17 into danmar:main Jun 20, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants