Quoting Luiz Capitulino (2014-05-14 13:25:16) > On Wed, 14 May 2014 20:29:37 +0300 > Marcel Apfelbaum <marce...@redhat.com> wrote: > > > On Wed, 2014-05-14 at 19:00 +0200, Andreas Färber wrote: > > > Am 13.05.2014 21:08, schrieb Eric Blake: > > > > On 05/13/2014 11:36 AM, Andreas Färber wrote: > > > >> Am 07.05.2014 16:42, schrieb Marcel Apfelbaum: > > > >>> A NULL value is not added to visitor's stack, but there is no > > > >>> check for that when the visitor tries to return that value, > > > >>> leading to Qemu crash. > > > >>> > > > >>> Reviewed-by: Eric Blake <ebl...@redhat.com> Signed-off-by: > > > >>> Marcel Apfelbaum <marce...@redhat.com> > > > >> > > > >> Where does the Rb come from on this v1? Is it in any tree > > > >> already? > > > >> > > > > > > > > The (weak) R-b was here: > > > > https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02861.html > > > > > > Thanks. > > > > > > > So Luiz was okay with it too, but his last message seems to be > > > indicating this needs to be fixed somewhere else, too: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg05228.html > > > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg00217.html > > > > > > Can/should that be addressed as a follow-up? Or is there a test case > > > that breaks? > > Simple and "popular" test case: the user does not use the -kernel-cmdline > > parameter. > > The patch is needed because otherwise the main function will fail > > if no value is passed by the user to string parameters. > > > > Regarding Luiz's concern, it can be a follow-up as I am not aware of > > any problem with that. > > My concern was that I wasn't sure if this is the right fix for the issue > or if it's papering over the real bug. I quickly checked the code and it > seemed to make sense, but I didn't have time to study it deeper.
Not sure the fix is bad or not, but the cause might be a little more subtle than NULL string values as mentioned in the other thread. QmpOutputVisitor encodes NULL strings as "" via qmp_output_type_str(), so the problem doesn't seem to lie there: it shouldn't generate NULL values on the stack. I think the real issue is that object_property_get_str() actually calls an accessor via property_get_str to get the string, then explicitly *skips* the call to visit_type_str() if it is NULL (as it would be in the case of, say, kernel_cmdline option being NULL). So I wonder if maybe the real issue we're fixing is a corner case where you call qmp_output_get_qobject() on an "empty" QmpOutputVisitor. Surprised that's not covered by tests, but didn't see any coverage doing a cursory glance. Actually, might as well just add one.. diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index e073d83..f190eaa 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -434,6 +434,17 @@ static void test_visitor_out_union(TestOutputVisitorData *data, QDECREF(qdict); } +static void test_visitor_out_empty(TestOutputVisitorData *data, + const void *unused) +{ + QObject *arg; + QDict *qdict; + + arg = qmp_output_get_qobject(data->qov); + qdict = qobject_to_qdict(arg); + QDECREF(qdict); +} + static void init_native_list(UserDefNativeListUnion *cvalue) { int i; @@ -782,6 +793,8 @@ int main(int argc, char **argv) &out_visitor_data, test_visitor_out_list_qapi_free); output_visitor_test_add("/visitor/output/union", &out_visitor_data, test_visitor_out_union); + output_visitor_test_add("/visitor/output/empty", + &out_visitor_data, test_visitor_out_empty); output_visitor_test_add("/visitor/output/native_list/int", &out_visitor_data, test_visitor_out_native_list_int); output_visitor_test_add("/visitor/output/native_list/int8", mdroth@loki:~/w/qemu-build$ tests/test-qmp-output-visitor /visitor/output/int: OK /visitor/output/bool: OK /visitor/output/number: OK /visitor/output/string: OK /visitor/output/no-string: OK /visitor/output/enum: OK /visitor/output/enum-errors: OK /visitor/output/struct: OK /visitor/output/struct-nested: OK /visitor/output/struct-errors: OK /visitor/output/list: OK /visitor/output/list-qapi-free: OK /visitor/output/union: OK /visitor/output/empty: Segmentation fault (core dumped) So I guess the question is whether we should support converting an empty QmpOutputVisitor to a QObject. I would say yes, and that a NULL value is probably the most reasonable value. I would ask that commit/code is a little more explicit about what corner case is being handled though, and that something like the above unit test be included with the series. > > We could ask Michael Roth or Anthony, but I wouldn't hold this series > because of that. Here's my ACK if you need it: > > Acked-by: Luiz Capitulino <lcapitul...@redhat.com>