Quoting Fam Zheng (2014-09-10 07:30:39) > We shouldn't do anything in the switch block in enum's visit_type_ > function, when the enum data's ->kind is not valid at all. This happens > when the dealloc visitor is called, after qmp input visitor returned > error.
Comment is kind of confusing. Enum doesn't really have a .kind, this is more specifically regarding QAPI Union types who are missing a valid discriminator/.kind field. > > Now, the input visitor will set ->kind to <TYPE>_MAX if the value is not > found, so that in dealloc, the switch block knows to skip calling into > specific type's visiting functions. Hmm, since QAPI input visitors handle the allocation for constructed types, and generally do so with g_malloc0, I wonder if we could possibly just make <TYPE>_INVALID correspond to 0 for discriminators? I think that way we could avoid needed to have input_type_enum pass the state along? In any case, since the fix doesn't break existing tests, could you break the iotest out into a separate patch? I also don't like that the iotest doesn't run during the normal make check, where I think we should try to capture all our QAPI tests. I've added a test to test-qmp-input-strict.c. Will send the patch shortly. If it looks reasonable could you add it to your series? > > The added test case would trigger SIGSEGV without this fix. > > This crash is introduced since commit b1de5f43 (QMP: Add support for > Archipelago). > > Signed-off-by: Fam Zheng <[email protected]> > --- > qapi/qapi-visit-core.c | 12 +++++------- > scripts/qapi-visit.py | 6 ++++++ > tests/qemu-iotests/087 | 17 +++++++++++++++++ > tests/qemu-iotests/087.out | 13 +++++++++++++ > 4 files changed, 41 insertions(+), 7 deletions(-) > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 55f8d40..6c46e0e 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -276,23 +276,21 @@ void input_type_enum(Visitor *v, int *obj, const char > *strings[], > > visit_type_str(v, &enum_str, name, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > - return; > + enum_str = NULL; > } > > while (strings[value] != NULL) { > - if (strcmp(strings[value], enum_str) == 0) { > + if (enum_str && strcmp(strings[value], enum_str) == 0) { > break; > } > value++; > } > > - if (strings[value] == NULL) { > - error_set(errp, QERR_INVALID_PARAMETER, enum_str); > - g_free(enum_str); > - return; > + if (!local_err && strings[value] == NULL) { > + error_set(&local_err, QERR_INVALID_PARAMETER, enum_str); > } > > g_free(enum_str); > *obj = value; > + error_propagate(errp, local_err); > } > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index c129697..dad7561 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -379,6 +379,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, > const char *name, Error **e > c_name=c_fun(key)) > > ret += mcgen(''' > + case %(enum_full_value)s: > + break; > +''', > + enum_full_value = generate_enum_full_value(disc_type, 'MAX')) > + > + ret += mcgen(''' > default: > abort(); > } > diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 > index 82c56b1..d7454d1 100755 > --- a/tests/qemu-iotests/087 > +++ b/tests/qemu-iotests/087 > @@ -218,6 +218,23 @@ run_qemu <<EOF > { "execute": "quit" } > EOF > > +echo > +echo === Missing driver === > +echo > + > +_make_test_img -o encryption=on $size > +run_qemu -S <<EOF > +{ "execute": "qmp_capabilities" } > +{ "execute": "blockdev-add", > + "arguments": { > + "options": { > + "id": "disk" > + } > + } > + } > +{ "execute": "quit" } > +EOF > + > # success, all done > echo "*** done" > rm -f $seq.full > diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out > index 7fbee3f..f16bad0 100644 > --- a/tests/qemu-iotests/087.out > +++ b/tests/qemu-iotests/087.out > @@ -64,4 +64,17 @@ QMP_VERSION > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": > "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": > "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > > + > +=== Missing driver === > + > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 encryption=on > +Testing: -S > +QMP_VERSION > +{"return": {}} > +{"error": {"class": "GenericError", "desc": "Invalid parameter type for > 'driver', expected: string"}} > +{"return": {}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": > "SHUTDOWN"} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": > "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": > "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > + > *** done > -- > 1.9.3
