[systemd-devel] [PATCH v2] watchdog: Don't require WDIOC_SETOPTIONS/WDIOS_ENABLECARD

[Jean Delvare]

On Wed, 17 Jun 2015 12:02:27 +0200, Lennart Poettering wrote:
> On Wed, 17.06.15 11:26, Jean Delvare (jdelvare at suse.de) wrote:
> 
> > Not all watchdog drivers implement WDIOC_SETOPTIONS. Drivers which do
> > not implement it have their device always enabled. So it's fine to
> > report an error if WDIOS_DISABLECARD is passed and the ioctl is not
> > implemented, however failing when WDIOS_ENABLECARD is passed and the
> > ioctl is not implemented is not good: if the device was already
> > enabled then WDIOS_ENABLECARD was a no-op and wasn't needed in the
> > first place. So we can just ignore the error and continue.
> > ---
> > Lennart, would that be OK with you?
> 
> Generally yes.
> 
> But: indentation is borked, there some whitespace issues, I cannot
> even apply this. 
> 
> Please have a look at CODING_STYLE.

Oops, sorry about that. I had read CODING_STYLE, and paid attention for
the first submission, but then forgot to double-check the indentation
after doing the requested changes. All other projects I'm working on
use tabs, not spaces, so that's my editor does unless I fix it manually.

> Also, please build the stuff locally before commit and send to the
> ML. We have a git commit hook that checks for weird whitespace at time
> of commit. It's enables as soon as you run "autogen.sh" once...

I always test-build. But I won't get to commit the patch myself, which
is why I did not get the warning.

> > Changes since v1:
> >  * Log WDIOC_SETOPTIONS/WDIOS_ENABLECARD failure at debug level if
> >    ENOTTY is returned. Suggested by Lennart Poettering.
> > 
> >  src/shared/watchdog.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > --- systemd.orig/src/shared/watchdog.c	2015-06-15
> > 18:51:40.811989627 +0200 +++ systemd/src/shared/watchdog.c
> > 2015-06-17 11:20:01.798331790 +0200 @@ -60,8 +60,13 @@ static int
> > update_timeout(void) { 
> >                  flags = WDIOS_ENABLECARD;
> >                  r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, &flags);
> > -                if (r < 0)
> > -                        return log_warning_errno(errno, "Failed to
> > enable hardware watchdog: %m");
> > +                if (r < 0) {
> > +	                /* ENOTTY means the watchdog is always
> > enabled so we're fine */
> 
> multiples of 8 space indenting, please.
> 
> > +                        log_full(errno == ENOTTY ? LOG_DEBUG :
> > LOG_WARNING,
> > +                                 "Failed to enable hardware
> > watchdog: %m");
> > +                        if (errno != ENOTTY)
> > +                        	return errno;
> 
> multiples of 8 space indenting, please.
> 
> > +                }
> >  
> >                  r = ioctl(watchdog_fd, WDIOC_KEEPALIVE, 0);
> >                  if (r < 0)
> > 

Both fixed, I'll resubmit shortly, sorry again and thanks for the
review.

-- 
Jean Delvare
SUSE L3 Support