Skip to content

bpo-31425: Expose AF_QIPCRTR in socket module#3706

Merged
taleinat merged 3 commits into
python:masterfrom
andersson:bpo-31425
Sep 26, 2018
Merged

bpo-31425: Expose AF_QIPCRTR in socket module#3706
taleinat merged 3 commits into
python:masterfrom
andersson:bpo-31425

Conversation

@andersson

@andersson andersson commented Sep 23, 2017

Copy link
Copy Markdown
Contributor

The AF_QIPCRTR address family was introduced in Linux v4.7, expose this
in the Python socket module to allow applications to operate on sockets
of this type.

Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org

https://bugs.python.org/issue31425

@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@andersson

Copy link
Copy Markdown
Contributor Author

The CLA is signed, but unfortunately I did not have the "Github name" filled in on my account. This is corrected now.

@tiran tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add tests and maybe an example, too.

Comment thread Modules/socketmodule.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please remove the extra space after sizeof

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For the record, I'm ignoring this to follow the coding style in this function.

Comment thread Modules/socketmodule.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You must add the new addr type to the large sock_addr_t union.

@bedevere-bot

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

@tiran tiran self-assigned this Nov 7, 2017
@tiran tiran added the type-feature A feature request or enhancement label Nov 7, 2017
@andersson
andersson force-pushed the bpo-31425 branch 2 times, most recently from e58b18e to 12e9198 Compare November 7, 2017 19:18
The AF_QIPCRTR address family was introduced in Linux v4.7, expose this
in the Python socket module to allow applications to operate on sockets
of this type.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

@tiran tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PR looks fine to me

@vstinner @serhiy-storchaka Can you have a look, please? We don't have a dedicated expert for the socket module. You have touched some code recently, too.

Comment thread Modules/socketmodule.c
Comment thread Modules/socketmodule.c Outdated
Comment thread Modules/socketmodule.c Outdated
Comment thread Modules/socketmodule.c Outdated
Comment thread Doc/library/socket.rst Outdated
Comment thread Doc/library/socket.rst
@tiran tiran removed their assignment Mar 24, 2018
@tiran

tiran commented Mar 24, 2018

Copy link
Copy Markdown
Member

@andersson I'm sorry that your PR didn't make it into Python 3.7.

Please rebase your PR on master and address @serhiy-storchaka's comments.

@csabella

Copy link
Copy Markdown
Contributor

@andersson Any chance you would be able to respond to @tiran and @serhiy-storchaka 's requests? Seems like this is close to being able to be merged. Thanks!

# Conflicts:
#	Doc/library/socket.rst
#	Lib/test/test_socket.py
@taleinat

taleinat commented Sep 26, 2018

Copy link
Copy Markdown
Contributor

Due to this being nearly ready to go but untouched for over 6 months, I've gone ahead and addressed @serhiy-storchaka's latest review comments, and merged the latest master branch in.

@serhiy-storchaka, @tiran, please consider reviewing again.

@serhiy-storchaka

Copy link
Copy Markdown
Member

Thank you @taleinat! All my comments have been addressed.

@taleinat
taleinat merged commit bb81651 into python:master Sep 26, 2018
@andersson

Copy link
Copy Markdown
Contributor Author

Thanks for picking this up @taleinat and thanks for merging it @serhiy-storchaka!

Comment thread Doc/library/socket.rst
family is represented as a ``(node, port)`` tuple where the *node* and *port*
are non-negative integers.

.. versionadded:: 3.7

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be 3.8, not 3.7.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants