Prefer NPCAP_AD prefix for Npcap over SKF_AD prefix #1478
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
| @@ -82,7 +82,8 @@ enum { | |||
| * | |||
| * Thanks to Ani Sinha <[email protected]> for providing initial implementation | |||
| */ | |||
| #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.
| @@ -9418,7 +9425,7 @@ gen_vlan(compiler_state_t *cstate, bpf_u_int32 vlan_num, int has_vlan_tag) | |||
| * | |||
| * 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__).
| #ifdef SKF_AD_VLAN_TAG | ||
| #ifdef NPCAP_AD_VLAN_TAG | ||
| [NPCAP_AD_VLAN_TAG] = "vlan_tci", | ||
| #elif defined(SKF_AD_VLAN_TAG) |
There was a problem hiding this comment.
If at most one of {NPCAP_AD_VLAN_TAG, SKF_AD_VLAN_TAG} is defined in any one build, using #elif defined(NPCAP_AD_VLAN_TAG) would keep the diff cleaner.
There was a problem hiding this comment.
As mentioned above, both may be defined currently for a Npcap build. That is why the NPCAP_AD_* check must come first. However, we have not made a SDK release for that yet, so it is still open to change.
There was a problem hiding this comment.
Thank you, let me check again.
|
Thank you, this generally makes sense, there is space for potential minor clean-ups as commented. |
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.