[Qemu-devel] [PATCH v4 0/3] watchdog: Allow setting action on the fly

[Qemu-devel] [PATCH v4 0/3] watchdog: Allow setting action on the fly

[Michal Privoznik]

diff to v3:
- Broken the last patch into two:
  - dropping local redefine of the WatchdogAction enum,
  - introducing new monitor command.
- Dropped two unused #include-s

Michal Privoznik (3):
  qapi: Rename WatchdogExpirationAction enum
  watchdog.h: Drop local redefinition of actions enum
  watchdog: Allow setting action on the fly

 hw/watchdog/watchdog.c    | 62 +++++++++++++++++++++++------------------------
 hw/watchdog/wdt_diag288.c |  6 ++---
 include/sysemu/watchdog.h | 12 ++-------
 monitor.c                 |  4 +--
 qapi-schema.json          |  9 +++++++
 qapi/run-state.json       |  6 ++---
 6 files changed, 49 insertions(+), 50 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v4 1/3] qapi: Rename WatchdogExpirationAction enum

[Michal Privoznik]

The new name is WatchdogAction which is shorter,

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/watchdog/watchdog.c | 14 +++++++-------
 monitor.c              |  4 ++--
 qapi/run-state.json    |  6 +++---
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 0c5c9cde1c..358d79804d 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -109,17 +109,17 @@ void watchdog_perform_action(void)
 {
     switch (watchdog_action) {
     case WDT_RESET:             /* same as 'system_reset' in monitor */
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_RESET, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_RESET, &error_abort);
         qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
         break;
 
     case WDT_SHUTDOWN:          /* same as 'system_powerdown' in monitor */
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_SHUTDOWN, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN, &error_abort);
         qemu_system_powerdown_request();
         break;
 
     case WDT_POWEROFF:          /* same as 'quit' command in monitor */
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_POWEROFF, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF, &error_abort);
         exit(0);
 
     case WDT_PAUSE:             /* same as 'stop' command in monitor */
@@ -127,21 +127,21 @@ void watchdog_perform_action(void)
          * you would get a deadlock.  Bypass the problem.
          */
         qemu_system_vmstop_request_prepare();
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_PAUSE, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_PAUSE, &error_abort);
         qemu_system_vmstop_request(RUN_STATE_WATCHDOG);
         break;
 
     case WDT_DEBUG:
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_DEBUG, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG, &error_abort);
         fprintf(stderr, "watchdog: timer fired\n");
         break;
 
     case WDT_NONE:
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_NONE, &error_abort);
+        qapi_event_send_watchdog(WATCHDOG_ACTION_NONE, &error_abort);
         break;
 
     case WDT_NMI:
-        qapi_event_send_watchdog(WATCHDOG_EXPIRATION_ACTION_INJECT_NMI,
+        qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI,
                                  &error_abort);
         nmi_monitor_handle(0, NULL);
         break;
diff --git a/monitor.c b/monitor.c
index 9239f7adde..e6a6675c15 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3537,8 +3537,8 @@ void watchdog_action_completion(ReadLineState *rs, int nb_args, const char *str)
         return;
     }
     readline_set_completion_index(rs, strlen(str));
-    for (i = 0; i < WATCHDOG_EXPIRATION_ACTION__MAX; i++) {
-        add_completion_option(rs, str, WatchdogExpirationAction_str(i));
+    for (i = 0; i < WATCHDOG_ACTION__MAX; i++) {
+        add_completion_option(rs, str, WatchdogAction_str(i));
     }
 }
 
diff --git a/qapi/run-state.json b/qapi/run-state.json
index d36ff49834..bca46a8785 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -253,10 +253,10 @@
 #
 ##
 { 'event': 'WATCHDOG',
-  'data': { 'action': 'WatchdogExpirationAction' } }
+  'data': { 'action': 'WatchdogAction' } }
 
 ##
-# @WatchdogExpirationAction:
+# @WatchdogAction:
 #
 # An enumeration of the actions taken when the watchdog device's timer is
 # expired
