bpo-25041: Document AF_PACKET socket address format #4092

csabella · 2017-10-23T17:56:54Z

Add documentation for AF_PACKET.

Includes suggestions by @berkerpeksag and @vadmium from the ticket.

https://bugs.python.org/issue25041

vadmium · 2017-10-29T00:15:06Z

Doc/library/socket.rst

-
-  .. XXX document them!
+- The :const:`AF_PACKET` address family is a Linux-only packet address protocol
+  represented by the tuple ``(ifname, proto[, pkttype[, hatype[, addr]]])``


I haven’t heard of “packet address protocols” before. What does it mean, and why did you add it?

The man page for socket has:

Name Purpose Man page AF_UNIX, AF_LOCAL Local communication unix(7) AF_INET IPv4 Internet protocols ip(7) AF_INET6 IPv6 Internet protocols ipv6(7) AF_IPX IPX - Novell protocols AF_NETLINK Kernel user interface device netlink(7) AF_X25 ITU-T X.25 / ISO-8208 protocol x25(7) AF_AX25 Amateur radio AX.25 protocol AF_ATMPVC Access to raw ATM PVCs AF_APPLETALK AppleTalk ddp(7) AF_PACKET Low level packet interface packet(7) AF_ALG Interface to kernel crypto API

I think I should have said Linux-only packet interface or Linux-only low level packet interface. I'm not sure why I changed that to protocol. I may have read it in a description somewhere, but I can't find the source now.

I think the important thing here is to describe that it's a low-level interface directly to network devices.

vadmium · 2017-10-29T00:20:35Z

Apart from the strange wording introducing the address tuple, the rest looks okay to me.

benjaminp · 2018-09-11T17:24:27Z

Doc/library/socket.rst

+  where:
+
+  - *ifname* - String specifying the device name.
+  - *proto* - An endian-corrected integer specifying the Ethernet protocol


I would say "in network byte order" rather than "endian-corrected".

benjaminp · 2018-09-11T17:25:06Z

Doc/library/socket.rst

+  - *ifname* - String specifying the device name.
+  - *proto* - An endian-corrected integer specifying the Ethernet protocol
+    number.
+  - *pkttype* - Optional constant integer specifying the packet type:


Remove "constant".

benjaminp · 2018-09-11T17:26:52Z

Doc/library/socket.rst

+          PF_PACKET
+          PACKET_*
+
+   Many constants of these forms, documented in the Linux documentation, are


"Linux documentation" -> "man pages"

"Linux documentation" is used in the other definitions within this section, so I had used it for consistency. Should I change them all?

Ah, sorry, I didn't realize that term was already in use here. I suppose this is fine to leave as is.

Looking at git blame, it seems that one person started it and then all the others continued it, presumably for consistency. If man pages is better, then maybe it would be best to change it now instead of propagating Linux documentation every time?

I think it would make sense to change that, but that should probably be another change.

benjaminp · 2018-09-11T17:29:29Z

Doc/library/socket.rst

-
-  .. XXX document them!
+- The :const:`AF_PACKET` address family is a Linux-only packet address protocol
+  represented by the tuple ``(ifname, proto[, pkttype[, hatype[, addr]]])``


I think the important thing here is to describe that it's a low-level interface directly to network devices.

bedevere-bot · 2018-09-11T17:29:43Z

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.

csabella · 2018-09-11T23:19:06Z

I have made the requested changes; please review again.

bedevere-bot · 2018-09-11T23:19:08Z

Thanks for making the requested changes!

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

miss-islington · 2018-09-12T00:32:17Z

Thanks @csabella for the PR, and @benjaminp for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

miss-islington · 2018-09-12T00:32:21Z

Sorry, @csabella and @benjaminp, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 731ff68eeef58babdf2b32dc9a73b141760c2be9 3.7

miss-islington · 2018-09-12T00:32:25Z

Sorry, @csabella and @benjaminp, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 731ff68eeef58babdf2b32dc9a73b141760c2be9 3.6

…-4092). (cherry picked from commit 731ff68) Co-authored-by: Cheryl Sabella <[email protected]>

bedevere-bot added the awaiting review label Oct 23, 2017

the-knights-who-say-ni added the CLA signed label Oct 23, 2017

berkerpeksag requested review from berkerpeksag and vadmium October 25, 2017 02:00

vadmium reviewed Oct 29, 2017

View reviewed changes

benjaminp requested changes Sep 11, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Sep 11, 2018

csabella added 2 commits September 11, 2018 18:55

bpo-25041: Document AF_PACKET socket address format

658f4d2

Add blurb

fef4c5a

csabella force-pushed the bpo25041 branch from c00768c to fef4c5a Compare September 11, 2018 22:58

Requested changes per review.

a1d9706

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 11, 2018

benjaminp added needs backport to 3.6 labels Sep 12, 2018

benjaminp merged commit 731ff68 into python:master Sep 12, 2018

bedevere-bot removed the awaiting change review label Sep 12, 2018

miss-islington assigned benjaminp Sep 12, 2018

benjaminp pushed a commit that referenced this pull request Sep 12, 2018

[3.7] closes bpo-25041: Document AF_PACKET socket address format. (GH…

772addc

…-4092). (cherry picked from commit 731ff68) Co-authored-by: Cheryl Sabella <[email protected]>

benjaminp pushed a commit that referenced this pull request Sep 12, 2018

[3.6] closes bpo-25041: Document AF_PACKET socket address format. (GH…

d141bab

…-4092). (cherry picked from commit 731ff68) Co-authored-by: Cheryl Sabella <[email protected]>

csabella deleted the bpo25041 branch September 12, 2018 11:32

serhiy-storchaka added the docs Documentation in the Doc dir label Oct 31, 2018

serhiy-storchaka removed needs backport to 3.6 labels Oct 31, 2018

serhiy-storchaka unassigned benjaminp Oct 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-25041: Document AF_PACKET socket address format #4092

bpo-25041: Document AF_PACKET socket address format #4092

csabella commented Oct 23, 2017 •

edited by bedevere-bot

Loading

vadmium Oct 29, 2017

csabella Oct 29, 2017

benjaminp Sep 11, 2018

csabella Sep 11, 2018

vadmium commented Oct 29, 2017

benjaminp Sep 11, 2018

csabella Sep 11, 2018

benjaminp Sep 11, 2018

csabella Sep 11, 2018

benjaminp Sep 11, 2018

csabella Sep 11, 2018

benjaminp Sep 11, 2018

csabella Sep 11, 2018

benjaminp Sep 12, 2018

benjaminp Sep 11, 2018

bedevere-bot commented Sep 11, 2018

csabella commented Sep 11, 2018

bedevere-bot commented Sep 11, 2018

miss-islington commented Sep 12, 2018

miss-islington commented Sep 12, 2018

miss-islington commented Sep 12, 2018

bpo-25041: Document AF_PACKET socket address format #4092

bpo-25041: Document AF_PACKET socket address format #4092

Conversation

csabella commented Oct 23, 2017 • edited by bedevere-bot 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

vadmium commented Oct 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

Choose a reason for hiding this comment

bedevere-bot commented Sep 11, 2018

csabella commented Sep 11, 2018

bedevere-bot commented Sep 11, 2018

miss-islington commented Sep 12, 2018

miss-islington commented Sep 12, 2018

miss-islington commented Sep 12, 2018

csabella commented Oct 23, 2017 •

edited by bedevere-bot

Loading