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

2015-06-15 Thread Lennart Poettering 
On Mon, 15.06.15 18:14, Jean Delvare (jdelv...@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.

Isn't this something that should be fixed in the drivers?

 ---
  src/shared/watchdog.c |3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 --- a/src/shared/watchdog.c
 +++ b/src/shared/watchdog.c
 @@ -64,7 +64,8 @@ static int update_timeout(void) {
  
  flags = WDIOS_ENABLECARD;
  r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, flags);
 -if (r  0) {
 +/* ENOTTY means the watchdog is always enabled so we're fine 
 */
 +if (r  0  errno != ENOTTY) {
  log_warning(Failed to enable hardware watchdog: 
 %m);
  return -errno;

If this is something to fix in systemd, rather than fix in the
drivers:  am pretty sure that we should log in all cases, but change
the log level to LOG_DEBUG if its ENOTTY. i.e. use log_full(errno ==
ENOTTY ? LOG_DEBUG : LOG_WARNING...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-06-15 Thread Jean Delvare 
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.
---
 src/shared/watchdog.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/src/shared/watchdog.c
+++ b/src/shared/watchdog.c
@@ -64,7 +64,8 @@ static int update_timeout(void) {
 
 flags = WDIOS_ENABLECARD;
 r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, flags);
-if (r  0) {
+/* ENOTTY means the watchdog is always enabled so we're fine */
+if (r  0  errno != ENOTTY) {
 log_warning(Failed to enable hardware watchdog: %m);
 return -errno;
 }

-- 
Jean Delvare
SUSE L3 Support

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-06-15 Thread Jean Delvare 
Hi Lennart,

Thanks for your quick reply.

Le Monday 15 June 2015 à 18:16 +0200, Lennart Poettering a écrit :
 On Mon, 15.06.15 18:14, Jean Delvare (jdelv...@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.
 
 Isn't this something that should be fixed in the drivers?

This is a legitimate question and I pondered it myself too. However the
fact that over 20 drivers do not implement WDIOC_SETOPTIONS, together
with the fact that it's named, well, set options, give me the feeling
that not implementing it is legitimate.

  ---
   src/shared/watchdog.c |3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)
  
  --- a/src/shared/watchdog.c
  +++ b/src/shared/watchdog.c
  @@ -64,7 +64,8 @@ static int update_timeout(void) {
   
   flags = WDIOS_ENABLECARD;
   r = ioctl(watchdog_fd, WDIOC_SETOPTIONS, flags);
  -if (r  0) {
  +/* ENOTTY means the watchdog is always enabled so we're 
  fine */
  +if (r  0  errno != ENOTTY) {
   log_warning(Failed to enable hardware watchdog: 
  %m);
   return -errno;
 
 If this is something to fix in systemd, rather than fix in the
 drivers:  am pretty sure that we should log in all cases, but change
 the log level to LOG_DEBUG if its ENOTTY. i.e. use log_full(errno ==
 ENOTTY ? LOG_DEBUG : LOG_WARNING...

Sure, I can try that. Updated patch coming (likely tomorrow.)

Thanks for the review,
-- 
Jean Delvare
SUSE L3 Support

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel