Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.07.15 at 10:52, <tiejun.chen@xxxxxxxxx> wrote:
> On 2015/7/22 16:28, Jan Beulich wrote:
>>>>> On 22.07.15 at 03:30, <tiejun.chen@xxxxxxxxx> wrote:
>>> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>>> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>>> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>>> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
>>> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>>> ---
>>> v11:
>>>
>>> * Use GCNEW_ARRAY to replace libxl__malloc()
>>>
>>> * #define pfn_to_paddrk is missing safety () around x, and
>>>    move this into libxl_internal.h
>>>
>>> * Rename set_rdm_entries() to add_rdm_entry() and put the
>>>    increment at the end so that the assignments are
>>>    to ->rdms[d_config->num_rdms].
>>>
>>> * "Simply make it so that if there are any rdms specified
>>>    in the domain config, they are used instead of the
>>>    automatically gathered information (from strategy and
>>>    devices)." So just return if d_config->rmds is valid.
>>>
>>> * Shorten some code comments.
>>
>> I think it is not the first time that we're pointing out to you that
>> when you make not just cosmetic changes, review and ack tags
>> should be dropped.
> 
> I don't recall this sort of requirement was mentioned. Instead, this is 
> new to me. So where can I found this warning you said previously?

Even if I misremember and didn't send such a request your way, if
you read xen-devel you'd have seen this quite a number of times.

> Furthermore, you ask me to drop Reviewed-by/Acked-by in this revision, 
> what's next? Just to this example,
> 
> No.1 revision:
> 
> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> No.2 revision:
> 
> I addressed some comments raised by Jackson. But you mean 
> Reviewed-by/Acked-by should be dropped.

Not unilaterally - it depends on the nature of the changes and
the area of code the tag was offered for (I'd tend to say that
reviews normally cover the whole patch, but acks may have
limited meaning and hence may be retained in a wider set of
cases).

> No.3 revision:
> 
> I assume Jackson Ack or Review to this so I should leave one line like this,
> 
> Reviewed-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> without two previous Acked-by/Reviewed-by, right? So looks like the 
> latter always override the former, right?
> 
> And I also can't understand why we should drop Reviewed-by/Acked-by from 
> other guys. And, all new comments I addressed don't conflict with our 
> previous revision so why?

In the case at hand you even had to adjust the algorithm (whether
and how to honor incoming data). I.e. the question isn't whether
there's a conflict, but whether there's any adjustment that may,
from an abstract pov, result in the person having offered such a
tag to now possibly have a different opinion. You have to face it -
(s)he may be of the opinion that the change was wrong (reviewers
make mistakes just like contributors do) or the suggestion wrongly
carried out.

Jan


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