[WESTON PATCH 2/2] compositor-drm: Watchdog timer implementation

[Frederic Plourde]

On 14-10-29 02:45 PM, Bill Spitzak wrote:
> On 10/29/2014 11:13 AM, Frederic Plourde wrote:
>
>> This watchdog timer (WDT) implementation is software only and includes
>> basic features usually found in a WDT. We simply exit Weston gracefully
>> with an exit code when the dog barks.
>>
>> The watchdog timeout value can be set via weston.ini by adding a
>> watchdog-timer-timeout=<SECONDS> entry under a new [compositor-drm]
>> section. Setting this timeout to 0 disables the watchdog feature.
>
>> +/* Creates a watchdog timer. Note that timer is not armed by default */
>> +static int
>> +drm_output_watchdog_timer_create(struct watchdog_timer *wdt, struct 
>> wl_display *display,
>> +                      wl_event_loop_timer_func_t timeout_func, void 
>> *timer_arg,
>> +                      uint32_t timeout)
>> +{...
>
> I think it would be better to remove all these arguments and just make 
> this function directly use the values that are being passed to it now. 
> Right now it is a bit hard to find what function is called on timeout 
> because I have to locate the call to this function. Also this should 
> print to the log the error rather than the caller doing so.

Mhh... you mean something like :

static int
drm_output_watchdog_timer_create(struct drm_output *output)
{
     struct wl_event_loop *loop = NULL;
     struct weston_compositor *ec = output_base->compositor;

     loop = wl_display_get_event_loop(ec->wl_display);
     if (loop) {
         output->wdt->timer = wl_event_loop_add_timer(loop,
                                         drm_output_watchdog_bark, 
ec->watchdog_timer_timeout);
     blah..blah...

?

If so, then I think I'd prefer to completely remove 
'drm_output_watchdog_timer_create" and inline it directly from 
'create_output_for_connector', since it's not a very big function anyway 
and we're not sure as of today that we're going to reuse this for 
anything else.

In that case, I'd also remove 'drm_output_watchdog_timer_destroy' and 
inline it in 'drm_output_destroy'

what do you think ?
I'm usually not a big fan of static functions with hard-coded values, 
but if you tell me it's better for clarity... I might be tempted, dunno.
>
>> +/* Start/reset/arm the dog's timer. This can also be used to 'pat' 
>> the dog */
>> +static int
>> +drm_output_watchdog_timer_start(struct watchdog_timer *wdt)
>> +{
>> +    if (wdt->timer)
>> +        return wl_event_source_timer_update(wdt->timer, wdt->timeout);
>> +
>> +    return -1;
>> +}
>
> Instead of the caller doing the logging I would do it here. You could 
> skip the log message if it failed to create the timer earlier since 
> that would have printed a message.
fair.
>
> IMHO this is a better method for making these functions as long as 
> they are static and not intended to be reused.
>
> Also wondering if weston_compositor_exit_with_code could be avoided by 
> fixing things to use atexit and then directly calling exit(). Or at 
> least fix it up enough that when you want to exit with an error it is 
> ok to skip the cleanup code that was missed.
I'd like to hear Pekka on that one ;-)
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel