-
Notifications
You must be signed in to change notification settings - Fork 26
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
Implement option retis pcap --list-probes #504
base: main
Are you sure you want to change the base?
Conversation
6b9e081
to
d9e12f6
Compare
eef4472
to
78a8393
Compare
The Fedora Rawhide fail is still due to upstream issues, right? |
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.
Thanks for working on this, and another thanks for adding multiple tests that is very much appreciated!
A few comments inline. Also could you split the added runtime CI test from the pcap
argument addition in separate commits? (If that's not too hard to do, otherwise that's fine).
retis/src/process/cli/pcap.rs
Outdated
let probe = self | ||
.probe | ||
.as_ref() | ||
.ok_or_else(|| anyhow!("Probe cannot be empty"))?; |
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.
The issue with doing that manually is retis pcap
alone was displaying the help when the arguments were not the expected ones while now it only displays a small error message, which is in addition only about one of the two ways of using retis pcap
.
There are two ways forward AFAIK:
- Using
ArgGroup
. - Or calling manually the help displaying function on error.
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.
Done, but I still couldn't get it exactly as I want it ..
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap -l
raw_tracepoint:net:net_dev_start_xmit
raw_tracepoint:net:netif_receive_skb
raw_tracepoint:skb:kfree_skb
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap -p a
M<+��������Error: Probe not found in the events
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap -p a -o a
Error: Probe not found in the events
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap -l -o a
error: the argument '--list-probes' cannot be used with '--out <OUT>'
Usage: retis pcap <--list-probes|--probe <PROBE>> [INPUT]
For more information, try '--help'.
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap -o a -l
error: the argument '--out <OUT>' cannot be used with '--list-probes'
Usage: retis pcap --out <OUT> <--list-probes|--probe <PROBE>> [INPUT]
For more information, try '--help'.
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap
error: the following required arguments were not provided:
<--list-probes|--probe <PROBE>>
Usage: retis pcap <--list-probes|--probe <PROBE>> [INPUT]
For more information, try '--help'.
[akaris@workstation retis (list-probes)]$ target/debug/retis pcap -h
Generate a PCAP file from stored events
Usage: retis pcap [OPTIONS] <--list-probes|--probe <PROBE>> [INPUT]
Arguments:
[INPUT] File from which to read events [default: retis.data]
Options:
-l, --list-probes List probes that are available in the data file
-p, --probe <PROBE> Filter events from this probe. Probes should follow the [TYPE:]TARGET pattern.
See `retis collect --help` for more details on the probe format.
-o, --out <OUT> Write the generated PCAP output to a file rather than stdout
-h, --help Print help
I've been looking into a bunch of stuff, such as:
- implementing (default) subcommands, but that didn't work for me, and even if I got it to work, the help output would prob still not look the way I'd want it
- override_usage (as a derive macro) would be the easiest way of achieving nice help output, but one would have to hardcode the binary name >_>
- override_usage (not as a derive macro) might be another way, but that would be down the rabbit hole of reimplementing the traits and that's overkill ...?
What disturbs me is this here:
Usage: retis pcap --out <OUT> <--list-probes|--probe <PROBE>> [INPUT]
when -o ... -l
is provided. Perhaps I'm just being too pedantic ..
retis/src/process/cli/pcap.rs
Outdated
if let Some(kernel) = event.get_section::<KernelEvent>(SectionId::Kernel) { | ||
// Insert the full probe_type:symbol. | ||
probe_set.insert(format!("{}:{}", kernel.probe_type, kernel.symbol)); | ||
} |
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.
We should also make sure a raw packet is available when listing a compatible probe here, otherwise some of them might fail later.
With this in mind this should also support UserEvent
, even though it should fail for now at finding associated raw packet in the same events.
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.
I'll address this in my next pass.
Also, pcap --probe <...> --out <...>
actually creates a file even when it cannot find any matching data. I see a few ways to fix this:
- remove file if no matching data found
- write to file in /tmp first, then if something is found, move the file to the --out location
- write to buffer and flush the buffer at regular intervals to the file or simply write everything to memory and write to file at the end
- parse the dataset first, check if probe in in dataset, then start the write logic
Which one do you prefer?
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.
ok, deferring is not possible, as
let mut writer = PcapNgWriter::new(
immediately writes to the file in:
72 /// Creates a new [`PcapNgWriter`] from an existing writer with the given section header.
73 pub fn with_section_header(mut writer: W, section: SectionHeaderBlock<'static>) -> PcapResult<Self> {
74 match section.endianness {
75 Endianness::Big => section.clone().into_block().write_to::<BigEndian, _>(&mut writer).map_err(PcapError::IoError)?,
76 Endianness::Little => section.clone().into_block().write_to::<LittleEndian, _>(&mut writer).map_err(PcapError::IoError)?,
77 };
Given that the same issue also happens with stdout, the only way would thus be to not create PcapNgWriter::new
until after all the events were parsed
Also please rebase as this conflicts with the recent cli PR being merged. Thanks! |
2fd849a
to
88879db
Compare
Signed-off-by: Andreas Karis <[email protected]>
PcapNgWriter::new will immediately write the section header to the provided writer. If anything failed during handle_events, garbage output was printed to stdout and/or the output file was written with garbage data. Instead, invert the logic and collect all Pcap Blocks in a Vector. Create PcapNgWriter only if handle_events passes without an error. Signed-off-by: Andreas Karis <[email protected]>
With the new option --list-probes, retis pcap can now list available probes. Signed-off-by: Andreas Karis <[email protected]>
Signed-off-by: Andreas Karis <[email protected]>
88879db
to
315564e
Compare
Signed-off-by: Andreas Karis <[email protected]>
315564e
to
937bbaf
Compare
With the new option --list-probes, retis pcap can now list available
probes.