[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 7/7] xen/mm: Clean up share_xen_page_with_guest() API
On 16/03/2018 07:43, Jan Beulich wrote: >>>> On 15.03.18 at 21:25, <andrew.cooper3@xxxxxxxxxx> wrote: >> On 13/03/18 14:39, Jan Beulich wrote: >>>>>> 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(). >> Splitting into (SHARE_r | SHARE_w( doesn't make sense because the >> underlying implementation take a boolean idea of whether to use PGT_none >> or PGT_writable_page. >> >> We've already got share_xen_page_with_privileged_guests() as a wrapper >> around share_xen_page_with_guest(). Therefore, we'd end up with a total >> of 4 extra wrappers if we wanted _rw and _ro suffixes, which seems over >> the top to me. >> >> I agree its not completely great like this, but it is the least bad >> option I managed to come up with. > Well, without wanting put under question the ack I've already > given, the question of course is whether the code is much > better after the change than it was before. If there's no really > good shape to put this in, leaving things as they are is certainly > also an option. The code is in better shape with the change, than without it (common prototypes, share_xen_page_with_privileged_guests() being a wrapper, fewer linebreaks, etc). I just can't see a way of getting it into a yet-better state. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |