Re: [Xen-devel] [PATCH 1/3] gnttab: Drop the frame parameter from acquire_grant_for_copy()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/08/17 11:13, Jan Beulich wrote:
>>>> On 24.08.17 at 19:55, <andrew.cooper3@xxxxxxxxxx> wrote:
>> It is redundant with the *page parameter.  Rename the grant_frame parameter 
>> to
>> indicate that it is local, and highlight the correctness of the change.
> I don't understand this second sentence: For one, grant_frame isn't
> and wasn't a parameter (but a local variable), and then I also don't
> see what is being highlighted to validate the correctness. Or am I
> being mislead by it not reading "and to highlight ..."? So far I was
> believing that such an omission is okay in German, but not in English.

Taking grant_frame out entirely at this point (is what I did originally,
but) does not make the patch read as obviously-safe.  Changing its name
causes the compiler to help spot all uses.

>
>> @@ -2238,10 +2240,9 @@ acquire_grant_for_copy(
>>          active_entry_release(act);
>>          grant_read_unlock(rgt);
>>  
>> -        rc = acquire_grant_for_copy(td, trans_gref, rd->domain_id,
>> -                                    readonly, &grant_frame, page,
>> -                                    &trans_page_off, &trans_length,
>> -                                    false);
>> +        rc = acquire_grant_for_copy(
>> +            td, trans_gref, rd->domain_id, readonly, page, &trans_page_off,
>> +            &trans_length, false);
> As hinted at by Wei, I'd prefer if you didn't alter the style to the
> uglier variant - we really should use that only if the commonly
> used one really gets unwieldy, which imo is not the case here.

Ugly? What is ugly (in general) is squashing all parameters together on
many lines on the RHS.

In this case, the false on the end also interacts with my command line
parameter patch, where it becomes opt_transitive_grants, and gets in the
way of the previous alignment strategy.

~Andrew

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