| Message ID | 20160721140030.28383-12-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > There are a few commands that are undef #ifdef conditions in under #ifdef > qmp-commands.hx. Move all the qmp_query_spice fallback in the same > location, return an error and update the comment. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > monitor.c | 14 ++++++++++++++ > qmp.c | 16 ---------------- > 2 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/monitor.c b/monitor.c > index c87089f..46966d5 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4048,6 +4048,20 @@ QemuOptsList qemu_mon_opts = { > }, > }; > > +/* > + * the QAPI schema is blissfully unaware #ifdef FOO commands, and the > + * QAPI code generator happily generates a dead qmp_marshal_foo_cmd() > + * that calls qmp_foo_cmd(). Provide it one, or else linking fails. > + * FIXME Educate the QAPI schema on #ifdef commands. > + */ > +#ifndef CONFIG_SPICE > +SpiceInfo *qmp_query_spice(Error **errp) > +{ > + error_setg(errp, QERR_FEATURE_DISABLED, "spice"); > + return NULL; Why do you change from abort() to error_setg()? > +}; > +#endif > + > #ifndef TARGET_I386 > void qmp_rtc_reset_reinjection(Error **errp) > { > diff --git a/qmp.c b/qmp.c > index b6d531e..884d1ab 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -161,22 +161,6 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) > }; > #endif > > -#ifndef CONFIG_SPICE > -/* > - * qmp-commands.hx ensures that QMP command query-spice exists only > - * #ifdef CONFIG_SPICE. Necessary for an accurate query-commands > - * result. However, the QAPI schema is blissfully unaware of that, > - * and the QAPI code generator happily generates a dead > - * qmp_marshal_query_spice() that calls qmp_query_spice(). Provide it > - * one, or else linking fails. FIXME Educate the QAPI schema on > - * CONFIG_SPICE. > - */ > -SpiceInfo *qmp_query_spice(Error **errp) > -{ > - abort(); > -}; > -#endif > - > void qmp_cont(Error **errp) > { > Error *local_err = NULL;
----- Original Message ----- > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > There are a few commands that are undef #ifdef conditions in > > under #ifdef > > > qmp-commands.hx. Move all the qmp_query_spice fallback in the same > > location, return an error and update the comment. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > monitor.c | 14 ++++++++++++++ > > qmp.c | 16 ---------------- > > 2 files changed, 14 insertions(+), 16 deletions(-) > > > > diff --git a/monitor.c b/monitor.c > > index c87089f..46966d5 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -4048,6 +4048,20 @@ QemuOptsList qemu_mon_opts = { > > }, > > }; > > > > +/* > > + * the QAPI schema is blissfully unaware #ifdef FOO commands, and the > > + * QAPI code generator happily generates a dead qmp_marshal_foo_cmd() > > + * that calls qmp_foo_cmd(). Provide it one, or else linking fails. > > + * FIXME Educate the QAPI schema on #ifdef commands. > > + */ > > +#ifndef CONFIG_SPICE > > +SpiceInfo *qmp_query_spice(Error **errp) > > +{ > > + error_setg(errp, QERR_FEATURE_DISABLED, "spice"); > > + return NULL; > > Why do you change from abort() to error_setg()? The rest of the commands do not abort. It sounds to easy to trigger, and it's quite harmless to return an error instead. > > +}; > > +#endif > > + > > #ifndef TARGET_I386 > > void qmp_rtc_reset_reinjection(Error **errp) > > { > > diff --git a/qmp.c b/qmp.c > > index b6d531e..884d1ab 100644 > > --- a/qmp.c > > +++ b/qmp.c > > @@ -161,22 +161,6 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) > > }; > > #endif > > > > -#ifndef CONFIG_SPICE > > -/* > > - * qmp-commands.hx ensures that QMP command query-spice exists only > > - * #ifdef CONFIG_SPICE. Necessary for an accurate query-commands > > - * result. However, the QAPI schema is blissfully unaware of that, > > - * and the QAPI code generator happily generates a dead > > - * qmp_marshal_query_spice() that calls qmp_query_spice(). Provide it > > - * one, or else linking fails. FIXME Educate the QAPI schema on > > - * CONFIG_SPICE. > > - */ > > -SpiceInfo *qmp_query_spice(Error **errp) > > -{ > > - abort(); > > -}; > > -#endif > > - > > void qmp_cont(Error **errp) > > { > > Error *local_err = NULL; >
Marc-André Lureau <mlureau@redhat.com> writes: > ----- Original Message ----- >> marcandre.lureau@redhat.com writes: >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> > There are a few commands that are undef #ifdef conditions in >> >> under #ifdef >> >> > qmp-commands.hx. Move all the qmp_query_spice fallback in the same >> > location, return an error and update the comment. >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > --- >> > monitor.c | 14 ++++++++++++++ >> > qmp.c | 16 ---------------- >> > 2 files changed, 14 insertions(+), 16 deletions(-) >> > >> > diff --git a/monitor.c b/monitor.c >> > index c87089f..46966d5 100644 >> > --- a/monitor.c >> > +++ b/monitor.c >> > @@ -4048,6 +4048,20 @@ QemuOptsList qemu_mon_opts = { >> > }, >> > }; >> > >> > +/* >> > + * the QAPI schema is blissfully unaware #ifdef FOO commands, and the >> > + * QAPI code generator happily generates a dead qmp_marshal_foo_cmd() >> > + * that calls qmp_foo_cmd(). Provide it one, or else linking fails. >> > + * FIXME Educate the QAPI schema on #ifdef commands. >> > + */ >> > +#ifndef CONFIG_SPICE >> > +SpiceInfo *qmp_query_spice(Error **errp) >> > +{ >> > + error_setg(errp, QERR_FEATURE_DISABLED, "spice"); >> > + return NULL; >> >> Why do you change from abort() to error_setg()? > > The rest of the commands do not abort. It sounds to easy to trigger, and it's quite harmless to return an error instead. If you can trigger it, the comment is wrong to claim "dead". Can you trigger it before your series? If not, which commit makes it triggerable? [...]
diff --git a/monitor.c b/monitor.c index c87089f..46966d5 100644 --- a/monitor.c +++ b/monitor.c @@ -4048,6 +4048,20 @@ QemuOptsList qemu_mon_opts = { }, }; +/* + * the QAPI schema is blissfully unaware #ifdef FOO commands, and the + * QAPI code generator happily generates a dead qmp_marshal_foo_cmd() + * that calls qmp_foo_cmd(). Provide it one, or else linking fails. + * FIXME Educate the QAPI schema on #ifdef commands. + */ +#ifndef CONFIG_SPICE +SpiceInfo *qmp_query_spice(Error **errp) +{ + error_setg(errp, QERR_FEATURE_DISABLED, "spice"); + return NULL; +}; +#endif + #ifndef TARGET_I386 void qmp_rtc_reset_reinjection(Error **errp) { diff --git a/qmp.c b/qmp.c index b6d531e..884d1ab 100644 --- a/qmp.c +++ b/qmp.c @@ -161,22 +161,6 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) }; #endif -#ifndef CONFIG_SPICE -/* - * qmp-commands.hx ensures that QMP command query-spice exists only - * #ifdef CONFIG_SPICE. Necessary for an accurate query-commands - * result. However, the QAPI schema is blissfully unaware of that, - * and the QAPI code generator happily generates a dead - * qmp_marshal_query_spice() that calls qmp_query_spice(). Provide it - * one, or else linking fails. FIXME Educate the QAPI schema on - * CONFIG_SPICE. - */ -SpiceInfo *qmp_query_spice(Error **errp) -{ - abort(); -}; -#endif - void qmp_cont(Error **errp) { Error *local_err = NULL;