-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/sockets: follow-up on AF_PACKET support. #17657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5411062
93d0480
27109f9
cb62974
e017a32
e4de953
4847c79
23060fe
df81dda
fe00896
ba41f45
eb41f3f
da14f3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2013,6 +2013,11 @@ | |
* @cvalue ETH_P_ALL | ||
*/ | ||
const ETH_P_ALL = UNKNOWN; | ||
/** | ||
* @var int | ||
* @cvalue ETH_FRAME_LEN | ||
*/ | ||
const ETH_FRAME_LEN = UNKNOWN; | ||
#endif | ||
|
||
/** | ||
|
@@ -2156,3 +2161,19 @@ function socket_wsaprotocol_info_import(string $info_id): Socket|false {} | |
|
||
function socket_wsaprotocol_info_release(string $info_id): bool {} | ||
#endif | ||
|
||
#ifdef AF_PACKET | ||
final class SocketEthernetInfo | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: In order to reduce the overhead of converting the raw packet to an object, we could make all the props virtual and store the raw packet directly in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ll see what I can do in a following PR |
||
{ | ||
/** @readonly **/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This object will be rewritten in a following PR anyway. |
||
public Socket $socket; | ||
Comment on lines
+2166
to
+2169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add a constructor taking a raw packet as argument, so that people can create their own instances of this class. This would be useful at least in tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I planned this at some point. |
||
/** @readonly **/ | ||
public int $ethprotocol; | ||
/** @readonly **/ | ||
public string $macsrc; | ||
/** @readonly **/ | ||
public string $macdst; | ||
/** @readonly **/ | ||
public array $payload; | ||
Comment on lines
+2176
to
+2177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Expose the payload as a raw string, and expose the parsed payload separately as an object (null when we don't support the protocol). The object would be instantiated on the fly, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok as object, not sure of the value of raw payload but I can always add it sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The raw payload would be useful when the application wants to log packets, or forward them without changing. It's also useful when the object is null due to not being a supported protocol. Also, having the raw payload (always) and the object (when supported) is forward compatible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see. I can get behind it indeed then. |
||
} | ||
#endif |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
My understanding is that this is only valid for
SOCK_RAW
packets (notSOCK_DGRAM
). As onlySOCK_RAW
is supported yet, we should error when an other socket type is passed, until we add support for them.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.
agreed