Prefer NPCAP_AD prefix for Npcap over SKF_AD prefix#1478
Prefer NPCAP_AD prefix for Npcap over SKF_AD prefix#1478dmiller-nmap wants to merge 2 commits intothe-tcpdump-group:masterfrom
Conversation
As discussed in the-tcpdump-group#1473, to avoid namespace collision and confusion, use separate namespaces for Npcap's and Linux's BPF extensions. In gencode.c, known-good tested code continues to use SKF_AD_ for both, but a comment and conditional definition of those constants should make things clearer.
dmiller-nmap
changed the title
| */ | ||
| #if defined(SKF_AD_VLAN_TAG_PRESENT) | ||
| /* Npcap does not provide auxiliary data */ | ||
| #if defined(SKF_AD_VLAN_TAG_PRESENT) && !defined(NPCAP_AD_VLAN_TAG_PRESENT) |
There was a problem hiding this comment.
Is it true in this context that so long as SKF_AD_VLAN_TAG_PRESENT is defined, NPCAP_AD_VLAN_TAG_PRESENT is never defined?
There was a problem hiding this comment.
As it currently stands, Npcap's npcap-bpf.h conditionally defines the same SKF_AD_* constants it did before (https://github.com/nmap/npcap/blob/a4e1690925da1c0086400a1dab7688567818ec5f/Common/npcap-bpf.h#L230-L238):
/* Npcap SDK 1.15 defined these names instead, so they are preserved here for
* compatibility and to simplify integration with existing Linux-targeted code.
* However, it is preferred to use the NPCAP_AD_* prefix in new code. */
#ifndef SKF_AD_OFF
#define SKF_AD_OFF NPCAP_AD_OFF
#define SKF_AD_VLAN_TAG NPCAP_AD_VLAN_TAG
#define SKF_AD_VLAN_TAG_PRESENT NPCAP_AD_VLAN_TAG_PRESENT
#define SKF_AD_MAX NPCAP_AD_MAX
#endifSo in a Npcap build, both constants are defined. We can still change this if necessary.
There was a problem hiding this comment.
Based on what you write, as far as I see, if there is a good way to remove SKF_AD_VLAN_TAG_PRESENT from Npcap scope now, it would be best to do that, then this specific change will not be required and things will be a bit simpler; if it is too late for that, this comment needs to explain this context a bit better:
/*
* Npcap does not provide auxiliary data. However, it can define
* SKF_AD_VLAN_TAG_PRESENT if it defines NPCAP_AD_VLAN_TAG_PRESENT.
*/| * This requires special treatment. | ||
| */ | ||
| #if defined(SKF_AD_VLAN_TAG_PRESENT) | ||
| #if defined(SKF_AD_VLAN_TAG_PRESENT) || defined(NPCAP_AD_VLAN_TAG_PRESENT) |
There was a problem hiding this comment.
Is it true in this context that so long as SKF_AD_VLAN_TAG_PRESENT is defined, it does not matter if NPCAP_AD_VLAN_TAG_PRESENT is defined, and so long as SKF_AD_VLAN_TAG_PRESENT is not defined, NPCAP_AD_VLAN_TAG_PRESENT is certainly not defined? In other words, changing this line seems to be a no-op.
There was a problem hiding this comment.
I intended this to draw attention to the fact that this block is active in a Npcap build as well, clarifying intent of the preprocessor directive, especially in contrast to other places in this file where SKF_AD_* constants are instead guarded with #if defined(__linux__).
There was a problem hiding this comment.
In this context it seems much easier to follow if this specific change was just:
--- a/gencode.c
+++ b/gencode.c
@@ -8871,6 +8871,12 @@ gen_vlan(compiler_state_t *cstate, bpf_u_int32 vlan_num, int has_vlan_tag)
* Newer version of the Linux kernel pass around
* packets in which the VLAN tag has been removed
* from the packet data and put into metadata.
+ * (Specifically, the kernel started defining the constant
+ * in release 3.8.13 and started using it for packet
+ * filtering in release 3.15.8.)
+ *
+ * Npcap since version 1.81 supports the same feature, in
+ * which case the same constant has been defined by now.
*
* This requires special treatment.
*/
|
Thank you, this generally makes sense, there is space for potential minor clean-ups as commented. |
|
|
||
| /* | ||
| * It's an absolute load. | ||
| * Is the offset a special Linux offset that we know about? |
There was a problem hiding this comment.
If this block is no longer Linux-specific, the comment needs to mention either both possibilities, or none.
| return b0; | ||
| } | ||
|
|
||
| /* Npcap defines these constants in a way that is compatible with Linux, as |
There was a problem hiding this comment.
"...compatible with Linux at source code level"
|
Thank you for waiting. I understand the proposed change better now, please address as many of today's comments as possible and rebase on the current master branch. |
|
On a related note, it seems in commit aacb928 the new code in |
|
Thanks for the feedback. I'll look at this. |
I agree that would be nice, but I wasn't sure if I could expect a C99-compliant compiler. |
|
libpcap started requiring C99 in release 1.10.0 a few years ago. |
3 participants
As discussed in #1473, to avoid namespace collision and confusion, use separate namespaces for Npcap's and Linux's BPF extensions. In gencode.c, known-good tested code continues to use SKF_AD_ for both, but a comment and conditional definition of those constants should make things clearer.