Skip to content

Commit 40d6a48

Browse files
DRiKEDRiKE
committed
More touching up, refactoring and documenting of path_selection helpers
1 parent efb5063 commit 40d6a48

File tree

2 files changed

+149
-147
lines changed

2 files changed

+149
-147
lines changed

‎fuzz/fuzz_targets/path_selection.rs

+34-2
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use libfuzzer_sys::fuzz_target;
66
use routecore::bgp::{
77
path_attributes::PaMap,
88
path_selection::{OrdStrat, OrdRoute, Rfc4271, SkipMed, TiebreakerInfo,
9-
best_backup, best
9+
best_backup, best_backup_generic, best
1010
}
1111
};
1212

@@ -73,6 +73,38 @@ fn verify_path_selection<'a, T, OS, I>(candidates: I)
7373
(best2.borrow().tiebreakers(), best2.borrow().pa_map()),
7474
(backup2.borrow().tiebreakers(), backup2.borrow().pa_map()),
7575
);
76+
77+
let (best_gen, backup_gen) = verify_path_selection_generic(candidates);
78+
let (best_gen, _backup_gen) = (best_gen.unwrap(), backup_gen.unwrap());
79+
80+
assert_eq!(best2, best_gen);
81+
82+
// Because best_backup_generic can't deal with duplicates, this won't
83+
// always hold:
84+
//assert_eq!(backup2, backup_gen);
85+
86+
}
87+
88+
fn verify_path_selection_generic<I, T>(candidates: I) -> (Option<T>, Option<T>)
89+
where
90+
I: Iterator<Item = T>,
91+
T: Debug + Ord
92+
{
93+
94+
let (best, backup) = best_backup_generic(
95+
// create tuples of the OrdRoute and a unique 'id'
96+
candidates.enumerate().map(|(idx, c)| (c, idx))
97+
);
98+
// because the returned values are tuples as well, we can now compare
99+
// those (instead of comparing the OrdRoute). With the unique IDs
100+
// attached, we can check whether `best` and `backup` are not equal in
101+
// terms of content.
102+
assert!(best.is_some());
103+
assert!(backup.is_some());
104+
assert_ne!(best, backup);
105+
106+
107+
(best.map(|t| t.0), backup.map(|t| t.0))
76108
}
77109

