Re: [Xen-devel] [PATCH] x86/P2M: pass on errors from p2m_set_entry()

[Tim Deegan <tim@xxxxxxx>]

At 11:19 +0100 on 02 May (1399025964), Jan Beulich wrote:
> >>> On 01.05.14 at 15:02, <tim@xxxxxxx> wrote:
> > At 11:49 +0100 on 25 Apr (1398422989), Jan Beulich wrote:
> >> @@ -719,8 +719,9 @@ p2m_type_t p2m_change_type(struct domain
> >>      gfn_lock(p2m, gfn, 0);
> >>  
> >>      mfn = p2m->get_entry(p2m, gfn, &pt, &a, 0, NULL);
> >> -    if ( pt == ot )
> >> -        p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, nt, 
> >> p2m->default_access);
> >> +    if ( pt == ot &&
> >> +         p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, nt, 
> >> p2m->default_access) 
> > )
> >> +        pt = p2m_invalid;
> > 
> > While I can see we want to do something on error here, think this
> > is a bit weird.  It would be better just to make this function return
> > bool, since every caller just tests the result for ==ot anyway.
> > (Well, the HVMOP_set_mem_type hanlder printks it but I don't think it's
> > that helpful.)
> 
> I can certainly do that, but wouldn't your return-type-changes
> concern you (imo validly) had on Mukesh's recent changes then
> here apply too, i.e. shouldn't we rename the function? (I might
> have done the rename right away, if only I could see a good
> replacement name.)

Oh - I had thought that the different return type would make the
compile fail but of course the implicit cast bool_t (-> int) -> p2m_type_t
works fine. :(

So yes, we should rename it.  How about p2m_change_type_one()
to contrast with p2m_change_type_range()?

> Also, regarding that printk: Now that it would need altering
> anyway, do you think it's of much as a whole (your comment left
> open whether you think printing the new type isn't very helpful, or
> the entire thing)? I.e. shouldn't we rather drop it instead of
> adjusting it?

Yes, I think we can drop it, or at least downgrade it from
XENLOG_WARNING.  The same goes for the p2m_is_grant() printk just
above.

Cheers,

Tim.

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