| Message ID | 1455119657-2494-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote: > Rename: > - arch/x86/monitor_x86.c -> arch/x86/monitor.c > - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h > > (previous commit explains why these renames were necessary) > Referencing a "previous commit" like this is not acceptable as you don't know how these patches will get applied and there might be other patches that get committed in-between yours. In each patch explain clearly why the patch is needed. I still find it odd that you need to rename x86/monitor.c -> x86/monitor_x86.c -> x86/monitor.c in the same series. > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> > --- >
>>> On 10.02.16 at 17:44, <tamas@tklengyel.com> wrote: > On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com> > wrote: > >> Rename: >> - arch/x86/monitor_x86.c -> arch/x86/monitor.c >> - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h >> >> (previous commit explains why these renames were necessary) >> > > Referencing a "previous commit" like this is not acceptable as you don't > know how these patches will get applied and there might be other patches > that get committed in-between yours. In each patch explain clearly why the > patch is needed. I still find it odd that you need to rename x86/monitor.c > -> x86/monitor_x86.c -> x86/monitor.c in the same series. Indeed. Thinking about it again perhaps the operations should be "move to common/ (mostly) verbatim" followed by "break out x86 specific code" (if that's the smaller portion compared to what is to stay). Jan
On 2/10/2016 6:44 PM, Tamas K Lengyel wrote: > > > On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> wrote: > > Rename: > - arch/x86/monitor_x86.c -> arch/x86/monitor.c > - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h > > (previous commit explains why these renames were necessary) > > > Referencing a "previous commit" like this is not acceptable as you > don't know how these patches will get applied and there might be other > patches that get committed in-between yours. In each patch explain > clearly why the patch is needed. I still find it odd that you need to > rename x86/monitor.c -> x86/monitor_x86.c -> x86/monitor.c in the same > series. > > > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com > <mailto:czuzu@bitdefender.com>> > --- > As I stated earlier, those commits (4 & 6) are the result of a git diff limitation (also see my responses to these patches in v1). The following simple experiment illustrates my point precisely: 1) create a local git repo, add file ./a.txt there with many lines (e.g. 100) and make that commit 1 2) now create a dir ./d, move a.txt -> ./d/a.txt and add another ./a.txt w/ few lines, e.g. 2, make that commit 2 Git diff (even w/ the -M, -C options) of commit 2 would then report that: # ./a.txt has been modified => diff w/ 100 deletions & 2 additions # ./d/a.txt has been added => diff w/ 100 additions => 202 additions & deletions total, *even though* similarity between ./a.txt in commit 1 and ./d/a.txt in commit 2 is 100% ! (also even if you use -M, -C diff options) *instead of* reporting # ./a.txt moved to ./d/a.txt, similarity index 100% => 0 deletions & 0 additions # added ./a.txt => 2 additions => 2 additions total You could see why this limitation would have made the diffs somewhat bulky, especially when those files also get to be modified in the process. This is also the reason why accepting patch 3 without 4 or 5 without 6 wouldn't make sense, 3+4 should be treated as a single patch, 5+6 similarly. As a proposed resolution: since I took these measures *only to ease code review*, patches 4 & 6 can be safely squashed onto their previous commits (3 & 5) when and if acking them. Or I could just worry less about this next time and simply squash them myself before sending them for review, less to do on my part :). Corneliu.
On 2/10/2016 7:16 PM, Jan Beulich wrote: >>>> On 10.02.16 at 17:44, <tamas@tklengyel.com> wrote: >> On Wed, Feb 10, 2016 at 8:54 AM, Corneliu ZUZU <czuzu@bitdefender.com> >> wrote: >> >>> Rename: >>> - arch/x86/monitor_x86.c -> arch/x86/monitor.c >>> - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h >>> >>> (previous commit explains why these renames were necessary) >>> >> Referencing a "previous commit" like this is not acceptable as you don't >> know how these patches will get applied and there might be other patches >> that get committed in-between yours. In each patch explain clearly why the >> patch is needed. I still find it odd that you need to rename x86/monitor.c >> -> x86/monitor_x86.c -> x86/monitor.c in the same series. > Indeed. Thinking about it again perhaps the operations should be > "move to common/ (mostly) verbatim" followed by "break out x86 > specific code" (if that's the smaller portion compared to what is to > stay). > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > That's a great alternative! Will do so in v3. Corneliu.
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 6e80cf0..8e6e901 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -36,7 +36,7 @@ obj-y += microcode_intel.o # This must come after the vendor specific files. obj-y += microcode.o obj-y += mm.o x86_64/mm.o -obj-y += monitor_x86.o +obj-y += monitor.o obj-y += mpparse.o obj-y += nmi.o obj-y += numa.o diff --git a/xen/arch/x86/monitor_x86.c b/xen/arch/x86/monitor.c similarity index 97% rename from xen/arch/x86/monitor_x86.c rename to xen/arch/x86/monitor.c index d19fd15..568def2 100644 --- a/xen/arch/x86/monitor_x86.c +++ b/xen/arch/x86/monitor.c @@ -1,5 +1,5 @@ /* - * arch/x86/monitor_x86.c + * arch/x86/monitor.c * * Arch-specific monitor_op domctl handler. * @@ -19,7 +19,7 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <asm/monitor_arch.h> +#include <asm/monitor.h> bool_t arch_monitor_domctl_event(struct domain *d, struct xen_domctl_monitor_op *mop, diff --git a/xen/common/monitor.c b/xen/common/monitor.c index a4899c3..03063bb 100644 --- a/xen/common/monitor.c +++ b/xen/common/monitor.c @@ -25,7 +25,7 @@ #include <xsm/xsm.h> #include <public/domctl.h> -#include <asm/monitor_arch.h> /* for monitor_arch_# */ +#include <asm/monitor.h> /* for monitor_arch_# */ #if CONFIG_X86 #include <public/vm_event.h> /* for VM_EVENT_X86_CR3 */ #include <asm/hvm/hvm.h> /* for hvm_update_guest_cr, ... */ diff --git a/xen/include/asm-arm/monitor_arch.h b/xen/include/asm-arm/monitor.h similarity index 90% rename from xen/include/asm-arm/monitor_arch.h rename to xen/include/asm-arm/monitor.h index d0df66c..eb770da 100644 --- a/xen/include/asm-arm/monitor_arch.h +++ b/xen/include/asm-arm/monitor.h @@ -1,5 +1,5 @@ /* - * include/asm-arm/monitor_arch.h + * include/asm-arm/monitor.h * * Arch-specific monitor_op domctl handler. * @@ -19,8 +19,8 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#ifndef __ASM_ARM_MONITOR_ARCH_H__ -#define __ASM_ARM_MONITOR_ARCH_H__ +#ifndef __ASM_ARM_MONITOR_H__ +#define __ASM_ARM_MONITOR_H__ #include <xen/sched.h> #include <public/domctl.h> @@ -50,4 +50,4 @@ bool_t arch_monitor_domctl_event(struct domain *d, return 0; } -#endif /* __ASM_ARM_MONITOR_ARCH_H__ */ +#endif /* __ASM_ARM_MONITOR_H__ */ diff --git a/xen/include/asm-x86/monitor_arch.h b/xen/include/asm-x86/monitor.h similarity index 94% rename from xen/include/asm-x86/monitor_arch.h rename to xen/include/asm-x86/monitor.h index d9daf65..b12823c 100644 --- a/xen/include/asm-x86/monitor_arch.h +++ b/xen/include/asm-x86/monitor.h @@ -1,5 +1,5 @@ /* - * include/asm-x86/monitor_arch.h + * include/asm-x86/monitor.h * * Arch-specific monitor_op domctl handler. * @@ -19,8 +19,8 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ -#ifndef __ASM_X86_MONITOR_ARCH_H__ -#define __ASM_X86_MONITOR_ARCH_H__ +#ifndef __ASM_X86_MONITOR_H__ +#define __ASM_X86_MONITOR_H__ #include <xen/sched.h> /* for struct domain, is_hvm_domain, ... */ #include <public/domctl.h> /* for XEN_DOMCTL_MONITOR_#, ... */ @@ -71,4 +71,4 @@ bool_t arch_monitor_domctl_event(struct domain *d, struct xen_domctl_monitor_op *mop, int *rc); -#endif /* __ASM_X86_MONITOR_ARCH_H__ */ +#endif /* __ASM_X86_MONITOR_H__ */
Rename: - arch/x86/monitor_x86.c -> arch/x86/monitor.c - asm-{x86,arm}/monitor_arch.h -> asm-{x86,arm}/monitor.h (previous commit explains why these renames were necessary) Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> --- xen/arch/x86/Makefile | 2 +- xen/arch/x86/{monitor_x86.c => monitor.c} | 4 ++-- xen/common/monitor.c | 2 +- xen/include/asm-arm/{monitor_arch.h => monitor.h} | 8 ++++---- xen/include/asm-x86/{monitor_arch.h => monitor.h} | 8 ++++---- 5 files changed, 12 insertions(+), 12 deletions(-) rename xen/arch/x86/{monitor_x86.c => monitor.c} (97%) rename xen/include/asm-arm/{monitor_arch.h => monitor.h} (90%) rename xen/include/asm-x86/{monitor_arch.h => monitor.h} (94%)