Re: [Xen-devel] [PATCH v7 1/2] xen: export do_block, add a parameter to enable local events delivery

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 19/04/2013 18:12, "Keir Fraser" <keir.xen@xxxxxxxxx> wrote:

> On 19/04/2013 17:23, "Stefano Stabellini" <stefano.stabellini@xxxxxxxxxxxxx>
> wrote:
> 
>> On Fri, 19 Apr 2013, Jan Beulich wrote:
>>>>>> On 19.04.13 at 16:06, Stefano Stabellini
>>>>>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
>>>> -static long do_block(void)
>>>> +int do_block(int event_delivery_enable)
>>> 
>>> In the previous version you didn't need this new parameter, so
>>> what changed since then?
>> 
>> I changed the ARM implementation of local_event_delivery_enable: now it
>> clears PSR_IRQ_MASK (the interrupt flag). That is the correct behavior
>> for the implementation of the SCHEDOP_block hypercall but it is not the
>> right behavior for the WFI trap handler. In order to reuse this code, I
>> had to add the event_delivery_enable parameter.
> 
> Given that basically you need special versions of both local_events_*()
> calls in do_block() (in one case you nop it out, and in the other it is a
> special case which ignores the irq mask), I think perhaps you are better
> with your own version of do_block() rather than code sharing. It's only a
> small function and it skanks up every caller in common code, and you're
> practically abusing do_block() by trying to play nicely with it.
> 
> I would also suggest that _local_events_need_delivery() takes no parameter,
> always ignores the mask, and instead push the mask check into
> local_events_need_delivery(). Perhaps rename the former to
> local_events_need_delivery_ignore_mask() or something.

Both these suggestions by the way are partly driven by my fairly strong
hatred of boolean parameters to functions. I hate those 1/0 true/false
arguments, almost always have to go look up what they mean because they're
often the sign of some gross hack or special case 'mode change' for a
function. This is arguable of course, my hatred is probably irrational to
some extent, but I my bad-code senses tingle when I see such parameters! :)

 -- Keir

>  -- Keir
> 
>>> Also, please use bool_t for boolean parameters.
>> 
>> OK
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel
> 
> 



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