1. Patchew
  2. Xen
  3. View series

[PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry

Jan Beulich posted 1 patch 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/e3ccc381-1fd5-b99c-e37e-5870af401dd0@suse.com
[PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry
Posted by Jan Beulich 8 months ago 
If the F: description is to be trusted, the two xen/arch/x86/hvm/
lines were fully redundant with the earlier wildcard ones. Arch header
files, otoh, were no longer covered by anything as of the move from
include/asm-*/ to arch/*/include/asm/. Further also generalize (by
folding) the x86- and Arm-specific mem_access.c entries.

Finally, again assuming the F: description can be trusted, there's no
point listing arch/, common/, and include/ entries separately. Fold
them all.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Further fold patterns.
---
Triggered by me looking at the entry in the context of Oleksii's RISC-V
preparatory patch.

--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -558,20 +558,9 @@ R:	Alexandru Isaila <aisaila@bitdefender
 R:	Petre Pircalabu <ppircalabu@bitdefender.com>
 S:	Supported
 F:	tools/misc/xen-access.c
-F:	xen/arch/*/monitor.c
-F:	xen/arch/*/vm_event.c
-F:	xen/arch/arm/mem_access.c
-F:	xen/arch/x86/include/asm/hvm/monitor.h
-F:	xen/arch/x86/include/asm/hvm/vm_event.h
-F:	xen/arch/x86/mm/mem_access.c
-F:	xen/arch/x86/hvm/monitor.c
-F:	xen/arch/x86/hvm/vm_event.c
-F:	xen/common/mem_access.c
-F:	xen/common/monitor.c
-F:	xen/common/vm_event.c
-F:	xen/include/*/mem_access.h
-F:	xen/include/*/monitor.h
-F:	xen/include/*/vm_event.h
+F:	xen/*/mem_access.[ch]
+F:	xen/*/monitor.[ch]
+F:	xen/*/vm_event.[ch]
 
 VPCI
 M:	Roger Pau Monné <roger.pau@citrix.com>
Re: [PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry
Posted by Anthony PERARD 7 months, 3 weeks ago 
On Thu, Aug 31, 2023 at 08:15:13AM +0200, Jan Beulich wrote:
> If the F: description is to be trusted, the two xen/arch/x86/hvm/
> lines were fully redundant with the earlier wildcard ones. Arch header
> files, otoh, were no longer covered by anything as of the move from
> include/asm-*/ to arch/*/include/asm/. Further also generalize (by
> folding) the x86- and Arm-specific mem_access.c entries.
> 
> Finally, again assuming the F: description can be trusted, there's no
> point listing arch/, common/, and include/ entries separately. Fold
> them all.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> -F:	xen/arch/*/monitor.c
> -F:	xen/arch/*/vm_event.c
> -F:	xen/arch/arm/mem_access.c
> -F:	xen/arch/x86/include/asm/hvm/monitor.h
> -F:	xen/arch/x86/include/asm/hvm/vm_event.h
> -F:	xen/arch/x86/mm/mem_access.c
> -F:	xen/arch/x86/hvm/monitor.c
> -F:	xen/arch/x86/hvm/vm_event.c
> -F:	xen/common/mem_access.c
> -F:	xen/common/monitor.c
> -F:	xen/common/vm_event.c
> -F:	xen/include/*/mem_access.h
> -F:	xen/include/*/monitor.h
> -F:	xen/include/*/vm_event.h
> +F:	xen/*/mem_access.[ch]
> +F:	xen/*/monitor.[ch]
> +F:	xen/*/vm_event.[ch]


Hi,

Did you mean to for example change the maintainer ship of
"xen/arch/x86/mm/mem_access.c"? Before it was:
    - VM EVENT, MEM ACCESS and MONITOR
    - X86 MEMORY MANAGEMENT
    - X86 ARCHITECTURE
And now, it's just:
    - X86 MEMORY MANAGEMENT
    - X86 ARCHITECTURE

(see ./scripts/get_maintainer.pl --sections -f xen/arch/x86/mm/mem_access.c)

Also, now "xen/include/xen/monitor.h" is only "THE REST".

On the other hand, there's no change for "xen/common/monitor.c", so the
pattern works for this particular file.

Cheers,

-- 
Anthony PERARD
Re: [PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry
Posted by Jan Beulich 7 months, 3 weeks ago 
On 06.09.2023 14:40, Anthony PERARD wrote:
> On Thu, Aug 31, 2023 at 08:15:13AM +0200, Jan Beulich wrote:
>> If the F: description is to be trusted, the two xen/arch/x86/hvm/
>> lines were fully redundant with the earlier wildcard ones. Arch header
>> files, otoh, were no longer covered by anything as of the move from
>> include/asm-*/ to arch/*/include/asm/. Further also generalize (by
>> folding) the x86- and Arm-specific mem_access.c entries.
>>
>> Finally, again assuming the F: description can be trusted, there's no
>> point listing arch/, common/, and include/ entries separately. Fold
>> them all.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> -F:	xen/arch/*/monitor.c
>> -F:	xen/arch/*/vm_event.c
>> -F:	xen/arch/arm/mem_access.c
>> -F:	xen/arch/x86/include/asm/hvm/monitor.h
>> -F:	xen/arch/x86/include/asm/hvm/vm_event.h
>> -F:	xen/arch/x86/mm/mem_access.c
>> -F:	xen/arch/x86/hvm/monitor.c
>> -F:	xen/arch/x86/hvm/vm_event.c
>> -F:	xen/common/mem_access.c
>> -F:	xen/common/monitor.c
>> -F:	xen/common/vm_event.c
>> -F:	xen/include/*/mem_access.h
>> -F:	xen/include/*/monitor.h
>> -F:	xen/include/*/vm_event.h
>> +F:	xen/*/mem_access.[ch]
>> +F:	xen/*/monitor.[ch]
>> +F:	xen/*/vm_event.[ch]
> 
> 
> Hi,
> 
> Did you mean to for example change the maintainer ship of
> "xen/arch/x86/mm/mem_access.c"? Before it was:
>     - VM EVENT, MEM ACCESS and MONITOR
>     - X86 MEMORY MANAGEMENT
>     - X86 ARCHITECTURE
> And now, it's just:
>     - X86 MEMORY MANAGEMENT
>     - X86 ARCHITECTURE
> 
> (see ./scripts/get_maintainer.pl --sections -f xen/arch/x86/mm/mem_access.c)
> 
> Also, now "xen/include/xen/monitor.h" is only "THE REST".

No, no change of maintainership was intended. But there was an uncertainty,
which is why I said "assuming the F: description can be trusted". So ...

> On the other hand, there's no change for "xen/common/monitor.c", so the
> pattern works for this particular file.

... together with this observation, I take it that

	   F:	*/net/*		all files in "any top level directory"/net

is actually at best misleading / ambiguous - I read it as not just a single
level of directories, but it may well be that that's what is meant. At
which point the question is how "any number of directories" could be
expressed. Would **/ or .../**/... work here? I'm afraid my Perl is far
from sufficient to actually spot where (and hence how) this is handled in
the script.

Jan
Re: [PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry
Posted by Anthony PERARD 7 months, 3 weeks ago 
On Wed, Sep 06, 2023 at 02:50:22PM +0200, Jan Beulich wrote:
> On 06.09.2023 14:40, Anthony PERARD wrote:
> > On Thu, Aug 31, 2023 at 08:15:13AM +0200, Jan Beulich wrote:
> >> If the F: description is to be trusted, the two xen/arch/x86/hvm/
> >> lines were fully redundant with the earlier wildcard ones. Arch header
> >> files, otoh, were no longer covered by anything as of the move from
> >> include/asm-*/ to arch/*/include/asm/. Further also generalize (by
> >> folding) the x86- and Arm-specific mem_access.c entries.
> >>
> >> Finally, again assuming the F: description can be trusted, there's no
> >> point listing arch/, common/, and include/ entries separately. Fold
> >> them all.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> -F:	xen/arch/*/monitor.c
> >> -F:	xen/arch/*/vm_event.c
> >> -F:	xen/arch/arm/mem_access.c
> >> -F:	xen/arch/x86/include/asm/hvm/monitor.h
> >> -F:	xen/arch/x86/include/asm/hvm/vm_event.h
> >> -F:	xen/arch/x86/mm/mem_access.c
> >> -F:	xen/arch/x86/hvm/monitor.c
> >> -F:	xen/arch/x86/hvm/vm_event.c
> >> -F:	xen/common/mem_access.c
> >> -F:	xen/common/monitor.c
> >> -F:	xen/common/vm_event.c
> >> -F:	xen/include/*/mem_access.h
> >> -F:	xen/include/*/monitor.h
> >> -F:	xen/include/*/vm_event.h
> >> +F:	xen/*/mem_access.[ch]
> >> +F:	xen/*/monitor.[ch]
> >> +F:	xen/*/vm_event.[ch]
> > 
> > 
> > Hi,
> > 
> > Did you mean to for example change the maintainer ship of
> > "xen/arch/x86/mm/mem_access.c"? Before it was:
> >     - VM EVENT, MEM ACCESS and MONITOR
> >     - X86 MEMORY MANAGEMENT
> >     - X86 ARCHITECTURE
> > And now, it's just:
> >     - X86 MEMORY MANAGEMENT
> >     - X86 ARCHITECTURE
> > 
> > (see ./scripts/get_maintainer.pl --sections -f xen/arch/x86/mm/mem_access.c)
> > 
> > Also, now "xen/include/xen/monitor.h" is only "THE REST".
> 
> No, no change of maintainership was intended. But there was an uncertainty,
> which is why I said "assuming the F: description can be trusted". So ...
> 
> > On the other hand, there's no change for "xen/common/monitor.c", so the
> > pattern works for this particular file.
> 
> ... together with this observation, I take it that
> 
> 	   F:	*/net/*		all files in "any top level directory"/net
> 
> is actually at best misleading / ambiguous - I read it as not just a single
> level of directories, but it may well be that that's what is meant. At

I guess the ambiguity would lie in the word "files". Here, "files" is a
single file and not a directory, unlike the shell globing which would
include directories with a '*'.

The first '*' is described at "any top level directory", but it is also
"only top level directory". This kind of tells me that there is only a
single level of directories that is match by '*'.

> which point the question is how "any number of directories" could be
> expressed. Would **/ or .../**/... work here? I'm afraid my Perl is far
> from sufficient to actually spot where (and hence how) this is handled in
> the script.

I think you could write a regexp with the "N:" type instead of "F:".
This is described Linux's MAINTAINERS file, but not ours, yet our
get_maintainer.pl script has the functionality. It might be nice to be
able to write just '**' but until someone implement that, we could go
for a regex, which is more complicated and more prone to mistake.

So I think in the short-term, you want:

N:	^xen/.*/mem_access\.[ch]$
N:	^xen/.*/monitor\.[ch]$
N:	^xen/.*/vm_event\.[ch]$

As for adding "**", there's maybe something to do with
"file_match_pattern()" in get_maintainer.pl, this function compare the
number of '/' in both the pattern and the filepath to find out if a '*'
only match one level of directory or more.

Cheers,

-- 
Anthony PERARD
Re: [PATCH v2] MAINTAINERS: consolidate vm-event/monitor entry
Posted by Jan Beulich 7 months, 3 weeks ago 
On 06.09.2023 16:45, Anthony PERARD wrote:
> On Wed, Sep 06, 2023 at 02:50:22PM +0200, Jan Beulich wrote:
>> On 06.09.2023 14:40, Anthony PERARD wrote:
>>> On Thu, Aug 31, 2023 at 08:15:13AM +0200, Jan Beulich wrote:
>>>> If the F: description is to be trusted, the two xen/arch/x86/hvm/
>>>> lines were fully redundant with the earlier wildcard ones. Arch header
>>>> files, otoh, were no longer covered by anything as of the move from
>>>> include/asm-*/ to arch/*/include/asm/. Further also generalize (by
>>>> folding) the x86- and Arm-specific mem_access.c entries.
>>>>
>>>> Finally, again assuming the F: description can be trusted, there's no
>>>> point listing arch/, common/, and include/ entries separately. Fold
>>>> them all.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> -F:	xen/arch/*/monitor.c
>>>> -F:	xen/arch/*/vm_event.c
>>>> -F:	xen/arch/arm/mem_access.c
>>>> -F:	xen/arch/x86/include/asm/hvm/monitor.h
>>>> -F:	xen/arch/x86/include/asm/hvm/vm_event.h
>>>> -F:	xen/arch/x86/mm/mem_access.c
>>>> -F:	xen/arch/x86/hvm/monitor.c
>>>> -F:	xen/arch/x86/hvm/vm_event.c
>>>> -F:	xen/common/mem_access.c
>>>> -F:	xen/common/monitor.c
>>>> -F:	xen/common/vm_event.c
>>>> -F:	xen/include/*/mem_access.h
>>>> -F:	xen/include/*/monitor.h
>>>> -F:	xen/include/*/vm_event.h
>>>> +F:	xen/*/mem_access.[ch]
>>>> +F:	xen/*/monitor.[ch]
>>>> +F:	xen/*/vm_event.[ch]
>>>
>>>
>>> Hi,
>>>
>>> Did you mean to for example change the maintainer ship of
>>> "xen/arch/x86/mm/mem_access.c"? Before it was:
>>>     - VM EVENT, MEM ACCESS and MONITOR
>>>     - X86 MEMORY MANAGEMENT
>>>     - X86 ARCHITECTURE
>>> And now, it's just:
>>>     - X86 MEMORY MANAGEMENT
>>>     - X86 ARCHITECTURE
>>>
>>> (see ./scripts/get_maintainer.pl --sections -f xen/arch/x86/mm/mem_access.c)
>>>
>>> Also, now "xen/include/xen/monitor.h" is only "THE REST".
>>
>> No, no change of maintainership was intended. But there was an uncertainty,
>> which is why I said "assuming the F: description can be trusted". So ...
>>
>>> On the other hand, there's no change for "xen/common/monitor.c", so the
>>> pattern works for this particular file.
>>
>> ... together with this observation, I take it that
>>
>> 	   F:	*/net/*		all files in "any top level directory"/net
>>
>> is actually at best misleading / ambiguous - I read it as not just a single
>> level of directories, but it may well be that that's what is meant. At
> 
> I guess the ambiguity would lie in the word "files". Here, "files" is a
> single file and not a directory, unlike the shell globing which would
> include directories with a '*'.
> 
> The first '*' is described at "any top level directory", but it is also
> "only top level directory". This kind of tells me that there is only a
> single level of directories that is match by '*'.
> 
>> which point the question is how "any number of directories" could be
>> expressed. Would **/ or .../**/... work here? I'm afraid my Perl is far
>> from sufficient to actually spot where (and hence how) this is handled in
>> the script.
> 
> I think you could write a regexp with the "N:" type instead of "F:".
> This is described Linux's MAINTAINERS file, but not ours, yet our
> get_maintainer.pl script has the functionality. It might be nice to be
> able to write just '**' but until someone implement that, we could go
> for a regex, which is more complicated and more prone to mistake.
> 
> So I think in the short-term, you want:
> 
> N:	^xen/.*/mem_access\.[ch]$
> N:	^xen/.*/monitor\.[ch]$
> N:	^xen/.*/vm_event\.[ch]$

Except that as per Linux'es description F: and N: differ beyond how
file names are expressed / matched. I don't think I want to be the
one to introduce something which I've been complaining about on the
Linux side (people being Cc-ed just because at some point they ended
up touching a file).

I conclude that for the time being I ought to revert that change of
mine.

Jan

> As for adding "**", there's maybe something to do with
> "file_match_pattern()" in get_maintainer.pl, this function compare the
> number of '/' in both the pattern and the filepath to find out if a '*'
> only match one level of directory or more.
> 
> Cheers,
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.