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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.14 at 12:32, <tim@xxxxxxx> wrote:
> 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()?

Fine with me. As to the type change, I think it is better to have the
function have an actual rc, forwarded from p2m_set_entry() or set
to -EBUSY when the type didn't match. That way the different
failure modes will be distinguishable at the call sites (eventually
useful for debugging as well as a decision towards a retry at the
original caller level).

Jan


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