On Wed, 26 Apr 2023 at 13:37, Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote: > > I understand your intentions and they make sense. > > Thanks! I am glad you agree it may have some value -- please see below > for details. > > > But adding this logic to syzbot won't help thousands of users of > > get_maintainer.pl and dozens of other testing systems. There also will > > I haven't said otherwise -- as I said, I think it would be nice to > have this be part of the kernel itself. :) > > > be a bit of get_maintainer.pl inside of syzbot code, so now all kernel > > developers will need to be aware of it and also submit changes to > > syzbot when they want to change maintainers logic. > > > > I think this also equally applies to all other users of K:. > > And a number of them had similar complaints re how K; works. > > Yeah, I would imagine so. > > > I am thinking if K: should actually apply just to patches and be > > ignored for source files? > > I considered that -- for things like Rust, it could make sense, but > perhaps somebody is already using `K:` to match files they do care > about, rather than `F:`. So we would need to ask others, but I think > it is fine. > > > If there are files that belong to "rust" (or "bpf" or any other user > > of K:), then I think these should be just listed explicitly in the > > subsystem (that should be a limited set of files that can be > > enumerated with wildcards). > > Yes, at least for Rust, modulo omissions, we match files explicitly > with `F:`. We have a couple unimportant omissions, e.g. > `.rustfmt.toml`, but I can send a patch. > > Personally, I have always seen `F:` files (and `N:`-matched ones) as > having more weight than `K:`-matched ones, i.e. I saw `K:` as more of > a "it depends on what it matches -- discretion needed". > > From a quick look, most `K:`-using subsystems seem to list `F:` and > `N:` as I would expect. > > > It's also reasonable to apply K: to patches. > > Yes, definitely, for Rust, that is our main use case, i.e. it is > mainly why we wanted to have the `K:` entry: to catch changes to > things that are tagged with "Rust" in C files (early on, at least). > > It is particularly important for us, since we are also considering > having more of these annotations in the future. > > > But if a random source file happened to mention "rust" somewhere once, > > I am not sure you want to be CCed on all issues in that file. > > Does it sound reasonable? > > For Rust, yes, that would probably work for us. Not sure for all > subsystems using `K:`, though. > > Having said that, I suggested including the kernel config too in this > decision (i.e. not for the patches case, but for testers finding > runtime issues), because it adds information: it leaves reports out > when something is not even enabled but matched via `K:`, but still > allows a Cc when matched via `K:` and enabled. It is, of course, still > potentially a false positive, but some subsystems may want to hear > about those. > > For instance, for Rust, this would be fine early on, since we don't > expect many to have `RUST=y` to begin with, and thus the odd false > positive report via `K:` is fine. Later on, this heuristic may change, > and we may not change those matches anymore (especially since, by > then, the goal is that subsystems would be taking care of their own > Rust bits). > > This is what I was suggesting to then put in `get_maintainer.pl`, e.g. > a `--bot` option (or `--runtime`, or `--config-based-filtering`, or > similar) option. Then the bots can add that option on their side. > > Thanks again for considering this! Was looking at what's the status of this, and if we need to file a feature request for syzbot. Turns out Joe Perches fixed this a bit ago by adding --keywords-in-file flag (off by default): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=71ca5ee18708c1f9f086e20ac0a657009bcfe43a I think that's the right thing to do. syzbot won't be confused by widely matched K: patterns with sending reports (since it runs get_maintainers.pl on files).
