Patchwork [2/8] qapi script: report error for default case in union visit

login
register
mail settings
Submitter Wayne Xia
Date Nov. 6, 2013, 7:33 p.m.
Message ID <1383766420-20745-3-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/289169/
State New
Headers show

Comments

Wayne Xia - Nov. 6, 2013, 7:33 p.m.
It is possible to reach default case, when an union have a enum
discriminator, so don't abort() but report the error message.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 scripts/qapi-visit.py |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
Eric Blake - Nov. 12, 2013, 4:51 p.m.
On 11/06/2013 12:33 PM, Wenchao Xia wrote:
> It is possible to reach default case, when an union have a enum
> discriminator, so don't abort() but report the error message.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  scripts/qapi-visit.py |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)

I still think this is not ideal.  You are proposing a runtime error:

> +    # Tell caller the value is invalid, since the discriminator value maybe an
> +    # unmapped enum value.
>      ret += mcgen('''
>              default:
> -                abort();
> +                error_setg(&err,
> +                           "Invalid discriminator value %(pi)s for %(name)s",
> +                           (*obj)->kind);
> +                break;
>              }

whereas I'm requesting a compile-time error - it is much easier for
maintenance reasons to have the generator require up-front that all enum
values are covered, and loudly complain if an enum type is extended
without also extending the use of that enum type in a union, than it is
to silently generate a runtime error and wait for the bug reports
several weeks down the road.

Patch

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c39e628..b3d3af8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -223,6 +223,8 @@  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                 c_type = type_name(members[key]),
                 c_name = c_fun(key))
 
+    # Only support input visitor for an anon union now, and it is not possible
+    # to reach default, so abort() here, see the logic for (*obj)->kind
     ret += mcgen('''
         default:
             abort();
@@ -312,15 +314,23 @@  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                 c_type=type_name(members[key]),
                 c_name=c_fun(key))
 
+    # Tell caller the value is invalid, since the discriminator value maybe an
+    # unmapped enum value.
     ret += mcgen('''
             default:
-                abort();
+                error_setg(&err,
+                           "Invalid discriminator value %(pi)s for %(name)s",
+                           (*obj)->kind);
+                break;
             }
         }
         error_propagate(errp, err);
         err = NULL;
     }
-''')
+''',
+                pi="%d",
+                name=name)
+
     pop_indent()
     ret += mcgen('''
         /* Always call end_struct if start_struct succeeded.  */