bpo-27584: New addition of vSockets to the python socket module by caavery · Pull Request #2489 · python/cpython

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

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

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

I'd use a shorter version:
returngetCid() isnotNone

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=getCidisnotNonereturnfoo
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

Please don't make unrelated cosmetic changes.

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

Style nit: Please add trailing comma.

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

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

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

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

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

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

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

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

Style nit: Can we use an under_score name here?

No problem.

is get_cid acceptable?

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

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

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

Yes that got accidentally deleted.

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

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

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

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

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

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

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

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

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:masterSep 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

Uh oh!

Uh oh!

Conversation

caavery commented Jun 29, 2017• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caavery commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

musically-utJul 3, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Jul 7, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caavery commented Jul 7, 2017

Uh oh!

caavery commented Jul 7, 2017

Uh oh!

berkerpeksag commented Jul 7, 2017

Uh oh!

caavery commented Jul 7, 2017

Uh oh!

caavery commented Jul 7, 2017

Uh oh!

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

musically-utJul 3, 2017•
edited
Loading

berkerpeksag commented Jul 7, 2017•
edited
Loading

tiran left a comment •
edited
Loading