@@ -279,7 +279,7 @@
 #
 # Since: 2.1
 ##
-{ 'enum': 'WatchdogExpirationAction',
+{ 'enum': 'WatchdogAction',
   'data': [ 'reset', 'shutdown', 'poweroff', 'pause', 'debug', 'none',
             'inject-nmi' ] }
 
-- 
2.13.5

[Qemu-devel] [PATCH v4 2/3] watchdog.h: Drop local redefinition of actions enum

[Michal Privoznik]

We already have enum that enumerates all the action that a
watchdog can take when hitting its timeout: WatchdogAction.
Use that instead of inventing our own.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/watchdog/watchdog.c    | 42 +++++++++++++++++-------------------------
 hw/watchdog/wdt_diag288.c |  6 +++---
 include/sysemu/watchdog.h | 12 ++----------
 3 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 358d79804d..547a49a1e4 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -30,7 +30,7 @@
 #include "hw/nmi.h"
 #include "qemu/help_option.h"
 
-static int watchdog_action = WDT_RESET;
+static WatchdogAction watchdog_action = WATCHDOG_ACTION_RESET;
 static QLIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
 
 void watchdog_add_model(WatchdogTimerModel *model)
@@ -77,27 +77,16 @@ int select_watchdog(const char *p)
 
 int select_watchdog_action(const char *p)
 {
-    if (strcasecmp(p, "reset") == 0)
-        watchdog_action = WDT_RESET;
-    else if (strcasecmp(p, "shutdown") == 0)
-        watchdog_action = WDT_SHUTDOWN;
-    else if (strcasecmp(p, "poweroff") == 0)
-        watchdog_action = WDT_POWEROFF;
-    else if (strcasecmp(p, "pause") == 0)
-        watchdog_action = WDT_PAUSE;
-    else if (strcasecmp(p, "debug") == 0)
-        watchdog_action = WDT_DEBUG;
-    else if (strcasecmp(p, "none") == 0)
-        watchdog_action = WDT_NONE;
-    else if (strcasecmp(p, "inject-nmi") == 0)
-        watchdog_action = WDT_NMI;
-    else
-        return -1;
+    int action;
 
+    action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
+    if (action < 0)
+        return -1;
+    watchdog_action = action;
     return 0;
 }
 
-int get_watchdog_action(void)
+WatchdogAction get_watchdog_action(void)
 {
     return watchdog_action;
 }
@@ -108,21 +97,21 @@ int get_watchdog_action(void)
 void watchdog_perform_action(void)
 {
     switch (watchdog_action) {
-    case WDT_RESET:             /* same as 'system_reset' in monitor */
+    case WATCHDOG_ACTION_RESET:     /* same as 'system_reset' in monitor */
         qapi_event_send_watchdog(WATCHDOG_ACTION_RESET, &error_abort);
         qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
         break;
 
-    case WDT_SHUTDOWN:          /* same as 'system_powerdown' in monitor */
+    case WATCHDOG_ACTION_SHUTDOWN:  /* same as 'system_powerdown' in monitor */
         qapi_event_send_watchdog(WATCHDOG_ACTION_SHUTDOWN, &error_abort);
         qemu_system_powerdown_request();
         break;
 
-    case WDT_POWEROFF:          /* same as 'quit' command in monitor */
+    case WATCHDOG_ACTION_POWEROFF:  /* same as 'quit' command in monitor */
         qapi_event_send_watchdog(WATCHDOG_ACTION_POWEROFF, &error_abort);
         exit(0);
 
-    case WDT_PAUSE:             /* same as 'stop' command in monitor */
+    case WATCHDOG_ACTION_PAUSE:     /* same as 'stop' command in monitor */
         /* In a timer callback, when vm_stop calls qemu_clock_enable
          * you would get a deadlock.  Bypass the problem.
          */
@@ -131,19 +120,22 @@ void watchdog_perform_action(void)
         qemu_system_vmstop_request(RUN_STATE_WATCHDOG);
         break;
 
-    case WDT_DEBUG:
+    case WATCHDOG_ACTION_DEBUG:
         qapi_event_send_watchdog(WATCHDOG_ACTION_DEBUG, &error_abort);
         fprintf(stderr, "watchdog: timer fired\n");
         break;
 
-    case WDT_NONE:
+    case WATCHDOG_ACTION_NONE:
         qapi_event_send_watchdog(WATCHDOG_ACTION_NONE, &error_abort);
         break;
 
-    case WDT_NMI:
+    case WATCHDOG_ACTION_INJECT_NMI:
         qapi_event_send_watchdog(WATCHDOG_ACTION_INJECT_NMI,
                                  &error_abort);
         nmi_monitor_handle(0, NULL);
         break;
+
+    default:
+        assert(0);
     }
 }
diff --git a/hw/watchdog/wdt_diag288.c b/hw/watchdog/wdt_diag288.c
index 47f289216a..1475743527 100644
--- a/hw/watchdog/wdt_diag288.c
+++ b/hw/watchdog/wdt_diag288.c
@@ -57,9 +57,9 @@ static void diag288_timer_expired(void *dev)
      * the BQL; reset before triggering the action to avoid races with
      * diag288 instructions. */
     switch (get_watchdog_action()) {
-    case WDT_DEBUG:
-    case WDT_NONE:
-    case WDT_PAUSE:
+    case WATCHDOG_ACTION_DEBUG:
+    case WATCHDOG_ACTION_NONE:
+    case WATCHDOG_ACTION_PAUSE:
         break;
     default:
         wdt_diag288_reset(dev);
diff --git a/include/sysemu/watchdog.h b/include/sysemu/watchdog.h
index 72a4da07a6..677ace3945 100644
--- a/include/sysemu/watchdog.h
+++ b/include/sysemu/watchdog.h
@@ -23,15 +23,7 @@
 #define QEMU_WATCHDOG_H
 
 #include "qemu/queue.h"
-
-/* Possible values for action parameter. */
-#define WDT_RESET        1      /* Hard reset. */
-#define WDT_SHUTDOWN     2      /* Shutdown. */
-#define WDT_POWEROFF     3      /* Quit. */
-#define WDT_PAUSE        4      /* Pause. */
-#define WDT_DEBUG        5      /* Prints a message and continues running. */
-#define WDT_NONE         6      /* Do nothing. */
-#define WDT_NMI          7      /* Inject nmi into the guest. */
+#include "qapi-types.h"
 
 struct WatchdogTimerModel {
     QLIST_ENTRY(WatchdogTimerModel) entry;
@@ -46,7 +38,7 @@ typedef struct WatchdogTimerModel WatchdogTimerModel;
 /* in hw/watchdog.c */
 int select_watchdog(const char *p);
 int select_watchdog_action(const char *action);
-int get_watchdog_action(void);
+WatchdogAction get_watchdog_action(void);
 void watchdog_add_model(WatchdogTimerModel *model);
 void watchdog_perform_action(void);
 
-- 
2.13.5

[Qemu-devel] [PATCH v4 3/3] watchdog: Allow setting action on the fly

[Michal Privoznik]

Currently, the only time that users can set watchdog action is at
the start as all we expose is this -watchdog-action command line
argument. This is suboptimal when users want to plug the device
later via monitor. Alternatively, they might want to change the
action for already existing device on the fly.

Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hw/watchdog/watchdog.c | 8 +++++++-
 qapi-schema.json       | 9 +++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 547a49a1e4..c49a069cbc 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -29,6 +29,7 @@
 #include "qapi-event.h"
 #include "hw/nmi.h"
 #include "qemu/help_option.h"
+#include "qmp-commands.h"
 
 static WatchdogAction watchdog_action = WATCHDOG_ACTION_RESET;
 static QLIST_HEAD(watchdog_list, WatchdogTimerModel) watchdog_list;
@@ -82,7 +83,7 @@ int select_watchdog_action(const char *p)
     action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
     if (action < 0)
         return -1;
-    watchdog_action = action;
+    qmp_watchdog_set_action(action, &error_abort);
     return 0;
 }
 
@@ -139,3 +140,8 @@ void watchdog_perform_action(void)
         assert(0);
     }
 }
