cleanup: make rayon optional

A modest performance gain of ~5% is achieved by avoiding some copies and
indirection.

.github/workflows/commit.yml

@@ -16,7 +16,7 @@ jobs:
 
     steps:
     - uses: actions/checkout@v3
-    - name: Build
+    - name: Build tests
-      run: cargo build --verbose
+      run: cargo test --no-run --verbose
     - name: Run tests
-      run: cargo test --verbose
+      run: cargo test

.github/workflows/release.yml

@@ -16,18 +16,20 @@ jobs:
           - target: x86_64-pc-windows-gnu
             archive: zip
           - target: x86_64-unknown-linux-musl
-            archive: tar.gz tar.xz tar.zst
+            archive: tar.gz
           - target: x86_64-apple-darwin
             archive: zip
     steps:
       - uses: actions/checkout@master
       - name: Compile and release
+        id: compile
         uses: rust-build/rust-build.action@v1.4.3
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         with:
           RUSTTARGET: ${{ matrix.target }}
           ARCHIVE_TYPES: ${{ matrix.archive }}
+          TOOLCHAIN_VERSION: stable
       - name: Upload artifact
         uses: actions/upload-artifact@v3
         with:

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "rs-aggregate"
-version = "0.2.0"
+version = "0.3.2"
 authors = ["Keenan Tims <ktims@gotroot.ca>"]
 edition = "2021"
 description = "Aggregate a list of IP prefixes into their minimum equivalent representation"
@@ -8,12 +8,16 @@ readme = "README.md"
 repository = "https://github.com/ktims/rs-aggregate"
 license = "MIT"
 categories = ["network-programming"]
+exclude = [".github/*", "doc/*", "test-data/*"]
+
+[features]
+default = ["rayon"]
 
 [dependencies]
-clap = { version = "4.1.8", features = ["derive"] }
+clap = { version = "4.4.6", features = ["derive"] }
-clio = { version = "0.2.7", features = ["clap-parse"] }
+clio = { version = "0.3.4", features = ["clap-parse"] }
-ipnet = "2.7.1"
+ipnet = "2.8.0"
-iprange = "0.6.7"
+rayon = { version = "1.8.0", optional = true }
 
 [dev-dependencies]
 assert_cmd = "2.0.10"
@@ -21,6 +25,15 @@ assert_fs = "1.0.12"
 predicates = "3.0.1"
 rstest = "0.16.0"
 glob = "0.3.1"
+tempfile = "3.8.1"
+json = "0.12.4"
+plotters = "0.3.5"
+rand_chacha = "0.3.1"
+rand = "0.8.5"
 
 [[bin]]
 name = "rs-aggregate"
+
+[[bench]]
+name = "perf"
+harness = false

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2023 Keenan Tims
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.

README.md

