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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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.

> @@ -2255,6 +2256,8 @@ acquire_grant_for_copy(
>              return rc;
>          }
>  
> +        frame = page_to_mfn(*page);
> +
>          /*
>           * We dropped the lock, so we have to check that the grant didn't
>           * change, and that nobody else tried to pin/unpin it. If anything
> @@ -2262,7 +2265,7 @@ acquire_grant_for_copy(
>           */
>          if ( rgt->gt_version != 2 ||
>               act->pin != old_pin ||
> -             (old_pin && (act->domid != ldom || act->frame != grant_frame ||
> +             (old_pin && (act->domid != ldom || act->frame != frame ||
>                            act->start != trans_page_off ||
>                            act->length != trans_length ||
>                            act->trans_domain != td ||
> @@ -2286,7 +2289,7 @@ acquire_grant_for_copy(
>              act->length = trans_length;
>              act->trans_domain = td;
>              act->trans_gref = trans_gref;
> -            act->frame = grant_frame;
> +            act->frame = frame;

These three get re-done in patch 2 - I think you could easily bring
them into their final shape right away, proving the correctness of
the change by reducing the scope of frame to ...

> @@ -2310,7 +2313,7 @@ acquire_grant_for_copy(
>          {
>              unsigned long gfn = shared_entry_v1(rgt, gref).frame;
>  
> -            rc = get_paged_frame(gfn, &grant_frame, page, readonly, rd);
> +            rc = get_paged_frame(gfn, &frame, page, readonly, rd);

... the outer if() this and the following hunks are contained in.

Jan


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