Re: [Xen-devel] [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.18 at 13:28, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Mar 09, 2018 at 01:18:42PM +0000, Andrew Cooper wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1187,8 +1187,8 @@ unsigned long domain_get_maximum_gpfn(struct domain *d)
>>      return gfn_x(d->arch.p2m.max_mapped_gfn);
>>  }
>>  
>> -void share_xen_page_with_guest(struct page_info *page,
>> -                          struct domain *d, int readonly)
>> +void share_xen_page_with_guest(struct page_info *page, struct domain *d,
>> +                               enum XENSHARE_flags flags)
> 
> Naming this _flags feels wrong to me, I would assume flags to be
> something which can be used as (SHARE_r | SHARE_w) (ie: stacked) and
> so on. I would maybe name this XENSHARE_options rather than flags.
> 
> TBH I would be OK with renaming the parameter to "bool ro/readonly"
> and let the callers use true and false directly. It seems like
> over-engineering to use an enum for this, or maybe you have further
> changes in mind that are going to expand the set of options?

On one hand I agree that an enum like this is somewhat strange
to have, and a boolean would seem like a better fit. Otoh using
plain true/false at the call sites would make it pretty unclear
whether "true" means r/o or r/w. So another option might be
to have multiple inline wrappers around the actual worker, like
share_xen_page_with_guest_ro().

Nevertheless the x86 parts of the patch can also have
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
as they currently are.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel