| Message ID | 1399473780-20374-3-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <eblake@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> Where does the Rb come from on this v1? Is it in any tree already? Regards, Andreas
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 <eblake@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel.a@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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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 <eblake@redhat.com> Signed-off-by: >>> Marcel Apfelbaum <marcel.a@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? Regards, Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTc6EeAAoJEPou0S0+fgE/EZEP/0Demtm8RD/bDXkgSZYq8H7C 6FuPxjrkx2F/5Cot36VWeyGWB9A4GX3xH8+OJAsWdPyiq2ChOVXub/knlfedvBKP et8Y2uub5FoLV3l8fPOy8CCO97erF4ZpZ28L1Br7jeUO+gR/b0/a0c2fj3V5SeMO vgY6or46nBteVTyYtkAVx4Ne3VddNjiJJrgTASJkIFCFicG1VSQn6+YT89qmEx1i JL8bbh5UNQ/hSFgQzl88mge9Udv3Zjp1zmbochhdazmoG6dHy7g0A7TuDkhIqnLX p4uEzerjJo+24z6rX50CRj4zr6jnZtMdSWJF25+hvRGOz0UsHQ8D5OTZfs7qwBN3 Nkzqx/tMGENcNOnivJ3CGu7GhlBBEK3moK885dlZQ8p7w0+agNQDrR0CSMJlI4ck 8LgxcDNfGgnK948ZZNPTAlqOR7ZJV1dNE1fHulPVNS1xSdk2h48xOn8NNncRAwyq bs+45pxwTjp+L5WLF7XLKsVF1hYy4wi5pBhT0w+xTymYoP5aHKRmIhogvJY1b6Wf 5isZQUHUeGiepZ26smYROrrQhO4Q2ZqHKjp3yUNfhHaP7bdZYh9ykPQQZra+9+K/ lXNmzFIj2L+kdl5R6/pn9h3nZDjrWxGd+IreVASseW0RuLZXDivOtBKZJ6ZBAqQd Dx5lbtVnihJl9KRRxQw2 =+B1b -----END PGP SIGNATURE-----
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 <eblake@redhat.com> Signed-off-by: > >>> Marcel Apfelbaum <marcel.a@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. Thanks, Marcel > > Regards, > Andreas >
On Wed, 14 May 2014 20:29:37 +0300 Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> Signed-off-by: > > >>> Marcel Apfelbaum <marcel.a@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. 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 <lcapitulino@redhat.com>
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 14 May 2014 20:29:37 +0300 > Marcel Apfelbaum <marcel.a@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 <eblake@redhat.com> Signed-off-by: >> > >>> Marcel Apfelbaum <marcel.a@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. I can have a look tomorrow. > 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 <lcapitulino@redhat.com>
Am 14.05.2014 19:29, schrieb Marcel Apfelbaum: > 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 <eblake@redhat.com> Signed-off-by: >>>>> Marcel Apfelbaum <marcel.a@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. You had already mentioned a NULL -kernel. I was asking what test case Luiz' qmp_output_last() would be about. :) Cheers, Andreas
Marcel Apfelbaum <marcel.a@redhat.com> writes: > 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 <eblake@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > qapi/qmp-output-visitor.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index 74a5684..0562f49 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) > static QObject *qmp_output_first(QmpOutputVisitor *qov) > { > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > + > + if (!e) { > + return NULL; > + } > + > return e->value; > } Let's see how this thing works. The visitor's mutable state is a QStack, which is stack of (QObject, bool). We can ignore the bool; it's just for qmp_output_next_list(). Visits start with an empty stack. See qmp_output_visitor_new(). qmp_output_first() returns the object on the bottom of the stack. qmp_output_last() returns the object on the top of the stack. <rant> When you implement a stack with a double-ended queue, you're totally free to pick either end of the queue for top of stack. You're also free to name your functions accessing top and the bottom of the stack however you like. "Of course" the author picked queue end and function names for maximum confusion: static QObject *qmp_output_first(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); return e->value; } static QObject *qmp_output_last(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_FIRST(&qov->stack); return e->value; } I hate you. </rant> The result of the visit sits at the bottom of the stack. Empty stack, null result. See qmp_output_get_qobject(). Visiting a scalar type creates the appropriate scalar QObject, and "adds" it. We'll find out what "adding" means shortly. See qmp_output_type_{int,bool,str,number}(). Special case: null strings get converted to empty strings. See qmp_output_type_str(). Starting a struct visit creates a QDict, adds it, and pushes it onto the stack. Ending it pops it from the stack. See qmp_output_{start,end}_struct(). Starting a list visit creates a QList, adds it, and pushes it onto the stack. Ending it pops it from the stack. See qmp_output_{start,end}_list(). Visiting a list member does nothing interesting; see qmp_output_next_list(). Aside: I suspect the GenericList traversal stuff now done in every next_list() method should be done in the visitor core instead. Now let's figure out what it means to "add" an object. This is qmp_output_add_obj(). If the stack is still empty, the object is the root object, and it gets pushed. Else, if the object on top of the stack is a QDict, we're visiting a struct. Enter the object into the QDict. Else, if the object on top of the stack is a QList, we're visiting a list. Append the object to the QList. Else, the object on top of the stack must be scalar, and I think it must be the root object. We replace it by the object being added. WTF? This feels more complicated than it could be. Anyway, how could a null object end up at the bottom of the stack, so that qmp_output_first() chokes on it? I can't see that. If it can get added, then why can it be seen only by qmp_output_first(), but not by qmp_output_last() and qmp_output_pop()?
Quoting Markus Armbruster (2014-05-15 11:13:09) > Marcel Apfelbaum <marcel.a@redhat.com> writes: > > > 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 <eblake@redhat.com> > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > qapi/qmp-output-visitor.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > > index 74a5684..0562f49 100644 > > --- a/qapi/qmp-output-visitor.c > > +++ b/qapi/qmp-output-visitor.c > > @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) > > static QObject *qmp_output_first(QmpOutputVisitor *qov) > > { > > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > > + > > + if (!e) { > > + return NULL; > > + } > > + > > return e->value; > > } > > Let's see how this thing works. > > The visitor's mutable state is a QStack, which is stack of (QObject, > bool). We can ignore the bool; it's just for qmp_output_next_list(). > > Visits start with an empty stack. See qmp_output_visitor_new(). > > qmp_output_first() returns the object on the bottom of the stack. > qmp_output_last() returns the object on the top of the stack. > > <rant> > When you implement a stack with a double-ended queue, you're totally > free to pick either end of the queue for top of stack. You're also free > to name your functions accessing top and the bottom of the stack however > you like. "Of course" the author picked queue end and function names > for maximum confusion: > > static QObject *qmp_output_first(QmpOutputVisitor *qov) > { > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > return e->value; > } > > static QObject *qmp_output_last(QmpOutputVisitor *qov) > { > QStackEntry *e = QTAILQ_FIRST(&qov->stack); > return e->value; > } > > I hate you. > </rant> > > The result of the visit sits at the bottom of the stack. Empty stack, > null result. See qmp_output_get_qobject(). > > Visiting a scalar type creates the appropriate scalar QObject, and > "adds" it. We'll find out what "adding" means shortly. See > qmp_output_type_{int,bool,str,number}(). > > Special case: null strings get converted to empty strings. See > qmp_output_type_str(). > > Starting a struct visit creates a QDict, adds it, and pushes it onto the > stack. Ending it pops it from the stack. See > qmp_output_{start,end}_struct(). > > Starting a list visit creates a QList, adds it, and pushes it onto the > stack. Ending it pops it from the stack. See > qmp_output_{start,end}_list(). > > Visiting a list member does nothing interesting; see > qmp_output_next_list(). Aside: I suspect the GenericList traversal > stuff now done in every next_list() method should be done in the visitor > core instead. > > Now let's figure out what it means to "add" an object. This is > qmp_output_add_obj(). > > If the stack is still empty, the object is the root object, and it gets > pushed. > > Else, if the object on top of the stack is a QDict, we're visiting a > struct. Enter the object into the QDict. > > Else, if the object on top of the stack is a QList, we're visiting a > list. Append the object to the QList. > > Else, the object on top of the stack must be scalar, and I think it must > be the root object. We replace it by the object being added. WTF? > > This feels more complicated than it could be. Anyway, how could a null > object end up at the bottom of the stack, so that qmp_output_first() > chokes on it? I can't see that. > > If it can get added, then why can it be seen only by qmp_output_first(), > but not by qmp_output_last() and qmp_output_pop()? See my note above, the corner case we're hitting seems to be when there's nothing in the stack at all: generating a QObject from an empty QmpOutputVisitor. This occurs with object_property_get_str skips visit_type_str if the property-specific accessor returns NULL, but we still covert the visitor to a QObject to pull the string out later.
Michael Roth <mdroth@linux.vnet.ibm.com> writes: > Quoting Markus Armbruster (2014-05-15 11:13:09) >> Marcel Apfelbaum <marcel.a@redhat.com> writes: >> >> > 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 <eblake@redhat.com> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> >> > --- >> > qapi/qmp-output-visitor.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c >> > index 74a5684..0562f49 100644 >> > --- a/qapi/qmp-output-visitor.c >> > +++ b/qapi/qmp-output-visitor.c >> > @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) >> > static QObject *qmp_output_first(QmpOutputVisitor *qov) >> > { >> > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); >> > + >> > + if (!e) { >> > + return NULL; >> > + } >> > + >> > return e->value; >> > } >> >> Let's see how this thing works. >> >> The visitor's mutable state is a QStack, which is stack of (QObject, >> bool). We can ignore the bool; it's just for qmp_output_next_list(). >> >> Visits start with an empty stack. See qmp_output_visitor_new(). >> >> qmp_output_first() returns the object on the bottom of the stack. >> qmp_output_last() returns the object on the top of the stack. >> >> <rant> >> When you implement a stack with a double-ended queue, you're totally >> free to pick either end of the queue for top of stack. You're also free >> to name your functions accessing top and the bottom of the stack however >> you like. "Of course" the author picked queue end and function names >> for maximum confusion: >> >> static QObject *qmp_output_first(QmpOutputVisitor *qov) >> { >> QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); >> return e->value; >> } >> >> static QObject *qmp_output_last(QmpOutputVisitor *qov) >> { >> QStackEntry *e = QTAILQ_FIRST(&qov->stack); >> return e->value; >> } >> >> I hate you. >> </rant> >> >> The result of the visit sits at the bottom of the stack. Empty stack, >> null result. See qmp_output_get_qobject(). >> >> Visiting a scalar type creates the appropriate scalar QObject, and >> "adds" it. We'll find out what "adding" means shortly. See >> qmp_output_type_{int,bool,str,number}(). >> >> Special case: null strings get converted to empty strings. See >> qmp_output_type_str(). >> >> Starting a struct visit creates a QDict, adds it, and pushes it onto the >> stack. Ending it pops it from the stack. See >> qmp_output_{start,end}_struct(). >> >> Starting a list visit creates a QList, adds it, and pushes it onto the >> stack. Ending it pops it from the stack. See >> qmp_output_{start,end}_list(). >> >> Visiting a list member does nothing interesting; see >> qmp_output_next_list(). Aside: I suspect the GenericList traversal >> stuff now done in every next_list() method should be done in the visitor >> core instead. >> >> Now let's figure out what it means to "add" an object. This is >> qmp_output_add_obj(). >> >> If the stack is still empty, the object is the root object, and it gets >> pushed. >> >> Else, if the object on top of the stack is a QDict, we're visiting a >> struct. Enter the object into the QDict. >> >> Else, if the object on top of the stack is a QList, we're visiting a >> list. Append the object to the QList. >> >> Else, the object on top of the stack must be scalar, and I think it must >> be the root object. We replace it by the object being added. WTF? >> >> This feels more complicated than it could be. Anyway, how could a null >> object end up at the bottom of the stack, so that qmp_output_first() >> chokes on it? I can't see that. >> >> If it can get added, then why can it be seen only by qmp_output_first(), >> but not by qmp_output_last() and qmp_output_pop()? > > See my note above, the corner case we're hitting seems to be when there's > nothing in the stack at all: generating a QObject from an empty > QmpOutputVisitor. The other user of qmp_output_first() calls it like this: QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v); Patching qmp_output_first() makes this check redundant. I suspect we should change both callers to test QTAILQ_EMPTY() instead. > This occurs with object_property_get_str skips visit_type_str if the > property-specific accessor returns NULL, but we still covert the > visitor to a QObject to pull the string out later. Can't see visit_type_str() being called from object_property_get_str(). Do you mean property_get_str()? static void property_get_str(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) { StringProperty *prop = opaque; char *value; value = prop->get(obj, errp); if (value) { visit_type_str(v, &value, name, errp); g_free(value); } } Why do we skip visit_type_str() when value is null?
Quoting Markus Armbruster (2014-05-15 12:19:08) > Michael Roth <mdroth@linux.vnet.ibm.com> writes: > > > Quoting Markus Armbruster (2014-05-15 11:13:09) > >> Marcel Apfelbaum <marcel.a@redhat.com> writes: > >> > >> > 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 <eblake@redhat.com> > >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > >> > --- > >> > qapi/qmp-output-visitor.c | 5 +++++ > >> > 1 file changed, 5 insertions(+) > >> > > >> > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > >> > index 74a5684..0562f49 100644 > >> > --- a/qapi/qmp-output-visitor.c > >> > +++ b/qapi/qmp-output-visitor.c > >> > @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) > >> > static QObject *qmp_output_first(QmpOutputVisitor *qov) > >> > { > >> > QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > >> > + > >> > + if (!e) { > >> > + return NULL; > >> > + } > >> > + > >> > return e->value; > >> > } > >> > >> Let's see how this thing works. > >> > >> The visitor's mutable state is a QStack, which is stack of (QObject, > >> bool). We can ignore the bool; it's just for qmp_output_next_list(). > >> > >> Visits start with an empty stack. See qmp_output_visitor_new(). > >> > >> qmp_output_first() returns the object on the bottom of the stack. > >> qmp_output_last() returns the object on the top of the stack. > >> > >> <rant> > >> When you implement a stack with a double-ended queue, you're totally > >> free to pick either end of the queue for top of stack. You're also free > >> to name your functions accessing top and the bottom of the stack however > >> you like. "Of course" the author picked queue end and function names > >> for maximum confusion: > >> > >> static QObject *qmp_output_first(QmpOutputVisitor *qov) > >> { > >> QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); > >> return e->value; > >> } > >> > >> static QObject *qmp_output_last(QmpOutputVisitor *qov) > >> { > >> QStackEntry *e = QTAILQ_FIRST(&qov->stack); > >> return e->value; > >> } > >> > >> I hate you. > >> </rant> > >> > >> The result of the visit sits at the bottom of the stack. Empty stack, > >> null result. See qmp_output_get_qobject(). > >> > >> Visiting a scalar type creates the appropriate scalar QObject, and > >> "adds" it. We'll find out what "adding" means shortly. See > >> qmp_output_type_{int,bool,str,number}(). > >> > >> Special case: null strings get converted to empty strings. See > >> qmp_output_type_str(). > >> > >> Starting a struct visit creates a QDict, adds it, and pushes it onto the > >> stack. Ending it pops it from the stack. See > >> qmp_output_{start,end}_struct(). > >> > >> Starting a list visit creates a QList, adds it, and pushes it onto the > >> stack. Ending it pops it from the stack. See > >> qmp_output_{start,end}_list(). > >> > >> Visiting a list member does nothing interesting; see > >> qmp_output_next_list(). Aside: I suspect the GenericList traversal > >> stuff now done in every next_list() method should be done in the visitor > >> core instead. > >> > >> Now let's figure out what it means to "add" an object. This is > >> qmp_output_add_obj(). > >> > >> If the stack is still empty, the object is the root object, and it gets > >> pushed. > >> > >> Else, if the object on top of the stack is a QDict, we're visiting a > >> struct. Enter the object into the QDict. > >> > >> Else, if the object on top of the stack is a QList, we're visiting a > >> list. Append the object to the QList. > >> > >> Else, the object on top of the stack must be scalar, and I think it must > >> be the root object. We replace it by the object being added. WTF? > >> > >> This feels more complicated than it could be. Anyway, how could a null > >> object end up at the bottom of the stack, so that qmp_output_first() > >> chokes on it? I can't see that. > >> > >> If it can get added, then why can it be seen only by qmp_output_first(), > >> but not by qmp_output_last() and qmp_output_pop()? > > > > See my note above, the corner case we're hitting seems to be when there's > > nothing in the stack at all: generating a QObject from an empty > > QmpOutputVisitor. > > The other user of qmp_output_first() calls it like this: > > QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v); > > Patching qmp_output_first() makes this check redundant. > > I suspect we should change both callers to test QTAILQ_EMPTY() instead. Or remove the redundant check, don't have a strong preference either way, though personally I think qmp_output_first should handle it internally and just give us the (possibly NULL) QObject, since it exposes less internals to callers. > > > This occurs with object_property_get_str skips visit_type_str if the > > property-specific accessor returns NULL, but we still covert the > > visitor to a QObject to pull the string out later. > > Can't see visit_type_str() being called from object_property_get_str(). > Do you mean property_get_str()? Yah, I think it's something like: object_property_get_str->object_property_get->prop.get->property_get_str > > static void property_get_str(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > { > StringProperty *prop = opaque; > char *value; > > value = prop->get(obj, errp); > if (value) { > visit_type_str(v, &value, name, errp); > g_free(value); > } > } > > Why do we skip visit_type_str() when value is null? Not sure..., seems like an explicit fall through to the QERR_INVALID_PARAMETER_TYPE error in object_property_get_str that wasn't reachable prior to Marcel's patch (due to segfault), so I'm not sure this code path was in play until now.
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 74a5684..0562f49 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov) static QObject *qmp_output_first(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); + + if (!e) { + return NULL; + } + return e->value; }