gh-92658: Add Hyper-V socket support #92755

jborean93 · 2022-05-13T00:54:35Z

Partially fixes #92658.

There are 2 outstanding questions I have for the PR:

What should connect and bind accept as the addr

Currently the code accepts a tuple of 2 16 byte strings only. This could potentially be changed to somehow accept a uuid.UUID or even potentially a string of a UUID. My C skills are rusty so I just went with the simpler solution but if it is desired to change this or add support for other types I'm happy to do so at someones direction.

How should I define/guard AF_HYPERV and # include <hvsocket.h>

Currently AF_HYPERV is defined at https://github.com/tpn/winsdk-10/blob/9b69fd26ac0c7d0b83d378dba01080e93349c2ed/Include/10.0.16299.0/shared/ws2def.h#L145-L148 and is only included if _WIN32_WINNT > 0x0604. Currently Python sets that at 0x0602 for Windows 8 compatibility which is why the PR manually defines it. The header also imports hvsocket.h for some other definitions used in the code but I'm unsure if this should be done conditionally somehow. Any suggestions or improvements would be great.

bedevere-bot · 2022-05-13T00:54:37Z

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

ronaldoussoren

On first glance the PR looks ok, with some minor issues.

That said, I'm not a Windows user and cannot test this PR myself.

Lib/test/test_socket.py

Modules/socketmodule.c

bedevere-bot · 2022-05-14T09:03:23Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Modules/socketmodule.c

Doc/library/socket.rst

jborean93 · 2022-05-19T05:01:13Z

I have made the requested changes; please review again

bedevere-bot · 2022-05-19T05:01:16Z

Thanks for making the requested changes!

@ronaldoussoren: please review the changes made to this pull request.

Lib/test/test_socket.py

Modules/socketmodule.c

Modules/socketmodule.h

Doc/library/socket.rst

Modules/socketmodule.c

jborean93 · 2022-05-19T10:11:32Z

@erlend-aasland sorry I can't seem to reply to your last review comment

After you've done the PyTuple_Check, you don't need PyArg_ParseTuple; you can just access each item quickly using PyTuple_GET_ITEM. That also makes for more compact code, FWIW.

Isn't PyArg_ParseTuple also helping to check the type. It has UU in the format string so it does all the checking needed to verify that the tuple contains 2 values and both are strings. If I didn't use that then I would need to duplicate those checks here anyway. Am I missing anything in particular here?

erlend-aasland · 2022-05-19T10:35:00Z

If I didn't use that then I would need to duplicate those checks here anyway. Am I missing anything in particular here?

No, that's correct. It's up to you to decide which APIs to use.

jborean93 · 2022-05-19T10:52:11Z

Thanks for clarifying, I'll probably keep it with PyArg_ParseTuple as it's checking the size and type in 1 go and it aligns with what the code around it seems to be following. Appreciate the review and helpful comments/suggestions.

Doc/library/socket.rst

Co-authored-by: Steve Dower <[email protected]>

erlend-aasland

Looks good, as far as I can see :) Good job!

Modules/socketmodule.h

zooba

Sorry, just one more request. This is the first time I've actually read over the whole change, and it looks great!

Modules/socketmodule.c

zooba · 2022-05-24T20:37:51Z

Great work, nice patch! Would love to see a more extensive example of how to use this (blog post or conf talk or similar), but it's very early days for 3.12, so no hurry ;)

jborean93 · 2022-05-24T21:00:38Z

Thanks for the reviews everyone, appreciate the time you've put in to help me get this in.

While not using the changes here, this is what I used as POC to talk to a Hyper-V guest for PowerShell Direct https://gist.github.com/jborean93/61854124d7686621ce9c1ade8687dc7a. I'm hoping to have it integrated in my PowerShell Remoting library at some point soon but need to deal with some asyncio problems first.

vstinner · 2022-05-24T23:55:37Z

The change broke the FreeBSD buildbot: https://buildbot.python.org/all/#/builders/483/builds/2438

The closest thing that I found is this FreeBSD commit: https://reviews.freebsd.org/rS361275 which adds
#define AF_HYPERV 43 /* HyperV sockets */ and uses struct sockaddr_hvs. It's unclear to me if struct sockaddr_hvs is public or not. Maybe for now, HyperV support should just be disabled on FreeBSD.

cc @emaste

