Re: [Qemu-devel] [PATCH qom-next 01/12] store prev_debug_excp_handler globaly and not per target

[Jan Kiszka]

On 2012-05-30 15:35, Igor Mammedov wrote:
> On 05/30/2012 12:51 PM, Jan Kiszka wrote:
>> On 2012-05-30 00:10, Igor Mammedov wrote:
>>> current callers all do the same thing, storing in prev_debug_excp_handler
>>> previous handler and then calling it in breakpoint_handler.
>>> Move prev_debug_excp_handler from local scope to global and make
>>> cpu_set_debug_excp_handler() always to store previous handler.
>>
>> NAK, this is not a beautiful interface, exposing the previous handler
>> via global variable. And it prevents chaining of handlers.
>>
>> Is there a technical reason for this refactoring?
> 
> current 2 users use prev_debug_excp_handler in the same manner i.e. doing
>          prev_debug_excp_handler =
>              cpu_set_debug_excp_handler(breakpoint_handler);
> so consolidating it in one place looks better than writing the same code 
> per-target.

As pointed out, this changes the API in disallowing chains of exception
handlers. Either drop this feature (it's currently unused) or leave the
API as it is. Introducing a global variable is not an acceptable API.

> In addition when chaining more then current and previous handlers would be 
> needed,
> the global var prev_debug_excp_handler could be replaced by global call
> prev_debug_excp_handler() without affecting per-target code and it could be
> implemented in cpu-exec.c or even doing chaining completely internally in 
> cpu-exec.c
> without breakpoint_handler() being aware of it.

Users would be different subsystems, and they would use different
variables to store and call the previous callback in the chain. That was
the idea of this API.

However, it is probably simpler to make it a non-chained callback for
now and rethink the design once a use case shows up.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux