Re: [Xen-devel] [PATCH V2 0/9] xentrace/xenalyze support on ARM

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/04/16 16:50, Ben Sanda wrote:
> Julien and Wei,
>
>>> You patches all have the same subject line.  Please make them more 
>>> specific. See my reply to #1 for example.
>> +1
>>
>> Also, you should at least check that Xen still builds after applying
>> each patch. Ideally, you also need to be careful to not break any
>> feature currently supported. It's useful when someone needs to 
>> bisect the tree.
>>
>> For instance, you use the function get_pg_owner for ARM in patch #2 
>> but introduce the function in patch #4. This will break ARM build. 
>> So the patch #2 should be moved after #4.
>>
>> Furthermore, you remove the functions get_pg_owner and put_pg_owner 
>> for x86 in patch #3 and then re-introduced them in patch #4. 
>> Therefore, the x86 will be broken after #3. In this case, it's better
>> to have a patch that move the 2 functions from x86 to common.
> Thank you for the comments. I apologize for the errors in the patch
> format. This is my first time submitting a patch to Xen and I was
> unaware that the patch set order mattered or that I had to account for
> a piecewise application of the patch set. I will attempt to resubmit
> with this corrected and the patch names updated. 
>
> So then it is permissible to have multiple file changes in one
> patch/commit? E.g. a patch that removes from one file and  adds to
> another in the same commit. I initially thought each patch/ commit was
> only supposed to modify one file and that's why I did it that way

One logical change per patch (wherever possible).  It is fine to change
more than one file in a patch.

In particular, your patches 3 though 5 are one logical change, "move
get_pg_owner() from being x86-specific to being common".

It is an absolute must that each invidiual patch compiles and doesn't
regress functionality, so the resulting patches can be bisected in the
case of an error being introduced.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel