Skip to content
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

Display interface on live captures on DLT_LINUX_SLL2 #689

Merged
merged 1 commit into from Jul 19, 2018

Conversation

@pevik
Copy link
Contributor

pevik commented Jul 12, 2018

The reason why displaying interface only for live captures is different
index when display pcap file on another machine than the one which was
doing the capture on [1].

See: GH the-tcpdump-group/libpcap#127

[1] https://lists.sandelman.ca/pipermail/tcpdump-workers/2018-July/001019.html

Signed-off-by: Petr Vorel pvorel@suse.cz
Suggested-by: Guy Harris guy@alum.mit.edu

@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch 5 times, most recently from 1f30387 to a87cc11 Jul 12, 2018
print-sll.c Outdated
#ifdef HAVE_NET_IF_H
if (ndo->ndo_live_capture && if_indextoname(EXTRACT_BE_U_4(sllp->sll2_if_index), ifname))
ND_PRINT("IFNAME %s ", ifname);
#endif

This comment has been minimized.

Copy link
@infrastation

infrastation Jul 13, 2018

Member

Most of the time the call will be successful. But when the packet reaches tcpdump after the interface was deleted (which is more likely with low packet rate and TPACKET_V3, which can buffer packets long enough), the ifindex at this point will refer to a non-existent interface:

  On success, if_indextoname() returns ifname; on error, NULL is returned
  and errno is set appropriately.

This comment has been minimized.

Copy link
@guyharris

guyharris Jul 13, 2018

Member

There's not much you can do about that.

But should it print something such as "IFINDEX %u" if if_indextoname() returns NULL?

This comment has been minimized.

Copy link
@infrastation

infrastation Jul 13, 2018

Member

Maybe "ifindex %u (%s)" on success and just "ifindex %u" on failure?

This comment has been minimized.

Copy link
@pevik

pevik Jul 13, 2018

Author Contributor

Changed it to suggested "IFNAME %s (%u) " on success on live capture otherwise "IFNAME %u ".

Do we want to show "IFNAME %s (%u) " also while reading packet? Not sure about it (on the same localhost it's a benefit, but can be misleading).

This comment has been minimized.

Copy link
@pevik

pevik Jul 13, 2018

Author Contributor

When I'm thinking about it, printing always interface name looks better for me. I'll remove ndo_live_capture and give a warning when not live capture.

@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch 3 times, most recently from c1b8ac8 to 97930af Jul 13, 2018
@pevik

This comment has been minimized.

Copy link
Contributor Author

pevik commented Jul 13, 2018

Implemented previous suggestion.

@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch 2 times, most recently from 080b03c to e6c5b79 Jul 16, 2018
@pevik

This comment has been minimized.

Copy link
Contributor Author

pevik commented Jul 18, 2018

@guyharris @infrastation this is a shy ping. Any objections or can it be merged?

@guyharris

This comment has been minimized.

Copy link
Member

guyharris commented Jul 18, 2018

Fine with me. Anybody else?

@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch from e6c5b79 to 99828e4 Jul 18, 2018
@pevik

This comment has been minimized.

Copy link
Contributor Author

pevik commented Jul 18, 2018

I've just removed first commit with updating configure. It was wrong (what autoconf adds autoheader removes + recent commit abe1aa5 shows that it wasn't needed).
Hope me or someone else implement removing configure from git according to Guy's instructions (https://lists.sandelman.ca/pipermail/tcpdump-workers/2018-July/001000.html)

@infrastation

This comment has been minimized.

Copy link
Member

infrastation commented Jul 18, 2018

Errrm, I would say something like "ifindex %u" and "ifindex %u (%s)" would be easier to parse if anyone needs to, rather than the format in the proposed commit.

@pevik

This comment has been minimized.

Copy link
Contributor Author

pevik commented Jul 18, 2018

@infrastation I got your point, reverse it as ifindex is for sure, but ifname not. Make sense, I'll do it. I'll just use upper case (as for other) + keep the warning.

@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch from 99828e4 to 13c3538 Jul 18, 2018
@pevik

This comment has been minimized.

Copy link
Contributor Author

pevik commented Jul 18, 2018

@guyharris @infrastation OK, at the end I used output suggested by Denis:
"ifindex %u (%s) " and "ifindex %u ".

@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch from 13c3538 to 558c80a Jul 18, 2018
Index is displayed always, name only if available.

Warn about possible wrong interfaces when in reading mode
(pcap file can be displayed on a different host then where
was captured) [1].

See: GH the-tcpdump-group/libpcap#127

[1] https://lists.sandelman.ca/pipermail/tcpdump-workers/2018-July/001019.html

Signed-off-by: Petr Vorel <pvorel@suse.cz>
Suggested-by: Guy Harris <guy@alum.mit.edu>
Reviewed-by: Denis Ovsienko <denis@ovsienko.info>
Reviewed-by: Guy Harris <guy@alum.mit.edu>
@pevik pevik force-pushed the pevik:DLT_LINUX_SLL2-gh.127 branch from 558c80a to 6e36c61 Jul 18, 2018
@infrastation infrastation merged commit 152acc2 into the-tcpdump-group:master Jul 19, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@infrastation

This comment has been minimized.

Copy link
Member

infrastation commented Jul 19, 2018

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.