GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-27584: New addition of vSockets to the python socket module by caavery · Pull Request #2489 · python/cpython

Conversation

@caavery

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Support for AF_VSOCK on Linux only

https://bugs.python.org/issue27584

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jun 29, 2017
@caavery

caavery commented Jun 29, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery caavery closed this Jun 29, 2017
@caavery caavery reopened this Jun 29, 2017
@terryjreedy terryjreedy added CLA signed and removed CLA not signed labels Jun 29, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 29, 2017
@python python deleted a comment from the-knights-who-say-ni Jun 29, 2017
kushaldas
kushaldas requested changes Jun 29, 2017
View reviewed changes
Comment thread Modules/socketmodule.c Outdated

@kushaldas kushaldas Jun 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jun 29, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kushaldas kushaldas Jun 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Thank you for your review,

Cathy

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aclocal.m4 Outdated

@kushaldas kushaldas Jun 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why are we using an old version here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jun 29, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kushaldas kushaldas Jun 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just update this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry to remove the aclocal.m4 from my patch.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
berkerpeksag
berkerpeksag reviewed Jul 2, 2017
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd use a shorter version:

return getCid() is not None

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

is get_cid acceptable?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry wrong comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@musically-ut musically-ut Jul 3, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Jul 4, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

And +1 for renaming it to get_cid, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please don't make unrelated cosmetic changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style nit: Please add trailing comma.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/socket.rst Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Indentation is off here:

- :const:`AF_VSOCK` allows [...]
  their hosts. [...]

  Availability: Linux >= 4.8. QEMU >= 2.8.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/socket.rst Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/socket.rst Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style nit: Can we use an under_score name here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No problem.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Jul 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

is get_cid acceptable?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@berkerpeksag berkerpeksag Jul 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Style nit: No need to add spaces around =.

methodName= 'runTest'

Same for line 411:

methodName=methodName

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@berkerpeksag

berkerpeksag commented Jul 7, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

@berkerpeksag

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

Thanks again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Jul 7, 2017

Copy link
Copy Markdown
Member

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

@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 [email protected]:caavery/cpython.git
! [rejected] bpo-27584 -> bpo-27584 (non-fast-forward)
error: failed to push some refs to '[email protected]: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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Jul 7, 2017

Copy link
Copy Markdown
Contributor Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@berkerpeksag

berkerpeksag commented Jul 7, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Jul 19, 2017

Copy link
Copy Markdown
Contributor Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ilevkivskyi
ilevkivskyi reviewed Aug 17, 2017
View reviewed changes
Comment thread Doc/library/socket.rst Outdated

@ilevkivskyi ilevkivskyi Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Aug 17, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes that got accidentally deleted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brettcannon brettcannon added the awaiting change review label Aug 17, 2017
rmsr
rmsr reviewed Aug 17, 2017
View reviewed changes
Comment thread Doc/library/socket.rst Outdated

@rmsr rmsr Aug 17, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Aug 23, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rmsr rmsr Aug 26, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
tiran
tiran suggested changes Aug 17, 2017
View reviewed changes

@tiran tiran left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated

@tiran tiran Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.c Outdated

@tiran tiran Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.c Outdated

@tiran tiran Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.h Outdated

@tiran tiran Aug 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why is this necessary?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Aug 17, 2017

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting change review labels Aug 17, 2017
@caavery

caavery commented Aug 25, 2017

Copy link
Copy Markdown
Contributor Author

I didn't expect the Spanish Inquisition!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Aug 25, 2017
@bedevere-bot

bedevere-bot commented Aug 25, 2017

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tiran
tiran suggested changes Aug 26, 2017
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You are almost there!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.h Outdated

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@caavery caavery Aug 27, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
LuckySherry reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/socketmodule.h Outdated

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting change review labels Aug 26, 2017
@bedevere-bot

bedevere-bot commented Aug 26, 2017

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Aug 26, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tiran

tiran commented Aug 28, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caavery

caavery commented Sep 5, 2017

Copy link
Copy Markdown
Contributor Author

I didn't expect the Spanish Inquisition!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Sep 5, 2017
@bedevere-bot

bedevere-bot commented Sep 5, 2017

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
caavery added 8 commits September 5, 2017 11:09
@caavery
bpo-27584: New addition of vSockets to the python socket module
2f7bdaa 
Support for AF_VSOCK on Linux only
@caavery
bpo-27584: Fixes for V2
bec6612 
Fixed syntax and naming problems.
Fixed #ifdef AF_VSOCK checking
Restored original aclocal.m4
@caavery
bpo-27584: Fixes for V3
c3027e5 
Added checking for fcntl and thread modules.
@caavery
bpo-27584: Fixes for V4
d11a8db 
Fixed white space error
@caavery
bpo-27584: Fixes for V5
ce380ae 
Added back comma in (CID, port).
@caavery
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.
@caavery
bpo-27584: Fixes for V7
42026f3 
Minor cleanup.
@caavery
bpo-27584: Fixes for V8
c3a9490 
Put back #undef AF_VSOCK as it is  necessary when vm_sockets.h is not installed.
tiran
tiran approved these changes Sep 6, 2017
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Sep 6, 2017
@tiran
tiran merged commit effc12f into python:master Sep 6, 2017
@Mariatta Mariatta removed the awaiting merge label Oct 8, 2017
@peterjpxie peterjpxie mentioned this pull request Mar 25, 2023
gh-101384: add socket timeout to ThreadedVSOCKSocketStreamTest and skip it on WSL #101419
Merged
@vstinner

vstinner commented May 23, 2024

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request May 23, 2024
gh-119461: Skip ThreadedVSOCKSocketStreamTest if cid is VMADDR_CID_ANY #119463
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag left review comments
@kushaldas kushaldas kushaldas requested changes
@ilevkivskyi ilevkivskyi ilevkivskyi left review comments
+3 more reviewers
@rmsr rmsr rmsr left review comments
@musically-ut musically-ut musically-ut left review comments
@tiran tiran tiran approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@caavery @berkerpeksag @bedevere-bot @tiran @vstinner @rmsr @kushaldas @musically-ut @ilevkivskyi @brettcannon @Mariatta @the-knights-who-say-ni @terryjreedy
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.