78110
// XXX while doing fuzz_target!(|data: &[u8]| ... and then creating an
@@ -104,9 +136,9 @@ fuzz_target!(|data: (
104136
Err(_) => return,
105137
};
106138

107-
//dbg!("skipmed");
108139
verify_ord(&a, &b, &c);
109140
verify_path_selection([&a, &b, &c, &b, &c, &a].into_iter());
141+
//verify_path_selection_generic([&a, &b, &c].into_iter());
110142

111143
//dbg!("rfc4271");
112144
/*

‎src/bgp/path_selection.rs

+115-145
Original file line numberDiff line numberDiff line change
@@ -492,61 +492,55 @@ impl fmt::Display for DecisionErrorType {
492492

493493
//------------ Helpers -------------------------------------------------------
494494

495-
pub fn preferred<'a, OS: OrdStrat>(
496-
a: OrdRoute<'a, OS>, b: OrdRoute<'a, OS>
497-
) -> OrdRoute<'a, OS> {
495+
496+
/// Returns the preferred route.
497+
///
498+
/// Note this method works on anything `T: Ord` and is thus not limited to
499+
/// `OrdRoute`. Hence one can pass in a tuple of `(OrdRoute, L)` as long as
500+
/// `L` implements `Ord`, to include and get back any local value like an ID.
501+
/// This is consistent with [`fn best`] and [`fn best_backup_generic`].
502+
pub fn preferred<T: Ord>(a: T, b: T) -> T {
498503
cmp::min(a,b)
499504
}
500505

501506
/// Selects the preferred ('best') route.
502-
pub fn best<'a, T, I, OS>(it: I) -> Option<T>
507+
///
508+
/// Note this method works on an iterator yielding anything `T: Ord`, so not
509+
/// limited to `OrdRoute`. It is in that sense consistent with [`fn
510+
/// best_backup_generic`], i.e. one can pass in tuples of `OrdRoute` and
511+
/// something else that implements `Ord`.
512+
pub fn best<T, I>(it: I) -> Option<T>
503513
where
504514
T: Ord,
505515
I: Iterator<Item = T>,
506-
T: core::borrow::Borrow<OrdRoute<'a, OS>>
507516
{
508517
it.min()
509518
}
510519

511-
/*
512-
pub fn best_with_strat<'a, OS, AltOS, I, T1>(it: I)
513-
-> Option<OrdRoute<'a, AltOS>>
514-
where
515-
OS: 'a + OrdStrat,
516-
AltOS: 'a + OrdStrat,
517-
I: Iterator<Item = T1>,
518-
T1: core::borrow::Borrow<OrdRoute<'a, OS>>
519-
{
520-
it.map(|r| r.into_strat()).min()
521-
}
522-
*/
523-
524-
525-
// XXX would this be convenient?
526-
pub fn best_alt<'a, OS: OrdStrat>(
527-
_pamaps: impl Iterator<Item = &'a PaMap>,
528-
_tiebreakers: impl Iterator<Item = TiebreakerInfo>
529-
) -> Option<OrdRoute<'a, OS>> {
530-
unimplemented!()
531-
}
532-
533-
/// Returns the 'best' and second-best path.
520+
/// Alternative, generic version of `fn best_backup`.
534521
///
535-
/// If the iterator passed in contains no paths, `(None, None)` is returned.
536-
/// If the iterator yields only a single item, that will be the 'best' path,
537-
/// and the 'backup' will be None, i.e. `(Some(best), None)`.
522+
/// This method takes any iterator yielding items implementing Ord. As such,
523+
/// it has little to do with routes or path selection per se.
538524
///
539-
/// In all other cases (an iterator yielding two or more non-identical
540-
/// values), both members of the tuple should be `Some(..)`.
541-
///
542-
/// The returned 'best' path is the same as the path returned by [`fn best`].
543-
pub fn best_backup<'a, OS, T>(
544-
it: impl Iterator<Item = T>
545-
) -> (Option<T>, Option<T>)
546-
where
547-
OS: OrdStrat,
548-
T: Ord,
549-
T: core::borrow::Borrow<OrdRoute<'a, OS>>
525+
/// Note that because of this genericness, we have no access to methods or
526+
/// members of `T`, and thus are unable to compare actual contents such as a
527+
/// `PaMap` or the `TiebreakerInfo` when comparing `OrdRoute`s. This means the
528+
/// method can not check for any duplicate route information between `T`s, and
529+
/// really only order them. The caller therefore has to make sure to pass in
530+
/// an iterator that does not yield any duplicate routes.
531+
///
532+
/// This method enables the caller to attach additional information, as long
533+
/// as it implements `Ord`. For example, one can pass in an iterator over
534+
/// tuples of `OrdRoute` and something else. As long as the `OrdRoute` is the
535+
/// first member of that tuple and no duplicates are yielded from the
536+
/// iterator, the additional information is not used in the ordering process
537+
/// but is returned together with the 'best' and 'backup' tuples. This can be
538+
/// useful when the caller needs to relate routes to local IDs or something
539+
/// alike.
540+
pub fn best_backup_generic<I, T>(it: I) -> (Option<T>, Option<T>)
541+
where
542+
I: Iterator<Item = T>,
543+
T: Ord
550544
{
551545
let mut best = None;
552546
let mut backup = None;
@@ -566,33 +560,11 @@ pub fn best_backup<'a, OS, T>(
566560

567561
// c is not better than best, now check backup
568562
match backup.take() {
569-
None => {
570-
// Before we set the backup route, ensure it is not
571-
// the same route as best.
572-
// We compare the actual contents of the OrdRoute
573-
// here, i.e. the TiebreakerInfo and PaMap. If we'd
574-
// simply compare the OrdRoutes themselves, we'd be
575-
// testing whether or not they are considered equal in
576-
// terms of path preference, NOT in terms of content.
577-
// If for example we have a candidate backup path with
578-
// a different AS_PATH but of equal length as the best
579-
// path, the OrdRoutes are considered equal even
580-
// though the actual path attributes differ.
581-
//
582-
583-
// Best is always Some at this point, unwrap is safe.
584-
if best.as_ref().unwrap().borrow().inner() != c.borrow().inner() {
585-
backup = Some(c);
586-
} }
563+
None => { backup = Some(c); }
587564
Some(cur_backup) => {
588565
if c < cur_backup {
589566
// c is preferred over current backup
590-
// check if it is not the same route as 'best'
591-
if best.as_ref() != Some(&c) {
592-
backup = Some(c);
593-
} else {
594-
backup = Some(cur_backup);
595-
}
567+
backup = Some(c);
596568
} else {
597569
// put it back in
598570
backup = Some(cur_backup);
@@ -607,61 +579,72 @@ pub fn best_backup<'a, OS, T>(
607579
(best, backup)
608580
}
609581

610-
/// Alternative, generic version of `fn best_backup`.
611-
///
612-
/// This method takes any iterator providing Items implementing Ord, such as
613-
/// `OrdRoute`. As such, it has little to do with routes or path selection per
614-
/// se.
615-
///
616-
/// Note that because of this genericness, we have no access to methods or
617-
/// members of `T`, and thus are unable to compare actual contents such as a
618-
/// `PaMap` or the `TiebreakerInfo`. This means the method can not check for
619-
/// any duplicate route information between `T`s, and really only order them.
620-
/// The caller therefore has to make sure to pass in an iterator that does not
621-
/// yield any duplicate routes.
622-
///
623-
/// This method enables the caller to attach additional information, as long
624-
/// as it implements `Ord`. For example, one can pass in an iterator over
625-
/// tuples of `OrdRoute` and something else. As long as the `OrdRoute` is the
626-
/// first member of that tuple and no duplicates are yielded from the
627-
/// iterator, the additional information is not used in the ordering process
628-
/// but is returned together with the 'best' and 'backup' tuples. This can be
629-
/// useful when the caller needs to relate routes to local IDs or something
630-
/// alike.
631-
///
632-
pub fn best_backup_generic<I, T>(it: I) -> (Option<T>, Option<T>)
582+
583+
// Attaches an index to a (generic) T, only used internally in _best_backup.
584+
type RouteWithIndex<T> = (usize, T);
585+
586+
// Internal version doing the heavy lifting for both best_backup and
587+
// best_backup_position. Returns both the routes themselves (T) and their
588+
// index (usize).
589+
fn _best_backup<'a, I, T, OS>(it: I)
590+
-> (Option<RouteWithIndex<T>>, Option<RouteWithIndex<T>>)
633591
where
592+
OS: OrdStrat,
634593
I: Iterator<Item = T>,
635-
T: Ord
594+
T: Ord + core::borrow::Borrow<OrdRoute<'a, OS>>
636595
{
637-
let mut best = None;
638-
let mut backup = None;
596+
let mut best: Option<RouteWithIndex<T>> = None;
597+
let mut backup: Option<RouteWithIndex<T>> = None;
639598

640-
for c in it {
599+
for (idx, c) in it.enumerate() {
641600
match best.take() {
642-
None => { best = Some(c); continue }
643-
Some(cur_best) => {
601+
None => { best = Some((idx, c)); continue }
602+
Some((idx_best, cur_best)) => {
644603
if c < cur_best {
645604
// c is preferred over current best
646-
best = Some(c);
647-
backup = Some(cur_best);
605+
best = Some((idx, c));
606+
backup = Some((idx_best, cur_best));
648607
continue;
649608
}
650609
// put it back in
651-
best = Some(cur_best);
610+
best = Some((idx_best, cur_best));
652611

653612
// c is not better than best, now check backup
654613
match backup.take() {
655-
None => { backup = Some(c); }
656-
Some(cur_backup) => {
614+
None => {
615+
// Before we set the backup route, ensure it is not
616+
// the same route as best.
617+
// We compare the actual contents of the OrdRoute
618+
// here, i.e. the TiebreakerInfo and PaMap. If we'd
619+
// simply compare the OrdRoutes themselves, we'd be
620+
// testing whether or not they are considered equal in
621+
// terms of path preference, NOT in terms of content.
622+
// If for example we have a candidate backup path with
623+
// a different AS_PATH but of equal length as the best
624+
// path, the OrdRoutes are considered equal even
625+
// though the actual path attributes differ.
626+
//
627+
628+
// `best` is always Some(..) at this point, but we do
629+
// an `if let` instead of an `unwrap` anyway.
630+
if let Some((_, cur_best)) = best.as_ref() {
631+
if cur_best.borrow().inner() != c.borrow().inner()
632+
{
633+
backup = Some((idx, c));
634+
}
635+
}
636+
}
637+
Some((idx_backup, cur_backup)) => {
657638
if c < cur_backup {
658639
// c is preferred over current backup
659-
backup = Some(c);
660-
} else {
661-
// put it back in
662-
backup = Some(cur_backup);
640+
// check if it is not the same route as 'best'
641+
if best.as_ref().map(|t| &t.1) != Some(&c) {
642+
backup = Some((idx, c));
643+
continue;
644+
}
663645
}
664-
646+
// put it back in
647+
backup = Some((idx_backup, cur_backup));
665648
}
666649
}
667650
}
@@ -671,50 +654,37 @@ where
671654
(best, backup)
672655
}
673656

674-
/*
675-
pub fn best_multistrat<'a, OS1: 'a + OrdStrat, OS2: 'a + OrdStrat, I>(it: I)
676-
-> Option<(OrdRoute<'a, OS1>, OrdRoute<'a, OS2>)>
677-
where
678-
I: Clone + Iterator<Item = OrdRoute<'a, OS1>>,
657+
/// Returns the 'best' and second-best path.
658+
///
659+
/// If the iterator passed in contains no paths, `(None, None)` is returned.
660+
/// If the iterator yields only a single item, that will be the 'best' path,
661+
/// and the 'backup' will be None, i.e. `(Some(best), None)`.
662+
///
663+
/// In all other cases (an iterator yielding two or more non-identical
664+
/// values), both members of the tuple should be `Some(..)`.
665+
///
666+
/// The returned 'best' path is the same as the path returned by [`fn best`].
667+
pub fn best_backup<'a, I, T, OS>(it: I) -> (Option<T>, Option<T>)
668+
where
669+
OS: OrdStrat,
670+
I: Iterator<Item = T>,
671+
T: Ord + core::borrow::Borrow<OrdRoute<'a, OS>>
679672
{
680-
let res1 = best(it.clone());
681-
let res1 = match res1 {
682-
Some(r) => r,
683-
None => return None
684-
};
685-
686-
// Given that res1 is not None, `it` is non-empty.
687-
// For a non-empty collection of OrdRoutes, there is always a best, so we
688-
// can unwrap().
689-
let res2 = best_with_strat::<'_, _, OS2, _>(it).unwrap();
690-
691-
Some((res1, res2))
673+
let (best, backup) = _best_backup(it);
674+
(best.map(|b| b.1), backup.map(|b| b.1))
692675
}
693-
*/
694-
695-
/*
696-
pub fn best_multistrat<'a, I, T1, T2, OS1, OS2>(it: I) -> Option<(T1, T2)>
697-
where
698-
T1: Ord + core::borrow::Borrow<OrdRoute<'a, OS1>>,
699-
T2: Ord,
700-
OS1: OrdStrat,
701-
OS2: OrdStrat,
702-
I: Clone + Iterator<Item = T1>,
676+
677+
/// Returns the index of the best and backup paths in the passed iterator.
678+
pub fn best_backup_position<'a, I, T, OS>(it: I)
679+
-> (Option<usize>, Option<usize>)
680+
where
681+
OS: OrdStrat,
682+
I: Iterator<Item = T>,
683+
T: Ord + core::borrow::Borrow<OrdRoute<'a, OS>>
703684
{
704-
let res1 = best(it.clone());
705-
let res1 = match res1 {
706-
Some(r) => r,
707-
None => return None
708-
};
709-
710-
// Given that res1 is not None, `it` is non-empty.
711-
// For a non-empty collection of OrdRoutes, there is always a best, so we
712-
// can unwrap().
713-
let res2 = best_with_strat::<'_, OS1, OS2, _>(it).unwrap();
714-
715-
Some((res1, res2))
685+
let (best, backup) = _best_backup(it);
686+
(best.map(|b| b.0), backup.map(|b| b.0))
716687
}
717-
*/
718688

719689

720690

0 commit comments

Comments
 (0)