Since commit 781f2b3d1e ("qga: process_event() simplification"),
send_response() is called unconditionally, but will assert when "rsp" is
NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
"guest-shutdown".
Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qga/main.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/qga/main.c b/qga/main.c
index f0e454f28d3..3febf3b0fdf 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict *rsp)
QString *payload_qstr, *response_qstr;
GIOStatus status;
- g_assert(rsp && s->channel);
+ g_assert(s->channel);
+
+ if (!rsp) {
+ return 0;
+ }
payload_qstr = qobject_to_json(QOBJECT(rsp));
if (!payload_qstr) {
--
2.26.2.561.g07d8ea56f2
On 6/4/20 11:44 AM, Marc-André Lureau wrote: > Since commit 781f2b3d1e ("qga: process_event() simplification"), > send_response() is called unconditionally, but will assert when "rsp" is > NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as > "guest-shutdown". > > Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35 > Cc: Michael Roth <mdroth@linux.vnet.ibm.com> > Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qga/main.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/qga/main.c b/qga/main.c > index f0e454f28d3..3febf3b0fdf 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict *rsp) > QString *payload_qstr, *response_qstr; > GIOStatus status; > > - g_assert(rsp && s->channel); > + g_assert(s->channel); > + > + if (!rsp) { > + return 0; > + } Why not assert after the check? Anyway: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > payload_qstr = qobject_to_json(QOBJECT(rsp)); > if (!payload_qstr) { >
On Thu, Jun 4, 2020 at 11:46 AM Marc-André Lureau < marcandre.lureau@redhat.com> wrote: > Since commit 781f2b3d1e ("qga: process_event() simplification"), > send_response() is called unconditionally, but will assert when "rsp" is > NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as > "guest-shutdown". > > Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35 > Cc: Michael Roth <mdroth@linux.vnet.ibm.com> > Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qga/main.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/qga/main.c b/qga/main.c > index f0e454f28d3..3febf3b0fdf 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict *rsp) > QString *payload_qstr, *response_qstr; > GIOStatus status; > > - g_assert(rsp && s->channel); > + g_assert(s->channel); > + > + if (!rsp) { > + return 0; > + } > > > Thanks Marc-André, LGTM and should fix the issues I was seeing. Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Thu, Jun 4, 2020 at 3:43 PM Christian Ehrhardt < christian.ehrhardt@canonical.com> wrote: > > > On Thu, Jun 4, 2020 at 11:46 AM Marc-André Lureau < > marcandre.lureau@redhat.com> wrote: > >> Since commit 781f2b3d1e ("qga: process_event() simplification"), >> send_response() is called unconditionally, but will assert when "rsp" is >> NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as >> "guest-shutdown". >> >> Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35 >> Cc: Michael Roth <mdroth@linux.vnet.ibm.com> >> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> qga/main.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/qga/main.c b/qga/main.c >> index f0e454f28d3..3febf3b0fdf 100644 >> --- a/qga/main.c >> +++ b/qga/main.c >> @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict >> *rsp) >> QString *payload_qstr, *response_qstr; >> GIOStatus status; >> >> - g_assert(rsp && s->channel); >> + g_assert(s->channel); >> + >> + if (!rsp) { >> + return 0; >> + } >> >> >> > Thanks Marc-André, > LGTM and should fix the issues I was seeing. > > Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> > In the meantime I also got to test this against the initially reported issue, LGTM as well (ran as no-change backport onto 4.2). Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
On Tue, Jun 9, 2020 at 1:15 PM Christian Ehrhardt < christian.ehrhardt@canonical.com> wrote: > > > On Thu, Jun 4, 2020 at 3:43 PM Christian Ehrhardt < > christian.ehrhardt@canonical.com> wrote: > >> >> >> On Thu, Jun 4, 2020 at 11:46 AM Marc-André Lureau < >> marcandre.lureau@redhat.com> wrote: >> >>> Since commit 781f2b3d1e ("qga: process_event() simplification"), >>> send_response() is called unconditionally, but will assert when "rsp" is >>> NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as >>> "guest-shutdown". >>> >>> Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35 >>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com> >>> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> qga/main.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/qga/main.c b/qga/main.c >>> index f0e454f28d3..3febf3b0fdf 100644 >>> --- a/qga/main.c >>> +++ b/qga/main.c >>> @@ -531,7 +531,11 @@ static int send_response(GAState *s, const QDict >>> *rsp) >>> QString *payload_qstr, *response_qstr; >>> GIOStatus status; >>> >>> - g_assert(rsp && s->channel); >>> + g_assert(s->channel); >>> + >>> + if (!rsp) { >>> + return 0; >>> + } >>> >>> >>> >> Thanks Marc-André, >> LGTM and should fix the issues I was seeing. >> >> Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> >> > > In the meantime I also got to test this against the initially reported > issue, LGTM as well (ran as no-change backport onto 4.2). > > Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> > This LGTM with 2*reviews 1*tested and 11 days on the list without any negative feedback. I just wanted to re-check if there is anything else left for this to be committed?
© 2016 - 2024 Red Hat, Inc.