On Wed, Apr 26, 2023 at 12:43 PM Dmitry Vyukov <dvyukov@xxxxxxxxxx> 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! Cheers, Miguel
