[pmg-devel] [PATCH pmg-log-tracker] fix SEntry print() with filter in before-queue case

[Wolfgang Bumiller]

On Wed, Mar 04, 2020 at 03:15:18PM +0100, Mira Limbeck wrote:
> With before-queue filtering if we don't accept the mail (e.g.
> quarantine), we don't have a queue which means we don't have a QEntry,
> so the SEntry has to handle the filter entries (ToEntrys).
> 
> This means we can't just return from print() when either a 'from' or 'to'
> filter is set or we exclude greylist entries or NDRs and no Noqueue entries
> exist or no entry matches any of the filters.
> 
> So continue printing if there is no filter parameter set, but an FEntry
> reference in the SEntry. If there's an FEntry reference, compare all ToEntrys
> to the filter parameter and return if there is no match at all.
> 
> Signed-off-by: Mira Limbeck <m.limbeck at proxmox.com>
> ---
>  src/main.rs | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/src/main.rs b/src/main.rs
> index 9cb96f0..d031fef 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -869,7 +869,7 @@ impl SEntry {
>              }
>          }
>  
> -        // if either ;from' or 'to' are set, check if it matches, if not, set
> +        // if either 'from' or 'to' are set, check if it matches, if not, set
>          // the status of the noqueue entry to Invalid
>          // if exclude_greylist or exclude_ndr are set, check if it matches
>          // and if so, set the status to Invalid so they are no longer included
> @@ -896,8 +896,31 @@ impl SEntry {
>                      found = true;
>                  }
>              }
> +            if let Some(fe) = &self.filter {
> +                if let Some(fe) = fe.upgrade() {

Please don't nest this deeply. Consider adding getter methods to all the
XEntry structs for the various `Weak`s.
There are a handful of places with the exact same 2-line pattern above
indenting twice.

Consider:
    impl SEntry {
        fn filter(&self) -> Option<Rc<RefCell<FEntry>>> {
            self.filter
                .clone()
                .and_then(|w| w.upgrade())
        }
    }

Replacing the above 2 `if let` with a single one calling the `filter()`
getter:

    if let Some(fe) = self.filter() {

Same for `FEntry::qentry()`

Also consider factoring this entire block into a separate function with
a proper name and doc comment (or at least the encosing `if` (with the
comment lines of the first hunk above).

> +                    let is_filtered =
> +                        !parser.options.from.is_empty() || !parser.options.to.is_empty();
> +                    if fe.borrow().is_bq && !fe.borrow().is_accepted {
> +                        for to in fe.borrow().to_entries.iter() {
> +                            if (!parser.options.from.is_empty()

When factoring this out with less indentation the above linei can be
moved above the loop since it never changes during the loop.

> +                                && find_lowercase(&self.bq_from, parser.options.from.as_bytes())

Almost all `find_lowercase` calls are preceded by some `!is_empty()`
check (on one side or another), which makes me wonder if the function
itself shouldn't be the one doing those? (But given that it's not always
on the same side we'd need to make sure the results would be the same.)

> +                                    .is_some())
> +                                || (!parser.options.to.is_empty()
> +                                    && find_lowercase(&to.to, parser.options.to.as_bytes())
> +                                        .is_some())
> +                            {
> +                                found = true;
> +                                break;
> +                            }
> +                        }
> +                        if !found && is_filtered {
> +                            return;
> +                        }
> +                    }
> +                }
> +            }
>  
> -            if !found {
> +            if !found && self.filter.is_none() {
>                  return;
>              }
>          }
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
>