Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU <czuzu@bitdefender.com>]

On 2/10/2016 6:26 PM, Jan Beulich wrote:
>>>> On 10.02.16 at 16:52, <czuzu@bitdefender.com> wrote:
>>   xen/arch/x86/Kconfig                              |   4 +
>>   xen/arch/x86/Makefile                             |   2 +-
>>   xen/arch/x86/hvm/event.c                          |   2 +-
>>   xen/arch/x86/hvm/hvm.c                            |   2 +-
>>   xen/arch/x86/hvm/vmx/vmx.c                        |   2 +-
>>   xen/arch/x86/monitor_x86.c                        |  72 ++++++++
>>   xen/common/Kconfig                                |  20 +++
>>   xen/common/Makefile                               |   1 +
>>   xen/common/domctl.c                               |   2 +-
>>   xen/{arch/x86 => common}/monitor.c                | 195 +++++++++-------------
>>   xen/include/asm-arm/{monitor.h => monitor_arch.h} |  34 +++-
>>   xen/include/asm-x86/monitor_arch.h                |  74 ++++++++
>>   xen/include/{asm-x86 => xen}/monitor.h            |  17 +-
>>   13 files changed, 293 insertions(+), 134 deletions(-)
>>   create mode 100644 xen/arch/x86/monitor_x86.c
>>   rename xen/{arch/x86 => common}/monitor.c (44%)
>>   rename xen/include/asm-arm/{monitor.h => monitor_arch.h} (46%)
>>   create mode 100644 xen/include/asm-x86/monitor_arch.h
>>   rename xen/include/{asm-x86 => xen}/monitor.h (74%)
> With percentages as low as 44 I'm not sure all this strange
> renaming and introduction of oddly named new files is actually a
> good idea.
The diff would have actually looked a lot worse if I didn't do that, at 
least IMO.
The "strange renaming and ..." was an effort I made to make reviewing these
patches easier :). The reason is explained in the introductory message 
and in this commit
message.
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -14,6 +14,10 @@ config X86
>>   	select HAS_MEM_ACCESS
>>   	select HAS_MEM_PAGING
>>   	select HAS_MEM_SHARING
>> +	select HAS_VM_EVENT_WRITE_CTRLREG
>> +	select HAS_VM_EVENT_SINGLESTEP
>> +	select HAS_VM_EVENT_SOFTWARE_BREAKPOINT
>> +	select HAS_VM_EVENT_GUEST_REQUEST
>>   	select HAS_NS16550
>>   	select HAS_PASSTHROUGH
>>   	select HAS_PCI
> Please don't break the alphabetic ordering.

Noted. Didn't realise they were in alphabetic order.

> +#include <xen/config.h> /* for XENLOG_WARNING */
> Please simply drop this include, it's being automatically included via
> compiler command line option. Also please avoid comments like this
> unless they explain an otherwise unexpected or unreasonable
> inclusion.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Noted, noted.

That you,
Corneliu.