@@ -5,20 +5,44 @@ Intended to be a drop-in replacement for [aggregate6](https://github.com/job/agg
 
 Takes a list of whitespace-separated IPs or IP networks and aggregates them to their minimal representation.
 
-## Known discrepancies with `aggregate6`
+## Installation
 
-* `rs-aggregate` accepts subnet and wilcard mask formats in addition to CIDR, ie all these are valid and equivalent:
+`rs-aggregate` is built statically. CI-built binaries can be found in the GitHub
-  * `1.1.1.0/255.255.255.0`
+releases for most common platforms. Simply download the appropriate binary and
-  * `1.1.1.0/0.0.0.255`
+place it in your path.
-  * `1.1.1.0/24`
+
-* `-m/--max-prefixlen` supports different maximums for each address family as ipv4,ipv6 format
+It can also be installed via some software management tools:
+
+### FreeBSD
+```
+pkg install rs-aggregate
+```
+
+### Cargo
+```
+cargo install rs-aggregate
+```
+
+
+## Known differences from `aggregate6`
+
+* `-m/--max-prefixlen` supports different maximums for each address family as
+  ipv4,ipv6 format. A single value is also supported and has the same behaviour
+  as `aggregate6` (apply the same maximum to both address families).
+* `-v` verbose dump is not supported
+* Truncation errors (when host bits are set without the `-t` flag) are printed
+  based on the parsed address, ie. always in CIDR format, whereas `aggregate6`
+  prints errors based on the input.
 
 ## Performance
 
 Performance comparison of `rs-aggregate` vs `aggregate6`. A speedup of >100x is achieved on DFZ data.
 
-Full DFZ (1154968 total, 202729 aggregates):
+### Full DFZ (1154968 total, 202729 aggregates):
 ![dfz perf comparison](doc/perfcomp_all.png)
 
-IPv4 DFZ (968520 total, 154061 aggregates):
+### IPv4 DFZ (968520 total, 154061 aggregates):
 ![ipv4 dfz perf comparison](doc/perfcomp_v4.png)
+
+### 1024 random prefixes (startup time):
+![startup time comparison](doc/perfcomp_startup.png)

benches/perf.rs

@@ -0,0 +1,282 @@
+use ipnet::Ipv4Net;
+use json::JsonValue;
+use plotters::backend::BitMapBackend;
+use plotters::chart::ChartBuilder;
+use plotters::coord::ranged1d::{IntoSegmentedCoord, SegmentValue};
+use plotters::drawing::IntoDrawingArea;
+use plotters::element::{EmptyElement, Text};
+use plotters::series::{Histogram, PointSeries};
+use plotters::style::full_palette::GREY;
+use plotters::style::text_anchor::{HPos, Pos, VPos};
+use plotters::style::{Color, IntoFont, RGBColor, ShapeStyle, BLACK, WHITE};
+use rand::prelude::*;
+use rand_chacha::ChaChaRng;
+use std::ffi::OsStr;
+use std::io::{Read, Write};
+
+use std::process::Stdio;
+use tempfile::NamedTempFile;
+
+const BAR_COLOUR: RGBColor = RGBColor(66, 133, 244);
+
+#[derive(Clone, Debug)]
+struct TestDefinition {
+    cmd: String,
+    name: String, // including version
+}
+
+#[derive(Clone, Debug)]
+struct TestResult {
+    mean: f64,
+    stddev: f64,
+    median: f64,
+    min: f64,
+    max: f64,
+}
+
+impl From<JsonValue> for TestResult {
+    fn from(value: JsonValue) -> Self {
+        Self {
+            mean: value["mean"].as_f64().unwrap(),
+            stddev: value["stddev"].as_f64().unwrap(),
+            median: value["median"].as_f64().unwrap(),
+            min: value["min"].as_f64().unwrap(),
+            max: value["max"].as_f64().unwrap(),
+        }
+    }
+}
+
+fn make_tests(input_path: &str) -> Vec<TestDefinition> {
+    let our_version = format!("rs-aggregate {}", env!("CARGO_PKG_VERSION"));
+    let our_path = env!("CARGO_BIN_EXE_rs-aggregate");
+
+    let python_version_raw = std::process::Command::new("python3")
+        .arg("--version")
+        .stdout(Stdio::piped())
+        .spawn()
+        .expect("Unable to run python3")
+        .wait_with_output()
+        .expect("Couldn't get python3 output")
+        .stdout;
+    let python_version = String::from_utf8_lossy(&python_version_raw);
+
+    let agg6_version_raw = std::process::Command::new("python3")
+        .arg("-m")
+        .arg("aggregate6")
+        .arg("-V")
+        .stdout(Stdio::piped())
+        .spawn()
+        .expect("Unable to run aggregate6")
+        .wait_with_output()
+        .expect("Couldn't get aggregate6 output")
+        .stdout;
+    let agg6_version = String::from_utf8_lossy(&agg6_version_raw);
+
+    vec![
+        TestDefinition {
+            cmd: format!("{} {}", our_path, input_path),
+            name: our_version.into(),
+        },
+        TestDefinition {
+            cmd: format!("python3 -m aggregate6 {}", input_path),
+            name: format!("{} ({})", agg6_version.trim(), python_version.trim()),
+        },
+    ]
+}
+
+fn make_v4_tests(input_path: &str) -> Vec<TestDefinition> {
+    let mut all_tests = make_tests(input_path);
+
+    let iprange_version_raw = std::process::Command::new("iprange")
+        .arg("--version")
+        .stdout(Stdio::piped())
+        .spawn()
+        .expect("Unable to run iprange")
+        .wait_with_output()
+        .expect("Couldn't get iprange output")
+        .stdout;
+    let iprange_version = String::from_utf8_lossy(&iprange_version_raw);
+
+    all_tests.push(TestDefinition {
+        cmd: format!("iprange --optimize {}", input_path),
+        name: iprange_version.lines().nth(0).unwrap().into(),
+    });
+
+    all_tests
+}
+
+// We don't really care if aggregation will actually be possible, but we'll only
+// generate prefixes with length 8->24 so some should be possible.
+fn make_random_prefix(rng: &mut impl Rng) -> Ipv4Net {
+    let prefix_len: u8 = rng.gen_range(8..25);
+    let netaddr: u32 = rng.gen_range(0..(1 << prefix_len)) << 32 - prefix_len;
+
+    Ipv4Net::new(netaddr.into(), prefix_len).unwrap()
+}
+
+// Generate 1024 random v4 addresses as a startup time test
+fn make_startup_tests() -> (NamedTempFile, Vec<TestDefinition>) {
+    let mut rng = ChaChaRng::seed_from_u64(0); // use a repeatable rng with custom seed
+    let addresses = std::iter::repeat_with(|| make_random_prefix(&mut rng)).take(1024);
+
+    let mut outfile = NamedTempFile::new().unwrap();
+    let mut outfile_f = outfile.as_file();
+    for addr in addresses {
+        outfile_f.write_fmt(format_args!("{}\n", addr)).unwrap();
+    }
+    outfile.flush().unwrap();
+
+    let outpath = outfile.path().as_os_str().to_string_lossy().to_string();
+
+    // outfile needs to live on so destructor doesn't delete it before we run the benches
+    (outfile, make_v4_tests(outpath.as_str()))
+}
+
+fn hyperfine_harness<S>(cmd: S) -> Result<TestResult, Box<dyn std::error::Error>>
+where
+    S: AsRef<OsStr>,
+{
+    let resultfile = NamedTempFile::new().expect("Unable to create tempfile");
+
+    let mut process = std::process::Command::new("hyperfine")
+        .arg("--export-json")
+        .arg(resultfile.path())
+        .arg("--min-runs")
+        .arg("10")
+        .arg("-N")
+        .arg("--")
+        .arg(&cmd)
+        .stdout(Stdio::null())
+        .spawn()
+        .expect("unable to run command");
+    let _rc = process.wait().expect("unable to wait on process");
+
+    let mut raw_result_buf = Vec::new();
+    resultfile
+        .as_file()
+        .read_to_end(&mut raw_result_buf)
+        .expect("Can't read results");
+    resultfile.close().unwrap();
+
+    let hf_result = json::parse(&String::from_utf8_lossy(&raw_result_buf)).expect(
+        format!(
+            "Can't parse hyperfine json results from command `{}`",
+            cmd.as_ref().to_string_lossy()
+        )
+        .as_str(),
+    );
+
+    let final_result = &hf_result["results"][0];
+
+    Ok((final_result.clone()).into())
+}
+
+fn plot_results(
+    results: &Vec<(TestDefinition, TestResult)>,
+    caption: &str,
+    outfile: &str,
+) -> Result<(), Box<dyn std::error::Error>> {
+    // Second result is our baseline
+    let norm_numerator = results[1].1.mean;
+    let max_result = norm_numerator / results.iter().map(|x| x.1.mean).reduce(f64::min).unwrap();
+
+    let drawing = BitMapBackend::new(outfile, (640, 480)).into_drawing_area();
+    drawing.fill(&WHITE)?;
+    let mut chart = ChartBuilder::on(&drawing)
+        .x_label_area_size(40)
+        .y_label_area_size(40)
+        .caption(caption, ("Roboto", 24).into_font())
+        .build_cartesian_2d((0..results.len() - 1).into_segmented(), 0.0..max_result)?;
+
+    chart
+        .configure_mesh()
+        .y_desc("Speedup vs aggregate6")
+        .y_labels(5)
+        .y_label_formatter(&|x| std::fmt::format(format_args!("{:.0}", *x)))
+        .light_line_style(WHITE)
+        .bold_line_style(GREY)
+        .disable_x_mesh()
+        .x_label_style(("Roboto", 18).into_font())
+        .x_label_formatter(&|x| match x {
+            SegmentValue::Exact(val) => results[*val].0.name.clone(),
+            SegmentValue::CenterOf(val) => results[*val].0.name.clone(),
+            SegmentValue::Last => String::new(),
+        })
+        .draw()?;
+
+    chart.draw_series(
+        Histogram::vertical(&chart)
+            .style(BAR_COLOUR.filled())
+            .margin(10)
+            .data(
+                results
+                    .iter()
+                    .enumerate()
+                    .map(|(x, y)| (x, norm_numerator / y.1.mean)),
+            ),
+    )?;
+
+    chart.draw_series(PointSeries::of_element(
+        results
+            .iter()
+            .enumerate()
+            .map(|(x, y)| (SegmentValue::CenterOf(x), norm_numerator / y.1.mean)),
+        5,
+        ShapeStyle::from(&BLACK).filled(),
+        &|coord, _size, _style| {
+            let (target_y, target_colour) = if coord.1 < 25.0 {
+                (-25, BAR_COLOUR)
+            } else {
+                (25, WHITE)
+            };
+            EmptyElement::at(coord.clone())
+                + Text::new(
+                    format!("{:.1} x", coord.1),
+                    (0, target_y),
+                    ("Roboto", 18)
+                        .into_font()
+                        .color(&target_colour)
+                        .pos(Pos::new(HPos::Center, VPos::Center)),
+                )
+        },
+    ))?;
+
+    Ok(())
+}
+
+fn main() -> Result<(), Box<dyn std::error::Error>> {
+    run_and_plot(
+        make_tests("test-data/dfz_combined/input"),
+        "doc/perfcomp_all.png",
+        "IPv4 & IPv6 Full DFZ",
+    )?;
+    run_and_plot(
+        make_v4_tests("test-data/dfz_v4/input"),
+        "doc/perfcomp_v4.png",
+        "IPv4 Full DFZ",
+    )?;
+
+    // Need to hold on to tmpfile so it doesn't get deleted before we can bench
+    let (_tmpfile, tests) = make_startup_tests();
+    run_and_plot(
+        tests,
+        "doc/perfcomp_startup.png",
+        "1024 Random IPv4 Prefixes",
+    )?;
+
+    Ok(())
+}
+
+fn run_and_plot(
+    tests: Vec<TestDefinition>,
+    filename: &str,
+    caption: &str,
+) -> Result<(), Box<dyn std::error::Error>> {
+    let mut results: Vec<(TestDefinition, TestResult)> = Vec::new();
+    for test in tests {
+        println!("Running bench: {:?}", test);
+        results.push((test.clone(), hyperfine_harness(&test.cmd)?));
+    }
+    plot_results(&results, caption, filename)?;
+    Ok(())
+}

src/iputils.rs

@@ -1,3 +1,5 @@
+#[cfg(feature = "rayon")]
+use rayon::join;
 use std::{
     error::Error,
     fmt::Display,
@@ -6,12 +8,11 @@ use std::{
 };
 
 use ipnet::{IpNet, Ipv4Net, Ipv6Net};
-use iprange::{IpRange, IpRangeIter};
 
 #[derive(Default)]
 pub struct IpBothRange {
-    v4: IpRange<Ipv4Net>,
+    v4: Vec<Ipv4Net>,
-    v6: IpRange<Ipv6Net>,
+    v6: Vec<Ipv6Net>,
 }
 
 impl IpBothRange {
@@ -19,49 +20,66 @@ impl IpBothRange {
         IpBothRange::default()
     }
     pub fn add(&mut self, net: IpOrNet) {
-        match net.net {
+        match net.0 {
-            IpNet::V4(v4_net) => drop(self.v4.add(v4_net)),
+            IpNet::V4(n) => self.v4.push(n),
-            IpNet::V6(v6_net) => drop(self.v6.add(v6_net)),
+            IpNet::V6(n) => self.v6.push(n),
         }
     }
+    #[cfg(feature = "rayon")]
     pub fn simplify(&mut self) {
-        self.v4.simplify();
+        (self.v4, self.v6) = join(
-        self.v6.simplify();
+            || Ipv4Net::aggregate(&self.v4),
+            || Ipv6Net::aggregate(&self.v6),
+        );
+    }
+    #[cfg(not(feature = "rayon"))]
+    pub fn simplify(&mut self) {
+        self.v4 = Ipv4Net::aggregate(&self.v4);
+        self.v6 = Ipv6Net::aggregate(&self.v6);
     }
 
-    pub fn v4_iter(&self) -> IpRangeIter<Ipv4Net> {
+    pub fn v4_iter(&self) -> impl Iterator<Item = &Ipv4Net> {
         self.v4.iter()
     }
 
-    pub fn v6_iter(&self) -> IpRangeIter<Ipv6Net> {
+    pub fn v6_iter(&self) -> impl Iterator<Item = &Ipv6Net> {
         self.v6.iter()
     }
 }
 
+impl Display for IpBothRange {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        for ip in self {
+            ip.fmt(f)?;
+            writeln!(f)?;
+        }
+        Ok(())
+    }
+}
+
 pub struct IpBothRangeIter<'a> {
-    v4_iter: IpRangeIter<'a, Ipv4Net>,
+    ranges: &'a IpBothRange,
-    v6_iter: IpRangeIter<'a, Ipv6Net>,
+    v4done: bool,
-    _v4_done: bool,
+    pos: usize,
 }
 
 impl<'a> Iterator for IpBothRangeIter<'a> {
     type Item = IpNet;
     fn next(&mut self) -> Option<Self::Item> {
-        if self._v4_done {
+        if !self.v4done {
-            match self.v6_iter.next() {
+            if self.pos < self.ranges.v4.len() {
-                Some(net) => return Some(net.into()),
+                self.pos += 1;
-                None => return None,
+                Some(IpNet::V4(self.ranges.v4[self.pos - 1]))
-            }
+            } else {
-        }
+                self.v4done = true;
-        match self.v4_iter.next() {
+                self.pos = 0;
-            Some(net) => Some(net.into()),
+                self.next()
-            None => {
-                self._v4_done = true;
-                match self.v6_iter.next() {
-                    Some(net) => Some(net.into()),
-                    None => None,
-                }
             }
+        } else if self.pos < self.ranges.v6.len() {
+            self.pos += 1;
+            Some(IpNet::V6(self.ranges.v6[self.pos - 1]))
+        } else {
+            None
         }
     }
 }
@@ -71,22 +89,20 @@ impl<'a> IntoIterator for &'a IpBothRange {
     type IntoIter = IpBothRangeIter<'a>;
     fn into_iter(self) -> Self::IntoIter {
         IpBothRangeIter {
-            v4_iter: self.v4.iter(),
+            ranges: self,
-            v6_iter: self.v6.iter(),
+            v4done: false,
-            _v4_done: false,
+            pos: 0,
         }
     }
 }
 
 #[derive(Debug, PartialEq)]
-pub struct IpOrNet {
+pub struct IpOrNet(IpNet);
-    pub net: IpNet,
-}
 
 #[derive(Debug, Clone)]
 pub struct NetParseError {
     #[allow(dead_code)]
-    msg: String,
+    msg: &'static str,
 }
 
 impl Display for NetParseError {
@@ -102,9 +118,7 @@ impl IpOrNet {
     //   netmask - 1.1.1.0/255.255.255.0
     //   wildcard mask - 1.1.1.0/0.0.0.255
     fn parse_mask(p: &str) -> Result<u8, Box<dyn Error>> {
-        let mask = p.parse::<Ipv4Addr>();
+        let mask = p.parse::<Ipv4Addr>()?;
-        match mask {
-            Ok(mask) => {
         let intrep: u32 = mask.into();
         let lead_ones = intrep.leading_ones();
         if lead_ones > 0 {
@@ -112,7 +126,7 @@ impl IpOrNet {
                 Ok(lead_ones.try_into()?)
             } else {
                 Err(Box::new(NetParseError {
-                            msg: "Invalid subnet mask".to_owned(),
+                    msg: "Invalid subnet mask",
                 }))
             }
         } else {
@@ -121,14 +135,12 @@ impl IpOrNet {
                 Ok(lead_zeros.try_into()?)
             } else {
                 Err(Box::new(NetParseError {
-                            msg: "Invalid wildcard mask".to_owned(),
+                    msg: "Invalid wildcard mask",
                 }))
             }
         }
     }
-            Err(e) => Err(Box::new(e)),
+
-        }
-    }
     fn from_parts(ip: &str, pfxlen: &str) -> Result<Self, Box<dyn Error>> {
         let ip = ip.parse::<IpAddr>()?;
         let pfxlenp = pfxlen.parse::<u8>();
@@ -140,20 +152,35 @@ impl IpOrNet {
                     Ok(IpNet::new(ip, IpOrNet::parse_mask(pfxlen)?)?.into())
                 } else {
                     Err(Box::new(NetParseError {
-                        msg: "Mask form is not valid for IPv6 address".to_owned(),
+                        msg: "Mask form is not valid for IPv6 address",
                     }))
                 }
             }
         }
     }
     pub fn prefix_len(&self) -> u8 {
-        self.net.prefix_len()
+        self.0.prefix_len()
     }
     pub fn is_ipv4(&self) -> bool {
-        self.net.network().is_ipv4()
+        match self.0 {
+            IpNet::V4(_) => true,
+            IpNet::V6(_) => false,
+        }
     }
     pub fn is_ipv6(&self) -> bool {
-        self.net.network().is_ipv6()
+        match self.0 {
+            IpNet::V4(_) => false,
+            IpNet::V6(_) => true,
+        }
+    }
+    pub fn addr(&self) -> IpAddr {
+        self.0.addr()
+    }
+    pub fn network(&self) -> IpAddr {
+        self.0.network()
+    }
+    pub fn has_host_bits(&self) -> bool {
+        self.0.addr() != self.0.network()
     }
 }
 
@@ -170,47 +197,43 @@ impl FromStr for IpOrNet {
 
 impl Display for IpOrNet {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        self.net.fmt(f)
+        self.0.fmt(f)
     }
 }
 
 impl From<IpNet> for IpOrNet {
     fn from(net: IpNet) -> Self {
-        IpOrNet { net }
+        IpOrNet(net)
     }
 }
 
 impl From<IpAddr> for IpOrNet {
     fn from(addr: IpAddr) -> Self {
-        IpOrNet { net: addr.into() }
+        IpOrNet(addr.into())
     }
 }
 
 impl From<Ipv4Net> for IpOrNet {
     fn from(net: Ipv4Net) -> Self {
-        IpOrNet { net: net.into() }
+        IpOrNet(net.into())
     }
 }
 
 impl From<Ipv6Net> for IpOrNet {
     fn from(net: Ipv6Net) -> Self {
-        IpOrNet { net: net.into() }
+        IpOrNet(net.into())
     }
 }
 
 impl From<Ipv4Addr> for IpOrNet {
     fn from(addr: Ipv4Addr) -> Self {
-        IpOrNet {
+        IpOrNet(IpAddr::from(addr).into())
-            net: IpAddr::from(addr).into(),
-        }
     }
 }
 
 impl From<Ipv6Addr> for IpOrNet {
     fn from(addr: Ipv6Addr) -> Self {
-        IpOrNet {
+        IpOrNet(IpAddr::from(addr).into())
-            net: IpAddr::from(addr).into(),
-        }
     }
 }
 
@@ -228,13 +251,13 @@ impl Default for PrefixlenPair {
 
 impl Display for PrefixlenPair {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str(format!("{},{}", self.v4, self.v6).as_str())
+        f.write_fmt(format_args!("{},{}", self.v4, self.v6))
     }
 }
 
 impl PartialEq<IpOrNet> for PrefixlenPair {
     fn eq(&self, other: &IpOrNet) -> bool {
-        match other.net {
+        match other.0 {
             IpNet::V4(net) => self.v4 == net.prefix_len(),
             IpNet::V6(net) => self.v6 == net.prefix_len(),
         }
@@ -249,31 +272,31 @@ impl PartialEq<PrefixlenPair> for PrefixlenPair {
 
 impl PartialOrd<IpOrNet> for PrefixlenPair {
     fn ge(&self, other: &IpOrNet) -> bool {
-        match other.net {
+        match other.0 {
             IpNet::V4(net) => self.v4 >= net.prefix_len(),
             IpNet::V6(net) => self.v6 >= net.prefix_len(),
         }
     }
     fn gt(&self, other: &IpOrNet) -> bool {
-        match other.net {
+        match other.0 {
             IpNet::V4(net) => self.v4 > net.prefix_len(),
             IpNet::V6(net) => self.v6 > net.prefix_len(),
         }
     }
     fn le(&self, other: &IpOrNet) -> bool {
-        match other.net {
+        match other.0 {
             IpNet::V4(net) => self.v4 <= net.prefix_len(),
             IpNet::V6(net) => self.v6 <= net.prefix_len(),
         }
     }
     fn lt(&self, other: &IpOrNet) -> bool {
-        match other.net {
+        match other.0 {
             IpNet::V4(net) => self.v4 < net.prefix_len(),
             IpNet::V6(net) => self.v6 < net.prefix_len(),
         }
     }
     fn partial_cmp(&self, other: &IpOrNet) -> Option<std::cmp::Ordering> {
-        match other.net {
+        match other.0 {
             IpNet::V4(net) => self.v4.partial_cmp(&net.prefix_len()),
             IpNet::V6(net) => self.v6.partial_cmp(&net.prefix_len()),
         }
@@ -282,12 +305,12 @@ impl PartialOrd<IpOrNet> for PrefixlenPair {
 
 #[derive(Debug)]
 pub struct ParsePrefixlenError {
-    msg: String,
+    msg: &'static str,
 }
 
 impl Display for ParsePrefixlenError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str(self.msg.as_str())
+        f.write_str(self.msg)
     }
 }
 
@@ -299,25 +322,25 @@ impl FromStr for PrefixlenPair {
         match s.split_once(',') {
             Some(pair) => {
                 let v4 = u8::from_str(pair.0).or(Err(ParsePrefixlenError {
-                    msg: "Unable to parse integer".to_owned(),
+                    msg: "Unable to parse integer",
                 }))?;
                 let v6 = u8::from_str(pair.1).or(Err(ParsePrefixlenError {
-                    msg: "Unable to parse integer".to_owned(),
+                    msg: "Unable to parse integer",
                 }))?;
                 if v4 > 32 || v6 > 128 {
                     return Err(ParsePrefixlenError {
-                        msg: "Invalid prefix length".to_owned(),
+                        msg: "Invalid prefix length",
                     });
                 }
                 Ok(PrefixlenPair { v4, v6 })
             }
             None => {
                 let len = u8::from_str(s).or(Err(ParsePrefixlenError {
-                    msg: "Unable to parse integer".to_owned(),
+                    msg: "Unable to parse integer",
                 }))?;
                 if len > 128 {
                     return Err(ParsePrefixlenError {
-                        msg: "Invalid prefix length".to_owned(),
+                        msg: "Invalid prefix length",
                     });
                 }
                 Ok(PrefixlenPair { v4: len, v6: len })

src/main.rs

@@ -1,41 +1,33 @@
 extern crate ipnet;
-extern crate iprange;
+
+use std::{io, process::exit};
 
 mod iputils;
 use iputils::{IpBothRange, IpOrNet, PrefixlenPair};
 
 use clio::*;
-use std::io::BufRead;
+use std::io::{BufRead, Write};
 
 use clap::Parser;
 
+const WRITER_BUFSIZE: usize = 16 * 1024;
+
 #[derive(Parser)]
-#[command(author, version, about, long_about=None)]
+#[command(author, version, about)]
 struct Args {
     #[clap(value_parser, default_value = "-")]
     input: Vec<Input>,
-    #[structopt(
+    /// Maximum prefix length for prefixes read. Single value applies to IPv4 and IPv6, comma-separated [IPv4],[IPv6].
-        short,
+    #[structopt(short, long, default_value = "32,128")]
-        long,
-        default_value = "32,128",
-        help = "Maximum prefix length for prefixes read. Single value applies to IPv4 and IPv6, comma-separated [IPv4],[IPv6]."
-    )]
     max_prefixlen: PrefixlenPair,
-    #[arg(short, long, help = "truncate IP/mask to network/mask (else ignore)")]
+    /// Truncate IP/mask to network/mask (else ignore)
+    #[arg(short, long)]
     truncate: bool,
-    #[arg(
+    /// Only output IPv4 prefixes
-        id = "4",
+    #[arg(id = "4", short, conflicts_with("6"))]
-        short,
-        help = "Only output IPv4 prefixes",
-        conflicts_with("6")
-    )]
     only_v4: bool,
-    #[arg(
+    /// Only output IPv6 prefixes
-        id = "6",
+    #[arg(id = "6", short, conflicts_with("4"))]
-        short,
-        help = "Only output IPv6 prefixes",
-        conflicts_with("4")
-    )]
     only_v6: bool,
 }
 
@@ -68,46 +60,55 @@ struct App {
 }
 
 impl App {
-    fn add_prefix(&mut self, pfx: IpOrNet) {
+    fn add_prefix<const TRUNCATE: bool>(&mut self, pfx: IpOrNet) {
         // Parser accepts host bits set, so detect that case and error if not truncate mode
         // Note: aggregate6 errors in this case regardless of -4, -6 so do the same
-        if !self.args.truncate {
+        if !TRUNCATE && pfx.has_host_bits() {
-            if pfx.net.addr() != pfx.net.network() {
+            // We don't have the original string any more so our error
+            // differs from `aggregate6` in that it prints the pfxlen as
+            // parsed, not as in the source.
             eprintln!("ERROR: '{}' is not a valid IP network, ignoring.", pfx);
             return;
         }
-        }
+
-        // Don't bother saving if we won't display.
         if self.args.only_v4 && pfx.is_ipv6() {
             return;
-        } else if self.args.only_v6 && pfx.is_ipv4() {
+        }
+        if self.args.only_v6 && pfx.is_ipv4() {
             return;
         }
         if self.args.max_prefixlen >= pfx {
             self.prefixes.add(pfx);
         }
     }
-    fn consume_input(&mut self, input: &mut Input) {
+    fn consume_input<const TRUNCATE: bool>(&mut self, input: &mut Input) {
         for line in input.lock().lines() {
-            for net in line.unwrap().split_whitespace().to_owned() {
+            match line {
+                Ok(line) => {
+                    for net in line.split_ascii_whitespace() {
                         let pnet = net.parse::<IpOrNet>();
                         match pnet {
-                    Ok(pnet) => self.add_prefix(pnet),
+                            Ok(pnet) => self.add_prefix::<TRUNCATE>(pnet),
                             Err(_e) => {
-                        // self.errors.push(IpParseError {
-                        //     ip: net.to_string(),
-                        //     problem: e.to_string(),
-                        // });
                                 eprintln!("ERROR: '{}' is not a valid IP network, ignoring.", net);
                             }
                         }
                     }
                 }
+                Err(e) => {
+                    eprintln!("I/O error! {}", e);
+                    exit(1);
+                }
+            }
+        }
     }
     fn simplify_inputs(&mut self) {
         let inputs = self.args.input.to_owned();
         for mut input in inputs {
-            self.consume_input(&mut input);
+            match self.args.truncate {
+                true => self.consume_input::<true>(&mut input),
+                false => self.consume_input::<false>(&mut input),
+            }
         }
         self.prefixes.simplify();
     }
@@ -117,9 +118,11 @@ impl App {
 
         self.simplify_inputs();
 
-        for net in &self.prefixes {
+        let stdout = io::stdout().lock();
-            println!("{}", net);
+        let mut w = io::BufWriter::with_capacity(WRITER_BUFSIZE, stdout);
-        }
+
+        write!(&mut w, "{}", self.prefixes).unwrap();
+        w.flush().unwrap();
     }
 }
 

tests/cli.rs

@@ -1,17 +1,66 @@
 use assert_cmd::Command;
 use glob::glob;
-use predicates::prelude::*; // Used for writing assertions
+use predicates::prelude::*;
+use predicates::reflection::PredicateReflection;
+// Used for writing assertions
 use rstest::*;
-use std::{error::Error, fs::File, io::Read, path::Path};
+use std::fmt::Display;
+use std::{error::Error, fs::File, io::Read, path::Path, str};
 
-// Really should normalize the data (lex sort) before comparison
+struct SortedEquals {
+    expect: Vec<u8>,
+}
+
+fn sort_buf(input: &[u8]) -> Vec<u8> {
+    let mut lines = input
+        .split(|x| *x == b'\n')
+        .map(|x| Vec::<u8>::from(x))
+        .collect::<Vec<Vec<u8>>>();
+    lines.sort();
+    lines.join(&b'\n')
+}
+
+impl SortedEquals {
+    fn new(expect: &[u8]) -> SortedEquals {
+        let sorted = sort_buf(expect);
+        SortedEquals { expect: sorted }
+    }
+}
+
+impl Display for SortedEquals {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(str::from_utf8(self.expect.as_slice()).unwrap())
+    }
+}
+
+impl Predicate<[u8]> for SortedEquals {
+    fn eval(&self, variable: &[u8]) -> bool {
+        // sort self into temporary, then compare with variable
+        let sorted = sort_buf(variable);
+        sorted == self.expect
+    }
+}
+
+impl PredicateReflection for SortedEquals {}
+
+/// Compare the output with pre-prepared expected outputs. When functionality is
+/// matching, we generate expected outputs with `aggregate6`, and expect byte-for-byte
+/// output consistency, including ordering. When our functionality and `aggregate6`'s
+/// diverge, we generate expected outputs ourselves, and expect output sorted by numeric
+/// value of the address.
+///
+/// Normalization is available for future test cases.
 #[rstest]
-#[case("test-data/dfz_combined", "")] // Basic aggregation test
+#[case::dfz_combined("test-data/dfz_combined", "", false)] // Basic aggregation test
-#[case("test-data/max_pfxlen", "-m 20")] // Filter on prefix length
+#[case::max_pfxlen("test-data/max_pfxlen", "-m 20", false)] // Filter on prefix length
-#[case("test-data/max_pfxlen_split", "-m 20,32")] // Filter on prefix length (split v4/v6)
+#[case::max_pfxlen_split("test-data/max_pfxlen_split", "-m 20,32", false)] // Filter on prefix length (split v4/v6)
-#[case("test-data/v4_only", "-4")] // Filter v4 only
+#[case::v4_only("test-data/v4_only", "-4", false)] // Filter v4 only
-#[case("test-data/v6_only", "-6")] // Filter v4 only
+#[case::v6_only("test-data/v6_only", "-6", false)] // Filter v6 only
-fn dfz_test(#[case] path: &str, #[case] args: &str) -> Result<(), Box<dyn Error>> {
+fn dfz_test(
+    #[case] path: &str,
+    #[case] args: &str,
+    #[case] normalize_data: bool,
+) -> Result<(), Box<dyn Error>> {
     let mut cmd = Command::cargo_bin("rs-aggregate")?;
     let in_path = Path::new(path).join("input");
     let expect_path = Path::new(path).join("expected");
@@ -26,10 +75,17 @@ fn dfz_test(#[case] path: &str, #[case] args: &str) -> Result<(), Box<dyn Error>
         .timeout(std::time::Duration::from_secs(30))
         .assert();
 
+    if normalize_data {
+        assert
+            .success()
+            .stdout(SortedEquals::new(&expect_data))
+            .stderr(predicate::str::is_empty());
+    } else {
         assert
             .success()
             .stdout(predicate::eq(expect_data))
             .stderr(predicate::str::is_empty());
+    }
 
     Ok(())
 }
@@ -79,7 +135,7 @@ fn multi_input_test(#[case] path: &str, #[case] args: &str) -> Result<(), Box<dy
 
     assert
         .success()
-        .stdout(predicate::eq(expect_data))
+        .stdout(SortedEquals::new(&expect_data))
         .stderr(predicate::str::is_empty());
 
     Ok(())