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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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?

Jan


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