I'll be adding Hans-Peter to the review. He can confirm your logic.
Having a more detailed description of what you are changing and why would be good in the summary. I'm sure that hps (hans peter) will want to know. What you said to me on discord would be a good start.
Describing the end effect is a good start, but the details from discord will help others understand why these changes were made.

Oops, yeah, I got a bit caught up in following the template that I wasn't sure what all to include.

Originally I had mentioned endianess confusion, but that was actually just me being confused and can be ignored.

The exact changes are:

• Fix generated BPF program accepting packets which match any filter (instead of all filters combined, considering the default behavior already accepts all packets I don't see any point in a way to accept more than every one).
• Fix parsing for -f inserting incorrect filters when a unit isn't specified. (-f .82 would make the first strol return 0, making it act the same as -f 0.82)
• Allow specifying values for filters in hex (endpoints are already displayed as hex, so I figure it would be best to allow copy-pasting output to write a filter instead of having to convert back to decimal for the -f argument)

As for the actual BPF, it boils down to a bit of bad logic as far as I can tell. The original logic is similar to

// for unit+endpoint filters
if (packet->unit == unit) {
    if (packet->endpoint == endpoint) {
        return packet;
    }
    else {
        goto next_filter;
    }
}
else {
    goto next_filter;
}
// for individual unit/endpoint filters
if (packet->field == field) {
    return packet;
}
else {
    goto next_filter;
}

last_filter:
return 0;

Which isn't totally incorrect. However, any "partial" unit/endpoint filter that matches any field of a given packet all will skip over all future filters and accept the packet immediately.
Additionally, when invoking with a device that already includes a unit (such as -d ugen3.2) a default unit filter for unit 2 is inserted, meaning that every packet will be matched before and user-provided filters will even be reached.

Instead of giving every filter the chance to accept a packet, these changes make it so individual filters can only deny a packet, and only packets which "survive" each filter in sequence will be accepted at the end of the program.

OR-ing the "-f" endpoint filters is the expected behaviour. They should not be ANDed from what I can see. It's if (A | B | C) capture packet.

Regarding the strtol() to accept hex values, that is probably OK.

--HPS

Do you have an example using usbdump in user-space, where your fixes make a difference? It would help understand what is wrong!

Hmm, looking back the same "bug" is present in match_filter and dates all the way back to the initial commit for the utility, so you're right about it being intentional. I had always been invoking with the full -d argument which implicitly added a X.-1 filter for -d ugenY.X, which was why I was so convinced it was wrong (as far as I could at the time, no matter what was entered for -f it had no effect).

(edit: just noticed you were the original author! Sorry about my phrasing there, I didn't mean to sound so skeptical, I was just hoping to show my thought process. I'm also not sure how I've marked more comments as done, I'm going to go to sleep before I end up hitting any wrong buttons)

Considering this "problem" has been around for so long and nobody else has seen it as such, I think it would probably be best for me to come back with another version of my changes which allow for both flavors of logic for filters, since there's 10 years of proof that the original was correct (and that "fixing" this could actually break things).

As for my actual usage, I have a Logitech G502 mouse with some extra buttons bound to Scan_Previous_Track/Scan_Next_Track, where the all regular "mouse" events happen on endpoint 0x81, but the media key events happen on endpoint 0x82. I was trying to debug uhidd dropping events so I decided to use usbdump as my ground truth. However, the default filter from -d alongside extra filters being OR'd ended up meaning that no combination of arguments would give me the output I was after. I've just now figured out that invoking with -d ugen3 -f 2.130 does actually get me the output I need (although in an a slightly obscure way, so I'll still see about allowing for more flexible filters).

I've thought about this more, and I think that the real problem is that OR-ing a unit and unit.endpoint filter for the same unit isn't intuitive, since the lone unit filter will always match and make the unit.endpoint filter pointless. I'm not sure if it is fair to expect someone writing a filter to know this and avoid unit.-1 filters when using unit.endpoint or -1.endpoint filters though (and the implicit unit filter for -d ugenX.Y muddies the waters a bit more).
So, I've come up with two potential solutions:

1. Make it more clear that this is the behavior on the man page ("for dummies" kind of stuff, aka "for me")
2. Add a way to "correct" invocations with both unit and unit.endpoint filters to only actually apply the unit.endpoint filters (and warn for redundant unit.endpoint filters)

Of course, solution 2 would include solution 1 since a new argument would have to be documented and explained.
I'll be updating the revision to implement number 2 in a moment since it seems like the optimal solution (and shouldn't break any existing uses unless they parse verbose output poorly). I haven't touched the man page yet though, since I'm not 100% sure if a code change is the best fit for a mostly human problem, and don't want to get invested in documenting unless the code changes will stick.

Hi,

Thanks for your detailed explanation. Now I see what was your problem!

Is there any reason we shouldn't prune by default?

--HPS

My only worry with pruning by default is breaking any existing uses. Despite how unlikely it is that someone built a script/automation around usbdump output with conflicting filters, there's a chance that their filters were wrong but seemed to work thanks to shadowing.

I'll start on updating the man page now (and will fix that leak). For the time being I'll keep things -p agnostic so we can still remove it if desired.

I think that will be OK. As long as it says in dmesg what is wrong, it will be good.

And if we do this change in FreeBSD-14 and newer, that is also an option for pruning by default.

Alright, sounds good to me. This definitely isn't an urgent change so I'll switch to pruning by default and get the man page updated, then this should be good to go.

I totally forgot that I need to update the revision when I make a new commit, sorry about the delay!

The man page changes didn't end up being too intrusive thanks to pruning by default, since I didn't have to explain how to avoid ending up with a set of filters that needs to be pruned. I also got that tiny leak fixed, and updated -h output, which I had forgotten before.

Since the behavior is changing from never pruning (until 13) to pruning by default with an option to return to earlier behavior in 14 , the manual page needs a COMPATIBILITY section.

Of the options marked "also work with" in the DESCRIPTION, which does it make sense to use alone, and which would likely be used in groups? If there's enough of the latter; I think they should be grouped in the synopsis, maybe by splitting the synopsis line.

I've added the compatibility section, and I realized that almost all of the options work together, so I changed it to only mention options which aren't compatible instead.