building '_socket' extension
cc -pthread -fPIC -Wsign-compare -g -Og -Wall -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I./Include -I. -I/usr/local/include -I/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Include -I/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build -c /usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c -o build/temp.freebsd-14.0-CURRENT-amd64-3.12-pydebug/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.o
In file included from /usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:252:
In file included from /usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.h:123:
In file included from /usr/include/bluetooth.h:54:
/usr/include/netgraph/bluetooth/include/ng_btsocket.h:243:2: warning: "Make sure new member of socket address initialized" [-W#warnings]
#warning "Make sure new member of socket address initialized"
 ^
In file included from /usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:252:
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.h:301:5: error: unknown type name 'SOCKADDR_HV'
    SOCKADDR_HV hv;
    ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1588:9: error: use of undeclared identifier 'SOCKADDR_HV'
        SOCKADDR_HV *a = (SOCKADDR_HV *) addr;
        ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1588:22: error: use of undeclared identifier 'a'
        SOCKADDR_HV *a = (SOCKADDR_HV *) addr;
                     ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1588:40: error: expected expression
        SOCKADDR_HV *a = (SOCKADDR_HV *) addr;
                                       ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1588:27: error: use of undeclared identifier 'SOCKADDR_HV'
        SOCKADDR_HV *a = (SOCKADDR_HV *) addr;
                          ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1591:9: error: use of undeclared identifier 'RPC_STATUS'
        RPC_STATUS res = UuidToStringW(&a->VmId, &guidStr);
        ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1592:13: error: use of undeclared identifier 'res'
        if (res != RPC_S_OK) {
            ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1592:20: error: use of undeclared identifier 'RPC_S_OK'
        if (res != RPC_S_OK) {
                   ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1593:13: error: implicit declaration of function 'PyErr_SetFromWindowsErr' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            PyErr_SetFromWindowsErr(res);
            ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1593:37: error: use of undeclared identifier 'res'
            PyErr_SetFromWindowsErr(res);
                                    ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1597:9: error: use of undeclared identifier 'res'
        res = RpcStringFreeW(&guidStr);
        ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1597:15: error: implicit declaration of function 'RpcStringFreeW' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        res = RpcStringFreeW(&guidStr);
              ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1598:16: error: use of undeclared identifier 'res'
        assert(res == RPC_S_OK);
               ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1598:23: error: use of undeclared identifier 'RPC_S_OK'
        assert(res == RPC_S_OK);
                      ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1600:9: error: use of undeclared identifier 'res'
        res = UuidToStringW(&a->ServiceId, &guidStr);
        ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1600:15: error: implicit declaration of function 'UuidToStringW' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        res = UuidToStringW(&a->ServiceId, &guidStr);
              ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1600:30: error: use of undeclared identifier 'a'
        res = UuidToStringW(&a->ServiceId, &guidStr);
                             ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1601:13: error: use of undeclared identifier 'res'
        if (res != RPC_S_OK) {
            ^
/usr/home/buildbot/python/3.x.koobs-freebsd-564d/build/Modules/socketmodule.c:1601:20: error: use of undeclared identifier 'RPC_S_OK'
        if (res != RPC_S_OK) {
                   ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
1 warning and 20 errors generated.

emaste · 2022-05-25T00:07:02Z

The FreeBSD comment about SOCKADDR_HV:

/* In the VM, we support Hyper-V Sockets with AF_HYPERV, and the endpoint is
 * <port> (see struct sockaddr_hvs).
 *
 * On the host, Hyper-V Sockets are supported by Winsock AF_HYPERV:
 * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/user-
 * guide/make-integration-service, and the endpoint is <VmID, ServiceId> with
 * the below sockaddr:
 *
 * struct SOCKADDR_HV
 * {
 *    ADDRESS_FAMILY Family;
 *    USHORT Reserved;
 *    GUID VmId;
 *    GUID ServiceId;
 * };
 * Note: VmID is not used by FreeBSD VM and actually it isn't transmitted via
 * VMBus, because here it's obvious the host and the VM can easily identify
 * each other. Though the VmID is useful on the host, especially in the case
 * of Windows container, FreeBSD VM doesn't need it at all.
 *
 * To be compatible with similar infrastructure in Linux VMs, we have
 * to limit the available GUID space of SOCKADDR_HV so that we can create
 * a mapping between FreeBSD AF_HYPERV port and SOCKADDR_HV Service GUID.
 * The rule of writing Hyper-V Sockets apps on the host and in FreeBSD VM is:
 *
 ****************************************************************************
 * The only valid Service GUIDs, from the perspectives of both the host and *
 * FreeBSD VM, that can be connected by the other end, must conform to this *
 * format: <port>-facb-11e6-bd58-64006a7986d3.                              *
 ****************************************************************************
 *
 * When we write apps on the host to connect(), the GUID ServiceID is used.
 * When we write apps in FreeBSD VM to connect(), we only need to specify the
 * port and the driver will form the GUID and use that to request the host.
 *
 * From the perspective of FreeBSD VM, the remote ephemeral port (i.e. the
 * auto-generated remote port for a connect request initiated by the host's
 * connect()) is set to HVADDR_PORT_UNKNOWN, which is not realy used on the
 * FreeBSD guest.
 */

Linux has a similar comment, but it seems it does not define AF_HYPERV. I guess this change should apply on Windows only?

jborean93 · 2022-05-25T00:08:06Z

Hmm, seems like we should also add an #ifdef MS_WINDOWS around the code instead of just #ifdef AF_HYPERV. I'm happy to create a PR but unsure if the standard practice is to revert this and do it in another PR at the same time. Would be nice for FreeBSD VM guests to be able to use it but I'm happy to punt that to a separate PR when someone needs that feature.

vstinner · 2022-05-25T00:09:20Z

Simple fix to only build AF_HYPERV support on Windows: #93192

vstinner · 2022-05-25T00:21:14Z

While FreeBSD defines the AF_HYPERV macro, it doesn't define the HyperV socket structure:

$ grep -R sockaddr_hvs /usr/include/
# no result
$ grep -R SOCKADDR_HV /usr/include/
# no result

The Python implementation requires to get access to members of this structure. One option would be to define the structure in Python, but I'm not volunteer to maintain such structure in Python. For now, the simplest option is to limit AF_HYPERV support to Windows which defines the structure.

bedevere-bot added the awaiting review label May 13, 2022

Add Hyper-V socket support

47699cd

jborean93 force-pushed the hyperv-sockets branch from 7b86915 to 47699cd Compare May 13, 2022 01:10

jborean93 mentioned this pull request May 13, 2022

socket.connect - support custom family value #92658

Closed

ronaldoussoren requested changes May 14, 2022

View reviewed changes

Lib/test/test_socket.py Outdated Show resolved Hide resolved

Lib/test/test_socket.py Show resolved Hide resolved

Modules/socketmodule.c Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting review labels May 14, 2022

ronaldoussoren added the OS-windows label May 14, 2022

ronaldoussoren requested a review from a team May 14, 2022 09:04

erlend-aasland reviewed May 14, 2022

View reviewed changes

Modules/socketmodule.c Outdated Show resolved Hide resolved

Modules/socketmodule.c Outdated Show resolved Hide resolved

Doc/library/socket.rst Show resolved Hide resolved

Doc/library/socket.rst Outdated Show resolved Hide resolved

bedevere-bot added awaiting change review and removed awaiting changes labels May 19, 2022

bedevere-bot requested a review from ronaldoussoren May 19, 2022 05:01

erlend-aasland reviewed May 19, 2022

View reviewed changes

jborean93 added 2 commits May 20, 2022 00:50

Use strings for addr information

6c0d158

Fix doc problem

931031f

zooba reviewed May 19, 2022

View reviewed changes

Doc/library/socket.rst Outdated Show resolved Hide resolved

jborean93 and others added 2 commits May 20, 2022 06:49

Fixes for review

f39731c

Update Doc/library/socket.rst

a0b06f3

Co-authored-by: Steve Dower <[email protected]>

erlend-aasland approved these changes May 19, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 19, 2022

zooba reviewed May 23, 2022

View reviewed changes

Modules/socketmodule.h Outdated Show resolved Hide resolved

Add comment for AF_HYPERV def

d7af175

jborean93 force-pushed the hyperv-sockets branch from 84fba26 to d7af175 Compare May 23, 2022 23:53

zooba reviewed May 24, 2022

View reviewed changes

Modules/socketmodule.c Outdated Show resolved Hide resolved

Better error handling for UUID conversions

025efb0

ronaldoussoren approved these changes May 24, 2022

View reviewed changes

zooba merged commit fbd11f3 into python:main May 24, 2022

bedevere-bot removed the awaiting merge label May 24, 2022

jborean93 deleted the hyperv-sockets branch May 24, 2022 20:45

sobolevn mentioned this pull request Jun 3, 2023

gh-92658: Fix typo in docs and tests for HV_GUID_PARENT #105267

Merged

Uh oh!

gh-92658: Add Hyper-V socket support #92755

gh-92658: Add Hyper-V socket support #92755

Uh oh!

Conversation

jborean93 commented May 13, 2022

Uh oh!

bedevere-bot commented May 13, 2022

Uh oh!

ronaldoussoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented May 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jborean93 commented May 19, 2022

Uh oh!

bedevere-bot commented May 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jborean93 commented May 19, 2022

Uh oh!

erlend-aasland commented May 19, 2022

Uh oh!

jborean93 commented May 19, 2022

Uh oh!

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zooba commented May 24, 2022

Uh oh!

jborean93 commented May 24, 2022

Uh oh!

vstinner commented May 24, 2022

Uh oh!

emaste commented May 25, 2022

Uh oh!

jborean93 commented May 25, 2022

Uh oh!

vstinner commented May 25, 2022

Uh oh!

vstinner commented May 25, 2022

Uh oh!

Uh oh!