output_type_enum() fails when *obj is not a valid value of the enum
type.  Should not happen.  Drop the check, along with its unit tests.
qapi_enum_lookup()'s assertion still guards.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi/qapi-visit-core.c              |  9 -------
 tests/test-qobject-output-visitor.c | 39 -----------------------------
 tests/test-string-output-visitor.c  | 19 --------------
 3 files changed, 67 deletions(-)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index d4aac206cf..80ca83bcb9 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -341,15 +341,6 @@ static void output_type_enum(Visitor *v, const char *name, int *obj,
     int value = *obj;
     char *enum_str;
 
-    /*
-     * TODO why is this an error, not an assertion?  If assertion:
-     * delete, and rely on qapi_enum_lookup()
-     */
-    if (value < 0 || value >= lookup->size) {
-        error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
-        return;
-    }
-
     enum_str = (char *)qapi_enum_lookup(lookup, value);
     visit_type_str(v, name, &enum_str, errp);
 }
diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index d7761ebf84..1c856d9bd2 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -141,21 +141,6 @@ static void test_visitor_out_enum(TestOutputVisitorData *data,
     }
 }
 
-static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
-                                         const void *unused)
-{
-    EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-
-    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        Error *err = NULL;
-
-        visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
-        error_free_or_abort(&err);
-        visitor_reset(data);
-    }
-}
-
-
 static void test_visitor_out_struct(TestOutputVisitorData *data,
                                     const void *unused)
 {
@@ -234,26 +219,6 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
     qapi_free_UserDefTwo(ud2);
 }
 
-static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
-                                           const void *unused)
-{
-    EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
-    UserDefOne u = {0};
-    UserDefOne *pu = &u;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        Error *err = NULL;
-
-        u.has_enum1 = true;
-        u.enum1 = bad_values[i];
-        visit_type_UserDefOne(data->ov, "unused", &pu, &err);
-        error_free_or_abort(&err);
-        visitor_reset(data);
-    }
-}
-
-
 static void test_visitor_out_list(TestOutputVisitorData *data,
                                   const void *unused)
 {
@@ -821,14 +786,10 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_no_string);
     output_visitor_test_add("/visitor/output/enum",
                             &out_visitor_data, test_visitor_out_enum);
-    output_visitor_test_add("/visitor/output/enum-errors",
-                            &out_visitor_data, test_visitor_out_enum_errors);
     output_visitor_test_add("/visitor/output/struct",
                             &out_visitor_data, test_visitor_out_struct);
     output_visitor_test_add("/visitor/output/struct-nested",
                             &out_visitor_data, test_visitor_out_struct_nested);
-    output_visitor_test_add("/visitor/output/struct-errors",
-                            &out_visitor_data, test_visitor_out_struct_errors);
     output_visitor_test_add("/visitor/output/list",
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/any",
diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index 1be1540767..3bd732222c 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -203,19 +203,6 @@ static void test_visitor_out_enum(TestOutputVisitorData *data,
     }
 }
 
-static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
-                                         const void *unused)
-{
-    EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-
-    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        Error *err = NULL;
-
-        visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
-        error_free_or_abort(&err);
-    }
-}
-
 static void
 output_visitor_test_add(const char *testpath,
                         TestOutputVisitorData *data,
@@ -260,12 +247,6 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_enum, false);
     output_visitor_test_add("/string-visitor/output/enum-human",
                             &out_visitor_data, test_visitor_out_enum, true);
-    output_visitor_test_add("/string-visitor/output/enum-errors",
-                            &out_visitor_data, test_visitor_out_enum_errors,
-                            false);
-    output_visitor_test_add("/string-visitor/output/enum-errors-human",
-                            &out_visitor_data, test_visitor_out_enum_errors,
-                            true);
     output_visitor_test_add("/string-visitor/output/intList",
                             &out_visitor_data, test_visitor_out_intList, false);
     output_visitor_test_add("/string-visitor/output/intList-human",
-- 
2.21.1

On 4/23/20 11:00 AM, Markus Armbruster wrote:
> output_type_enum() fails when *obj is not a valid value of the enum
> type.  Should not happen.  Drop the check, along with its unit tests.
> qapi_enum_lookup()'s assertion still guards.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qapi/qapi-visit-core.c              |  9 -------
>   tests/test-qobject-output-visitor.c | 39 -----------------------------
>   tests/test-string-output-visitor.c  | 19 --------------
>   3 files changed, 67 deletions(-)

Nice cleanup.

The commit message implies adding an assertion; but in reality, the 
change is deleting dead code (because we already have the assertion in 
place).  Maybe it's worth updating the subject?

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

> -    /*
> -     * TODO why is this an error, not an assertion?  If assertion:
> -     * delete, and rely on qapi_enum_lookup()
> -     */
> -    if (value < 0 || value >= lookup->size) {
> -        error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
> -        return;
> -    }

The comment being deleted is what points out the assertion that already 
exists.


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

Eric Blake <eblake@redhat.com> writes:

> On 4/23/20 11:00 AM, Markus Armbruster wrote:
>> output_type_enum() fails when *obj is not a valid value of the enum
>> type.  Should not happen.  Drop the check, along with its unit tests.
>> qapi_enum_lookup()'s assertion still guards.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qapi/qapi-visit-core.c              |  9 -------
>>   tests/test-qobject-output-visitor.c | 39 -----------------------------
>>   tests/test-string-output-visitor.c  | 19 --------------
>>   3 files changed, 67 deletions(-)
>
> Nice cleanup.
>
> The commit message implies adding an assertion; but in reality, the
> change is deleting dead code (because we already have the assertion in
> place).  Maybe it's worth updating the subject?

I tried to say it in the body: "qapi_enum_lookup()'s assertion still
guards."  I could replace that by "This unmasks qapi_enum_lookup()'s
assertion."  Okay?  Better ideas?

> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> -    /*
>> -     * TODO why is this an error, not an assertion?  If assertion:
>> -     * delete, and rely on qapi_enum_lookup()
>> -     */
>> -    if (value < 0 || value >= lookup->size) {
>> -        error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
>> -        return;
>> -    }
>
> The comment being deleted is what points out the assertion that
> already exists.

Thanks!