+
+void qmp_watchdog_set_action(WatchdogAction action, Error **errp)
+{
+    watchdog_action = action;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index f3af2cb851..f5db401838 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3143,3 +3143,12 @@
 # Since 2.9
 ##
 { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
+
+##
+# @watchdog-set-action:
+#
+# Set watchdog action
+#
+# Since 2.11
+##
+{ 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
-- 
2.13.5

Re: [Qemu-devel] [PATCH v4 0/3] watchdog: Allow setting action on the fly

[Markus Armbruster]

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Perhaps Rich would like to have a look, too.

Re: [Qemu-devel] [PATCH v4 1/3] qapi: Rename WatchdogExpirationAction enum

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On 09/06/2017 06:24 AM, Michal Privoznik wrote:
> The new name is WatchdogAction which is shorter,
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  hw/watchdog/watchdog.c | 14 +++++++-------
>  monitor.c              |  4 ++--
>  qapi/run-state.json    |  6 +++---
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] watchdog: Allow setting action on the fly

[Richard W.M. Jones]

On Wed, Sep 06, 2017 at 05:32:08PM +0200, Markus Armbruster wrote:
> Series
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Perhaps Rich would like to have a look, too.

I looked at them and they seemed OK but I didn't review them.

I also added this comment on the bug which maybe relevant,
maybe not ...

https://bugzilla.redhat.com/show_bug.cgi?id=1447169#c4

  "Not that it matters, but the real hardware was built into the
   southbridge and so not hotpluggable.

   Do you know what happens (or should happen) if the guest kernel
   loads the i6300esb driver with nowayout=1 and the hardware is
   unplugged?  My reading of the driver says that this will not break
   or crash, although it probably won't work as the guest user
   expects.  You can tell from the qemu side if the guest selected
   nowayout because ESB_WDT_LOCK is written to the ESB_LOCK_REG
   register.

   Anyway, this may not matter."

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [Qemu-devel] [PATCH v4 2/3] watchdog.h: Drop local redefinition of actions enum

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

On 09/06/2017 06:24 AM, Michal Privoznik wrote:
> We already have enum that enumerates all the action that a

s/action/actions/

> watchdog can take when hitting its timeout: WatchdogAction.
> Use that instead of inventing our own.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---

> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>  
>  int select_watchdog_action(const char *p)
>  {
> -    if (strcasecmp(p, "reset") == 0)
> -        watchdog_action = WDT_RESET;

The old code was case-insensitive,

> +    action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);

the new code is not.  Do we care?  (I don't, but we could be breaking
someone's control flow).  Should qapi_enum_parse be taught to be
case-insensitive?  Or perhaps we answer related questions first: Do we
have any QAPI enums that have values differing only in case? Do we
prevent such QAPI definitions, to give us the potential of making the
parsing insensitive?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 3/3] watchdog: Allow setting action on the fly

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On 09/06/2017 06:24 AM, Michal Privoznik wrote:
> Currently, the only time that users can set watchdog action is at
> the start as all we expose is this -watchdog-action command line
> argument. This is suboptimal when users want to plug the device
> later via monitor. Alternatively, they might want to change the
> action for already existing device on the fly.
> 
> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  hw/watchdog/watchdog.c | 8 +++++++-
>  qapi-schema.json       | 9 +++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 

> +++ b/qapi-schema.json
> @@ -3143,3 +3143,12 @@
>  # Since 2.9
>  ##
>  { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
> +
> +##
> +# @watchdog-set-action:
> +#
> +# Set watchdog action
> +#
> +# Since 2.11
> +##
> +{ 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }

Markus went to some effort to sort the documentation output; is plopping
this at the end of the file the best location?

Otherwise,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 0/3] watchdog: Allow setting action on the fly

[Michal Privoznik]

On 09/06/2017 05:37 PM, Richard W.M. Jones wrote:
> On Wed, Sep 06, 2017 at 05:32:08PM +0200, Markus Armbruster wrote:
>> Series
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>> Perhaps Rich would like to have a look, too.
> 
> I looked at them and they seemed OK but I didn't review them.
> 
> I also added this comment on the bug which maybe relevant,
> maybe not ...
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1447169#c4
> 
>   "Not that it matters, but the real hardware was built into the
>    southbridge and so not hotpluggable.
> 
>    Do you know what happens (or should happen) if the guest kernel
>    loads the i6300esb driver with nowayout=1 and the hardware is
>    unplugged?  My reading of the driver says that this will not break
>    or crash, although it probably won't work as the guest user
>    expects.  You can tell from the qemu side if the guest selected
>    nowayout because ESB_WDT_LOCK is written to the ESB_LOCK_REG
>    register.
> 
>    Anyway, this may not matter."

I've tested this and the guest continued running happily after I've
detached the watchdog.

Michal

Re: [Qemu-devel] [PATCH v4 3/3] watchdog: Allow setting action on the fly

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>> Currently, the only time that users can set watchdog action is at
>> the start as all we expose is this -watchdog-action command line
>> argument. This is suboptimal when users want to plug the device
>> later via monitor. Alternatively, they might want to change the
>> action for already existing device on the fly.
>> 
>> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>> 
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  hw/watchdog/watchdog.c | 8 +++++++-
>>  qapi-schema.json       | 9 +++++++++
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>> 
>
>> +++ b/qapi-schema.json
>> @@ -3143,3 +3143,12 @@
>>  # Since 2.9
>>  ##
>>  { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
>> +
>> +##
>> +# @watchdog-set-action:
>> +#
>> +# Set watchdog action
>> +#
>> +# Since 2.11
>> +##
>> +{ 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
>
> Markus went to some effort to sort the documentation output; is plopping
> this at the end of the file the best location?

This patch won't regress any of my work, as I only moved stuff out of
qapi-schema-json, I didn't reorder within.

> Otherwise,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH v4 2/3] watchdog.h: Drop local redefinition of actions enum

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>> We already have enum that enumerates all the action that a
>
> s/action/actions/
>
>> watchdog can take when hitting its timeout: WatchdogAction.
>> Use that instead of inventing our own.
>> 
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>
>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>  
>>  int select_watchdog_action(const char *p)
>>  {
>> -    if (strcasecmp(p, "reset") == 0)
>> -        watchdog_action = WDT_RESET;
>
> The old code was case-insensitive,
>
>> +    action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>
> the new code is not.  Do we care?  (I don't, but we could be breaking
> someone's control flow).  Should qapi_enum_parse be taught to be
> case-insensitive?  Or perhaps we answer related questions first: Do we
> have any QAPI enums that have values differing only in case? Do we
> prevent such QAPI definitions, to give us the potential of making the
> parsing insensitive?

Case-sensitive everywhere is fine.  Case-insensitive everywhere also
fine, just not my personal preference.  What's not fine is "guess
whether this part of the interface is case-sensitive or not".

QMP is case-sensitive.  Let's keep it that way.

The -watchdog-action option has a case-insensitive argument.  The
obvious way to remain misfeature-^Wbackwards compatible is converting
the argument to lower case before handing it off to qapi_enum_parse.  I
doubt it matters, but just doing it is less work than debating how far
exactly we want to bend over backwards.

g_ascii_strdown() should do.  It only converts ASCII characters, but
anything else is going to fail in qapi_enum_parse() anyway.

Re: [Qemu-devel] [PATCH v4 2/3] watchdog.h: Drop local redefinition of actions enum

[Michal Privoznik]

On 09/06/2017 07:25 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>>> We already have enum that enumerates all the action that a
>>
>> s/action/actions/
>>
>>> watchdog can take when hitting its timeout: WatchdogAction.
>>> Use that instead of inventing our own.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>
>>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>>  
>>>  int select_watchdog_action(const char *p)
>>>  {
>>> -    if (strcasecmp(p, "reset") == 0)
>>> -        watchdog_action = WDT_RESET;
>>
>> The old code was case-insensitive,
>>
>>> +    action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>>
>> the new code is not.  Do we care?  (I don't, but we could be breaking
>> someone's control flow).  Should qapi_enum_parse be taught to be
>> case-insensitive?  Or perhaps we answer related questions first: Do we
>> have any QAPI enums that have values differing only in case? Do we
>> prevent such QAPI definitions, to give us the potential of making the
>> parsing insensitive?
> 
> Case-sensitive everywhere is fine.  Case-insensitive everywhere also
> fine, just not my personal preference.  What's not fine is "guess
> whether this part of the interface is case-sensitive or not".
> 
> QMP is case-sensitive.  Let's keep it that way.
> 
> The -watchdog-action option has a case-insensitive argument.  The
> obvious way to remain misfeature-^Wbackwards compatible is converting
> the argument to lower case before handing it off to qapi_enum_parse.  I
> doubt it matters, but just doing it is less work than debating how far
> exactly we want to bend over backwards.
> 
> g_ascii_strdown() should do.  It only converts ASCII characters, but
> anything else is going to fail in qapi_enum_parse() anyway.
> 

On the other hand, the documentation enumerates the accepted values in
lowercase. So one can argue that upper- or mixed-case is just a misuse
of a bug in the code. But getting the code in is more important to me so
I'll do the strdown() conversion and sent yet another version.

Michal

Re: [Qemu-devel] [PATCH v4 2/3] watchdog.h: Drop local redefinition of actions enum

[Markus Armbruster]

Michal Privoznik <mprivozn@redhat.com> writes:

> On 09/06/2017 07:25 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>>>> We already have enum that enumerates all the action that a
>>>
>>> s/action/actions/
>>>
>>>> watchdog can take when hitting its timeout: WatchdogAction.
>>>> Use that instead of inventing our own.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>
>>>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>>>  
>>>>  int select_watchdog_action(const char *p)
>>>>  {
>>>> -    if (strcasecmp(p, "reset") == 0)
>>>> -        watchdog_action = WDT_RESET;
>>>
>>> The old code was case-insensitive,
>>>
>>>> +    action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>>>
>>> the new code is not.  Do we care?  (I don't, but we could be breaking
>>> someone's control flow).  Should qapi_enum_parse be taught to be
>>> case-insensitive?  Or perhaps we answer related questions first: Do we
>>> have any QAPI enums that have values differing only in case? Do we
>>> prevent such QAPI definitions, to give us the potential of making the
>>> parsing insensitive?
>> 
>> Case-sensitive everywhere is fine.  Case-insensitive everywhere also
>> fine, just not my personal preference.  What's not fine is "guess
>> whether this part of the interface is case-sensitive or not".
>> 
>> QMP is case-sensitive.  Let's keep it that way.
>> 
>> The -watchdog-action option has a case-insensitive argument.  The
>> obvious way to remain misfeature-^Wbackwards compatible is converting
>> the argument to lower case before handing it off to qapi_enum_parse.  I
>> doubt it matters, but just doing it is less work than debating how far
>> exactly we want to bend over backwards.
>> 
>> g_ascii_strdown() should do.  It only converts ASCII characters, but
>> anything else is going to fail in qapi_enum_parse() anyway.
>> 
>
> On the other hand, the documentation enumerates the accepted values in
> lowercase. So one can argue that upper- or mixed-case is just a misuse
> of a bug in the code.

I quite agree, but...

>                       But getting the code in is more important to me so
> I'll do the strdown() conversion and sent yet another version.

... like you, I have to pick my battles.  A respin adding the stupid
case conversion seems safer for both of us than risking a debate on how
far we need to go for backward compatibility.