bpo-27584: New addition of vSockets to the python socket module #2489

caavery · 2017-06-29T14:59:00Z

Support for AF_VSOCK on Linux only

https://bugs.python.org/issue27584

caavery · 2017-06-29T15:01:52Z

The patch is also in Issue number 27584 along with a README file that explains how to setup the environment for tests.

kushaldas · 2017-06-29T17:50:21Z

Modules/socketmodule.c

+       case AF_VSOCK:
+       {
+           struct sockaddr_vm *a = (struct sockaddr_vm *) addr;
+           return Py_BuildValue("II", a->svm_cid, a->svm_port);


This is the line failing in the Travis. https://travis-ci.org/python/cpython/jobs/248412030#L2063

You don't have the proper headers.
checking for linux/vm_sockets.h... no

So it looks like my checking for HAVE_LINUX_VM_SOCKETS_H is faulty.

Okay, so please update that, and push in your branch, the bots will pick up the changes and will test.

I've restored the original aclocal.m4 and fixed the issues you pointed out.

Thank you for your review,

Cathy

kushaldas · 2017-06-29T17:51:39Z

aclocal.m4

@@ -1,6 +1,6 @@
-# generated automatically by aclocal 1.15 -*- Autoconf -*-
+# generated automatically by aclocal 1.13.4 -*- Autoconf -*-


Why are we using an old version here?

I must have an older version installed. automake-1.13.4-3.el7.noarch I can upgrade that.

So do I make the corrections and ask for another pull?

Just update this PR.

I rebased my code to remove my patch. Is rebasing and pushing an acceptable way to make changes? Or do I need to add a new commit each time?

Thanks

Sorry to remove the aclocal.m4 from my patch.

berkerpeksag

Thank you for the PR. Could you please add your name to Misc/ACKS?

berkerpeksag · 2017-07-02T14:03:16Z

Lib/test/test_socket.py

@@ -75,12 +94,20 @@ def _have_socket_alg():
        s.close()
    return True

+def _have_socket_vsock():
+    """Check whether AF_VSOCK sockets are supported on this host."""
+    if (getCid() == None):


I'd use a shorter version:

return getCid() is not None

is get_cid acceptable?

Sorry wrong comment.

I don't understand this syntax. It looks like return the cid if it is not None or do nothing and continue. getCid will return an integer or None. I want to translate that into return True or False in _have_socket_vsock().

@caavery read @berkerpeksag's suggestion as a contraction of the following:

foo = getCid is not None return foo

foo will contain a boolean value which will be returned.

Yeah, that's what I was trying to say. Sorry for being unclear!

And +1 for renaming it to get_cid, thanks!

berkerpeksag · 2017-07-02T14:04:49Z

Lib/test/test_socket.py

@@ -191,7 +218,6 @@ def setUp(self):
        except OSError:
            self.skipTest('unable to bind RDS socket')

-


Please don't make unrelated cosmetic changes.

berkerpeksag · 2017-07-02T14:06:15Z

Lib/test/test_socket.py

@@ -5627,6 +5734,10 @@ def test_main():
    tests.extend([BasicRDSTest, RDSTest])
    tests.append(LinuxKernelCryptoAPI)
    tests.extend([
+        BasicVSOCKTest,
+        ThreadedVSOCKSocketStreamTest


Style nit: Please add trailing comma.

berkerpeksag · 2017-07-02T14:12:03Z

Lib/test/test_socket.py

@@ -44,6 +46,23 @@
 except ImportError:
    _socket = None

+def getCid():
+    import struct
+    import fcntl


You may want to check if fcntl is available on the platform.

berkerpeksag · 2017-07-02T14:14:36Z

Doc/library/socket.rst

@@ -149,6 +149,14 @@ created.  Socket addresses are represented as follows:

  .. versionadded:: 3.6

+- :const:`AF_VSOCK` allows communication between virtual machines and
+   their hosts. The sockets are represented as a (CID, port) tuple


Indentation is off here:

- :const:`AF_VSOCK` allows [...] their hosts. [...] Availability: Linux >= 4.8. QEMU >= 2.8.

berkerpeksag · 2017-07-02T14:17:09Z

Doc/library/socket.rst

+   their hosts. The sockets are represented as a (CID, port) tuple
+   where the context ID or CID and port are integers.
+
+  Availability: Linux >= 4.8. QEMU >= 2.8.


Do we need to mention QEMU here? I'd say just document kernel version.

Well I thought it would be better informationally to have it in but if it is not acceptable I can remove it.

berkerpeksag · 2017-07-02T14:17:42Z

Doc/library/socket.rst

@@ -149,6 +149,14 @@ created.  Socket addresses are represented as follows:

  .. versionadded:: 3.6

+- :const:`AF_VSOCK` allows communication between virtual machines and
+   their hosts. The sockets are represented as a (CID, port) tuple


Style nit: (CID, port) -> ``(CID, port)``

berkerpeksag · 2017-07-02T14:19:12Z

Lib/test/test_socket.py

@@ -44,6 +46,23 @@
 except ImportError:
    _socket = None

+def getCid():


Style nit: Can we use an under_score name here?

No problem.

is get_cid acceptable?

berkerpeksag · 2017-07-02T14:22:21Z

Lib/test/test_socket.py

+          "This test can only be run on a virtual guest.")
+class ThreadedVSOCKSocketStreamTest(unittest.TestCase, ThreadableTest):
+
+    def __init__(self, methodName = 'runTest'):


Style nit: No need to add spaces around =.

methodName= 'runTest'

Same for line 411:

methodName=methodName

berkerpeksag · 2017-07-07T11:00:38Z

Looks like your branch contains additional commits and unfortunately it makes code review harder. Could you rebase your branch against current master? You might find https://devguide.python.org/gitbootcamp/#syncing-with-upstream helpful.

caavery · 2017-07-07T11:11:05Z

@berkerpeksag

So you want me to sync my github branch bpo-27584 with the upstream cpython branch? Is that correct? I'm just a little unclear as to what you are asking.

Cathy

caavery · 2017-07-07T11:20:50Z

@berkerpeksag

Or do you want me to rebase both my master ( fork of upstream ) and my bpo-27584 branch?

Thanks again

berkerpeksag · 2017-07-07T11:32:34Z

So you want me to sync my github branch bpo-27584 with the upstream cpython branch? Is that correct?

Correct. Your branch bpo-27584 now contains 34 commits and 30 of them are unrelated to the vSockets feature as you can see at https://github.com/python/cpython/pull/2489/files (the reason is probably this merge commit)

caavery · 2017-07-07T11:51:05Z

@berkerpeksag

Sorry I seem to have an issue.
git fetch upstream
git rebase upstream/master

Python Dev~/caavery.github/cpython# git push origin bpo-27584
To git@github.com:caavery/cpython.git
! [rejected] bpo-27584 -> bpo-27584 (non-fast-forward)
error: failed to push some refs to 'git@github.com:caavery/cpython.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

caavery · 2017-07-07T12:30:23Z

@berkerpeksag

Sorry this is my first time with github and working with remote branches. I just forced the push into my branch bpo-27584. My branch says that 'This branch is 4 commits ahead of python:master. ' which is what I think you wanted.

berkerpeksag · 2017-07-07T12:46:32Z

My branch says that 'This branch is 4 commits ahead of python:master. ' which is what I think you wanted.

Looks great, thanks! I will try to take another look at the patch while we wait for Kushal's review.

caavery · 2017-07-19T10:46:23Z

@kushaldas

Is there anything else I should be doing? The patches have passed all tests so far and I have addressed all the issues raised.

Thanks,

Cathy

ilevkivskyi · 2017-08-17T15:10:14Z

Doc/library/socket.rst

@@ -149,6 +149,14 @@ created.  Socket addresses are represented as follows:

  .. versionadded:: 3.6

+- :const:`AF_VSOCK` allows communication between virtual machines and
+  their hosts. The sockets are represented as a ``(CID,port)`` tuple


Shouldn't there be space after coma? (CID, port).

Yes that got accidentally deleted.

rmsr · 2017-08-17T20:53:54Z

Doc/library/socket.rst

+  their hosts. The sockets are represented as a ``(CID, port)`` tuple
+  where the context ID or CID and port are integers.
+
+  Availability: Linux >= 4.8. QEMU >= 2.8.


Given that vsockets originated with VMWare, add a note that they are available in that environment also?

This is a bit more complicated. The availability in a particular linux kernel depends on the implementation.
virtio-vsock is Linux >= 4.8. QEMU >= 2.8.
vmci_transport is Linux >= 3.9
Should I just change it to when it first appeared which is v3.9?

I meant availability when running under the VMWare hypervisor. As for the kernel, to keep it simple, whichever version supplies full functionality with both qemu and vmware.

tiran

I added some comments.

You also have to add the vsock struct to typedef union sock_addr in socketmodule.h.

Please use blurb to add a news entry, too.

tiran · 2017-08-17T21:15:35Z

Lib/test/test_socket.py

+def get_cid():
+    if fcntl is None:
+        return None
+    if not os.path.exists("/dev/vsock"):


Please rewrite this block with proper context manager. You can skip the os.path.exists check, too.

try: with open(...) as f: r = fcntl.ioctl(f, socket.IOCTL_VM_SOCKETS_GET_LOCAL_CID, " ") except OSError: return None else: return struct.unpack("I", r)[0]

tiran · 2017-08-17T21:25:39Z

Modules/socketmodule.c

-        socklen_t flagsize = sizeof flag;
+#ifdef AF_VSOCK
+        if (s->sock_family == AF_VSOCK) {
+            uint64_t vflag = 0;


I've been starring at this block for a minute until I realized that it handles uint64_t instead of int. Please add a comment that this block that explains the difference.

tiran · 2017-08-17T21:26:29Z

Modules/socketmodule.c

+                         (void *)&vflag, &flagsize);
+            if (res < 0)
+                return s->errorhandler();
+            return PyLong_FromLong(vflag);


It's an uint64_t, you have to use PyLong_FromUnsignedLong().

tiran · 2017-08-17T21:27:00Z

Modules/socketmodule.h

+#ifdef HAVE_LINUX_VM_SOCKETS_H
+# include <linux/vm_sockets.h>
+#  ifndef AF_VSOCK
+#    define AF_VSOCK 40


Why is this necessary?

bedevere-bot · 2017-08-17T21:28:16Z

A Python core developer, tiran, 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 didn't expect the Spanish Inquisition!
I will then notify tiran along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.

caavery · 2017-08-25T10:36:43Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-08-25T10:36:45Z

Nobody expects the Spanish Inquisition!

@kushaldas, @tiran: please review the changes made to this pull request.

tiran

You are almost there!

tiran · 2017-08-26T13:55:59Z

Modules/socketmodule.h

+#ifdef HAVE_LINUX_VM_SOCKETS_H
+# include <linux/vm_sockets.h>
+#else
+# undef AF_VSOCK


I don't think #undef AF_VSOCK is necessary. Please remove it.

It needs to be there. In the case that vm_sockets.h is not available in include/linux which is the case on the last build bot run. Not all of the bots have the newer kernel.That way the code dependent on vm_sockets.h (AF_VSOCK) will not try to compile.

checking for linux/vm_sockets.h... no

This is also the way its done it at the top of socketmodule.h with AF_NETLINK

I also noticed AF_CAN was not adding the #undef AF_CAN in socketmodule.h so I renamed linux/can.h, ran configure and make and sure enough any code conditionally compiling under #ifdef AF_CAN in socketmodule.c failed.

tiran · 2017-08-26T13:56:34Z

Modules/socketmodule.h

@@ -193,6 +199,9 @@ typedef union sock_addr {
 #ifdef HAVE_SOCKADDR_ALG
    struct sockaddr_alg alg;
 #endif
+#ifdef HAVE_LINUX_VM_SOCKETS_H


Please be consistent and use #ifdef AF_VSOCK in all places.

bedevere-bot · 2017-08-26T13:58:06Z

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 didn't expect the Spanish Inquisition!. 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.

caavery · 2017-08-26T22:05:12Z

OK there seems to be a problem with the build now. I'll address it next week. Thanks.

tiran · 2017-08-28T08:40:06Z

You were right, either #undef AF_VSOCK is required or you have to replace all #ifdef AF_VSOCK with #ifdef HAVE_LINUX_VM_SOCKETS_H. The address family and the VM socket struct are defined in different header files. AF_VSOCK can be present w/o linux/vm_sockets.h.

caavery · 2017-09-05T12:39:40Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-05T12:39:42Z

Nobody expects the Spanish Inquisition!

@kushaldas, @tiran: please review the changes made to this pull request.

Support for AF_VSOCK on Linux only

Fixed syntax and naming problems. Fixed #ifdef AF_VSOCK checking Restored original aclocal.m4

Added checking for fcntl and thread modules.

Fixed white space error

Added back comma in (CID, port).

Added news file. socket.rst now reflects first Linux introduction of AF_VSOCK. Fixed get_cid in test_socket.py. Replaced PyLong_FromLong with PyLong_FromUnsignedLong in socketmodule.c Got rid of extra AF_VSOCK #define. Added sockaddr_vm to sock_addr.

Minor cleanup.

Put back #undef AF_VSOCK as it is necessary when vm_sockets.h is not installed.

tiran

LGTM, thanks!

vstinner · 2024-05-23T13:28:56Z

FYI I wrote PR gh-119463 to skip the tests if get_cid() returns VMADDR_CID_ANY. Apparently, the Linux kernel 6.9 changed /dev/vsock device.

the-knights-who-say-ni added the CLA not signed label Jun 29, 2017

caavery closed this Jun 29, 2017

caavery reopened this Jun 29, 2017

terryjreedy added CLA signed and removed CLA not signed labels Jun 29, 2017

the-knights-who-say-ni added the CLA signed label Jun 29, 2017

python deleted a comment from the-knights-who-say-ni Jun 29, 2017

kushaldas requested changes Jun 29, 2017

View reviewed changes

berkerpeksag reviewed Jul 2, 2017

View reviewed changes

ilevkivskyi reviewed Aug 17, 2017

View reviewed changes

brettcannon added the awaiting change review label Aug 17, 2017

rmsr reviewed Aug 17, 2017

View reviewed changes

tiran requested changes Aug 17, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 17, 2017

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 25, 2017

tiran requested changes Aug 26, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 26, 2017

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 5, 2017

caavery added 8 commits September 5, 2017 11:09

bpo-27584: New addition of vSockets to the python socket module

2f7bdaa

Support for AF_VSOCK on Linux only

bpo-27584: Fixes for V2

bec6612

Fixed syntax and naming problems. Fixed #ifdef AF_VSOCK checking Restored original aclocal.m4

bpo-27584: Fixes for V3

c3027e5

Added checking for fcntl and thread modules.

bpo-27584: Fixes for V4

d11a8db

Fixed white space error

bpo-27584: Fixes for V5

ce380ae

Added back comma in (CID, port).

bpo-27584: Fixes for V6

0f653c6

Added news file. socket.rst now reflects first Linux introduction of AF_VSOCK. Fixed get_cid in test_socket.py. Replaced PyLong_FromLong with PyLong_FromUnsignedLong in socketmodule.c Got rid of extra AF_VSOCK #define. Added sockaddr_vm to sock_addr.

bpo-27584: Fixes for V7

42026f3

Minor cleanup.

bpo-27584: Fixes for V8

c3a9490

Put back #undef AF_VSOCK as it is necessary when vm_sockets.h is not installed.

tiran approved these changes Sep 6, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 6, 2017

tiran merged commit effc12f into python:master Sep 6, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

peterjpxie mentioned this pull request Mar 25, 2023

gh-101384: add socket timeout to ThreadedVSOCKSocketStreamTest and skip it on WSL #101419

Merged

vstinner mentioned this pull request May 23, 2024

gh-119461: Skip ThreadedVSOCKSocketStreamTest if cid is VMADDR_CID_ANY #119463

Closed

		@@ -1,6 +1,6 @@
		# generated automatically by aclocal 1.15 -- Autoconf --
		# generated automatically by aclocal 1.13.4 -- Autoconf --

		@@ -191,7 +218,6 @@ def setUp(self):
		except OSError:
		self.skipTest('unable to bind RDS socket')

bpo-27584: New addition of vSockets to the python socket module #2489

bpo-27584: New addition of vSockets to the python socket module #2489

Conversation

caavery commented Jun 29, 2017 • edited by bedevere-bot Loading

caavery commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musically-ut Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Jul 7, 2017 • edited Loading

caavery commented Jul 7, 2017

caavery commented Jul 7, 2017

berkerpeksag commented Jul 7, 2017

caavery commented Jul 7, 2017

caavery commented Jul 7, 2017

berkerpeksag commented Jul 7, 2017

caavery commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 17, 2017

caavery commented Aug 25, 2017

bedevere-bot commented Aug 25, 2017

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caavery Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 26, 2017

caavery commented Aug 26, 2017

tiran commented Aug 28, 2017

caavery commented Sep 5, 2017

bedevere-bot commented Sep 5, 2017

tiran left a comment

Choose a reason for hiding this comment

vstinner commented May 23, 2024

caavery commented Jun 29, 2017 •

edited by bedevere-bot

Loading

musically-ut Jul 3, 2017 •

edited

Loading

berkerpeksag commented Jul 7, 2017 •

edited

Loading

tiran left a comment •

edited

Loading

caavery Aug 27, 2017 •

edited

Loading