Add #if defined(CONFIG_VNC) in generated code, and adjust the
qmp/hmp code accordingly.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qapi-schema.json | 34 ++++++++++++++++++++++------------
qapi/event.json | 9 ++++++---
hmp.c | 14 +++++++++++++-
qmp.c | 30 ++++--------------------------
4 files changed, 45 insertions(+), 42 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 9c6c3e1a53..829c66f9eb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1660,7 +1660,8 @@
'data': { 'host': 'str',
'service': 'str',
'family': 'NetworkAddressFamily',
- 'websocket': 'bool' } }
+ 'websocket': 'bool' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncServerInfo:
@@ -1674,7 +1675,8 @@
##
{ 'struct': 'VncServerInfo',
'base': 'VncBasicInfo',
- 'data': { '*auth': 'str' } }
+ 'data': { '*auth': 'str' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncClientInfo:
@@ -1691,7 +1693,8 @@
##
{ 'struct': 'VncClientInfo',
'base': 'VncBasicInfo',
- 'data': { '*x509_dname': 'str', '*sasl_username': 'str' } }
+ 'data': { '*x509_dname': 'str', '*sasl_username': 'str' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncInfo:
@@ -1732,7 +1735,8 @@
{ 'struct': 'VncInfo',
'data': {'enabled': 'bool', '*host': 'str',
'*family': 'NetworkAddressFamily',
- '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} }
+ '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']},
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncPrimaryAuth:
@@ -1743,7 +1747,8 @@
##
{ 'enum': 'VncPrimaryAuth',
'data': [ 'none', 'vnc', 'ra2', 'ra2ne', 'tight', 'ultra',
- 'tls', 'vencrypt', 'sasl' ] }
+ 'tls', 'vencrypt', 'sasl' ],
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VncVencryptSubAuth:
@@ -1757,7 +1762,8 @@
'tls-none', 'x509-none',
'tls-vnc', 'x509-vnc',
'tls-plain', 'x509-plain',
- 'tls-sasl', 'x509-sasl' ] }
+ 'tls-sasl', 'x509-sasl' ],
+ 'if': 'defined(CONFIG_VNC)' }
##
@@ -1775,7 +1781,8 @@
{ 'struct': 'VncServerInfo2',
'base': 'VncBasicInfo',
'data': { 'auth' : 'VncPrimaryAuth',
- '*vencrypt' : 'VncVencryptSubAuth' } }
+ '*vencrypt' : 'VncVencryptSubAuth' },
+ 'if': 'defined(CONFIG_VNC)' }
##
@@ -1808,7 +1815,8 @@
'clients' : ['VncClientInfo'],
'auth' : 'VncPrimaryAuth',
'*vencrypt' : 'VncVencryptSubAuth',
- '*display' : 'str' } }
+ '*display' : 'str' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @query-vnc:
@@ -1839,7 +1847,8 @@
# }
#
##
-{ 'command': 'query-vnc', 'returns': 'VncInfo' }
+{ 'command': 'query-vnc', 'returns': 'VncInfo',
+ 'if': 'defined(CONFIG_VNC)' }
##
# @query-vnc-servers:
@@ -1850,7 +1859,8 @@
#
# Since: 2.3
##
-{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'] }
+{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'],
+ 'if': 'defined(CONFIG_VNC)' }
##
# @SpiceBasicInfo:
@@ -3077,8 +3087,8 @@
# Notes: An empty password in this command will set the password to the empty
# string. Existing clients are unaffected by executing this command.
##
-{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
-
+{ 'command': 'change-vnc-password', 'data': {'password': 'str'},
+ 'if': 'defined(CONFIG_VNC)' }
##
# @change:
#
diff --git a/qapi/event.json b/qapi/event.json
index 6d22b025cc..c8b8e9f384 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -263,7 +263,8 @@
##
{ 'event': 'VNC_CONNECTED',
'data': { 'server': 'VncServerInfo',
- 'client': 'VncBasicInfo' } }
+ 'client': 'VncBasicInfo' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VNC_INITIALIZED:
@@ -290,7 +291,8 @@
##
{ 'event': 'VNC_INITIALIZED',
'data': { 'server': 'VncServerInfo',
- 'client': 'VncClientInfo' } }
+ 'client': 'VncClientInfo' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @VNC_DISCONNECTED:
@@ -316,7 +318,8 @@
##
{ 'event': 'VNC_DISCONNECTED',
'data': { 'server': 'VncServerInfo',
- 'client': 'VncClientInfo' } }
+ 'client': 'VncClientInfo' },
+ 'if': 'defined(CONFIG_VNC)' }
##
# @SPICE_CONNECTED:
diff --git a/hmp.c b/hmp.c
index fd80dce758..9454c634bd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -605,6 +605,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
qapi_free_BlockStatsList(stats_list);
}
+#ifdef CONFIG_VNC
/* Helper for hmp_info_vnc_clients, _servers */
static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
const char *name)
@@ -692,6 +693,12 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
qapi_free_VncInfo2List(info2l);
}
+#else
+void hmp_info_vnc(Monitor *mon, const QDict *qdict)
+{
+ warn_report("VNC support is disabled");
+}
+#endif
#ifdef CONFIG_SPICE
void hmp_info_spice(Monitor *mon, const QDict *qdict)
@@ -1708,12 +1715,14 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &err);
}
+#ifdef CONFIG_VNC
static void hmp_change_read_arg(void *opaque, const char *password,
void *readline_opaque)
{
qmp_change_vnc_password(password, NULL);
monitor_read_command(opaque, 1);
}
+#endif
void hmp_change(Monitor *mon, const QDict *qdict)
{
@@ -1724,6 +1733,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
BlockdevChangeReadOnlyMode read_only_mode = 0;
Error *err = NULL;
+#ifdef CONFIG_VNC
if (strcmp(device, "vnc") == 0) {
if (read_only) {
monitor_printf(mon,
@@ -1738,7 +1748,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
}
}
qmp_change("vnc", target, !!arg, arg, &err);
- } else {
+ } else
+#endif
+ {
if (read_only) {
read_only_mode =
qapi_enum_parse(BlockdevChangeReadOnlyMode_lookup,
diff --git a/qmp.c b/qmp.c
index b86201e349..2c90dacb56 100644
--- a/qmp.c
+++ b/qmp.c
@@ -130,22 +130,6 @@ void qmp_cpu_add(int64_t id, Error **errp)
}
}
-#ifndef CONFIG_VNC
-/* If VNC support is enabled, the "true" query-vnc command is
- defined in the VNC subsystem */
-VncInfo *qmp_query_vnc(Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
- return NULL;
-};
-
-VncInfo2List *qmp_query_vnc_servers(Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
- return NULL;
-};
-#endif
-
#ifndef CONFIG_SPICE
/*
* qmp-commands.hx ensures that QMP command query-spice exists only
@@ -403,23 +387,17 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
qmp_change_vnc_listen(target, errp);
}
}
-#else
-void qmp_change_vnc_password(const char *password, Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
-}
-static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
- Error **errp)
-{
- error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
-}
#endif /* !CONFIG_VNC */
void qmp_change(const char *device, const char *target,
bool has_arg, const char *arg, Error **errp)
{
if (strcmp(device, "vnc") == 0) {
+#ifdef CONFIG_VNC
qmp_change_vnc(target, has_arg, arg, errp);
+#else
+ error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
+#endif
} else {
qmp_blockdev_change_medium(true, device, false, NULL, target,
has_arg, arg, false, 0, errp);
--
2.14.0.rc0.1.g40ca67566
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Add #if defined(CONFIG_VNC) in generated code, and adjust the
> qmp/hmp code accordingly.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> diff --git a/hmp.c b/hmp.c
> index fd80dce758..9454c634bd 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -605,6 +605,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
> qapi_free_BlockStatsList(stats_list);
> }
>
> +#ifdef CONFIG_VNC
> /* Helper for hmp_info_vnc_clients, _servers */
> static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
> const char *name)
> @@ -692,6 +693,12 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> qapi_free_VncInfo2List(info2l);
>
> }
> +#else
> +void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> +{
> + warn_report("VNC support is disabled");
> +}
> +#endif
I'm OK with this, so
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
although you might just be able to add a #ifdef in hmp-commands-info.hx
and avoid the is disabled function, or you might find that with the QMP
returning an error the HMP just passes that error on.
Dave
> #ifdef CONFIG_SPICE
> void hmp_info_spice(Monitor *mon, const QDict *qdict)
> @@ -1708,12 +1715,14 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, &err);
> }
>
> +#ifdef CONFIG_VNC
> static void hmp_change_read_arg(void *opaque, const char *password,
> void *readline_opaque)
> {
> qmp_change_vnc_password(password, NULL);
> monitor_read_command(opaque, 1);
> }
> +#endif
>
> void hmp_change(Monitor *mon, const QDict *qdict)
> {
> @@ -1724,6 +1733,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> BlockdevChangeReadOnlyMode read_only_mode = 0;
> Error *err = NULL;
>
> +#ifdef CONFIG_VNC
> if (strcmp(device, "vnc") == 0) {
> if (read_only) {
> monitor_printf(mon,
> @@ -1738,7 +1748,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> }
> }
> qmp_change("vnc", target, !!arg, arg, &err);
> - } else {
> + } else
> +#endif
> + {
> if (read_only) {
> read_only_mode =
> qapi_enum_parse(BlockdevChangeReadOnlyMode_lookup,
> diff --git a/qmp.c b/qmp.c
> index b86201e349..2c90dacb56 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -130,22 +130,6 @@ void qmp_cpu_add(int64_t id, Error **errp)
> }
> }
>
> -#ifndef CONFIG_VNC
> -/* If VNC support is enabled, the "true" query-vnc command is
> - defined in the VNC subsystem */
> -VncInfo *qmp_query_vnc(Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> - return NULL;
> -};
> -
> -VncInfo2List *qmp_query_vnc_servers(Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> - return NULL;
> -};
> -#endif
> -
> #ifndef CONFIG_SPICE
> /*
> * qmp-commands.hx ensures that QMP command query-spice exists only
> @@ -403,23 +387,17 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> qmp_change_vnc_listen(target, errp);
> }
> }
> -#else
> -void qmp_change_vnc_password(const char *password, Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -}
> -static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> - Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -}
> #endif /* !CONFIG_VNC */
>
> void qmp_change(const char *device, const char *target,
> bool has_arg, const char *arg, Error **errp)
> {
> if (strcmp(device, "vnc") == 0) {
> +#ifdef CONFIG_VNC
> qmp_change_vnc(target, has_arg, arg, errp);
> +#else
> + error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> +#endif
> } else {
> qmp_blockdev_change_medium(true, device, false, NULL, target,
> has_arg, arg, false, 0, errp);
> --
> 2.14.0.rc0.1.g40ca67566
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
>> Add #if defined(CONFIG_VNC) in generated code, and adjust the
>> qmp/hmp code accordingly.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> diff --git a/hmp.c b/hmp.c
>> index fd80dce758..9454c634bd 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -605,6 +605,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>> qapi_free_BlockStatsList(stats_list);
>> }
>>
>> +#ifdef CONFIG_VNC
>> /* Helper for hmp_info_vnc_clients, _servers */
>> static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
>> const char *name)
>> @@ -692,6 +693,12 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>> qapi_free_VncInfo2List(info2l);
>>
>> }
>> +#else
>> +void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>> +{
>> + warn_report("VNC support is disabled");
error_report(), please (see below).
>> +}
>> +#endif
>
> I'm OK with this, so
>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> although you might just be able to add a #ifdef in hmp-commands-info.hx
> and avoid the is disabled function, or you might find that with the QMP
> returning an error the HMP just passes that error on.
Let's compare failures when !CONFIG_VNC:
(a) Marc-André's patch as is:
(qemu) info vnc
warning: VNC support is disabled
Drop the "warning: " (because it ain't; the command failed), and this
is fine.
(b) Compiling them out completely (#ifdef in hmp-commands*.hx):
unknown command: 'vnc'
HMP bug; should be something like
Unknown command: 'info vnc'
but that's not this series' problem.
Good enough for me.
(c) Forwarding the QMP error verbatim
The command query-vnc has not been found
No good.
(d) Handling CommandNotFound
More work than (a) for the same result.
As far as I'm concerned, feel free to do (a) or (b).
[...]
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> >> Add #if defined(CONFIG_VNC) in generated code, and adjust the
> >> qmp/hmp code accordingly.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >> diff --git a/hmp.c b/hmp.c
> >> index fd80dce758..9454c634bd 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -605,6 +605,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
> >> qapi_free_BlockStatsList(stats_list);
> >> }
> >>
> >> +#ifdef CONFIG_VNC
> >> /* Helper for hmp_info_vnc_clients, _servers */
> >> static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
> >> const char *name)
> >> @@ -692,6 +693,12 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> >> qapi_free_VncInfo2List(info2l);
> >>
> >> }
> >> +#else
> >> +void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> >> +{
> >> + warn_report("VNC support is disabled");
>
> error_report(), please (see below).
>
> >> +}
> >> +#endif
> >
> > I'm OK with this, so
> >
> > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > although you might just be able to add a #ifdef in hmp-commands-info.hx
> > and avoid the is disabled function, or you might find that with the QMP
> > returning an error the HMP just passes that error on.
>
> Let's compare failures when !CONFIG_VNC:
>
> (a) Marc-André's patch as is:
>
> (qemu) info vnc
> warning: VNC support is disabled
>
> Drop the "warning: " (because it ain't; the command failed), and this
> is fine.
>
> (b) Compiling them out completely (#ifdef in hmp-commands*.hx):
>
> unknown command: 'vnc'
>
> HMP bug; should be something like
>
> Unknown command: 'info vnc'
>
> but that's not this series' problem.
I'll fix that missing 'info'
Dave
> Good enough for me.
>
> (c) Forwarding the QMP error verbatim
>
> The command query-vnc has not been found
>
> No good.
>
> (d) Handling CommandNotFound
>
> More work than (a) for the same result.
>
> As far as I'm concerned, feel free to do (a) or (b).
>
> [...]
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Copying our resident VNC maintainer^Wodd fixer Gerd.
Also copying Dan for QCryptoCipherAlgorithm.
Gerd, Dan, this patch is about making VNC support visible in
query-qmp-schema, by having the QAPI generators generate suitable
ifdeffery. Bonus: no need for QMP command stubs for
!defined(CONFIG_VNC).
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Add #if defined(CONFIG_VNC) in generated code, and adjust the
> qmp/hmp code accordingly.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi-schema.json | 34 ++++++++++++++++++++++------------
> qapi/event.json | 9 ++++++---
> hmp.c | 14 +++++++++++++-
> qmp.c | 30 ++++--------------------------
> 4 files changed, 45 insertions(+), 42 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9c6c3e1a53..829c66f9eb 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1660,7 +1660,8 @@
> 'data': { 'host': 'str',
> 'service': 'str',
> 'family': 'NetworkAddressFamily',
> - 'websocket': 'bool' } }
> + 'websocket': 'bool' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VncServerInfo:
> @@ -1674,7 +1675,8 @@
> ##
> { 'struct': 'VncServerInfo',
> 'base': 'VncBasicInfo',
> - 'data': { '*auth': 'str' } }
> + 'data': { '*auth': 'str' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VncClientInfo:
> @@ -1691,7 +1693,8 @@
> ##
> { 'struct': 'VncClientInfo',
> 'base': 'VncBasicInfo',
> - 'data': { '*x509_dname': 'str', '*sasl_username': 'str' } }
> + 'data': { '*x509_dname': 'str', '*sasl_username': 'str' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VncInfo:
> @@ -1732,7 +1735,8 @@
> { 'struct': 'VncInfo',
> 'data': {'enabled': 'bool', '*host': 'str',
> '*family': 'NetworkAddressFamily',
> - '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']} }
> + '*service': 'str', '*auth': 'str', '*clients': ['VncClientInfo']},
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VncPrimaryAuth:
> @@ -1743,7 +1747,8 @@
> ##
> { 'enum': 'VncPrimaryAuth',
> 'data': [ 'none', 'vnc', 'ra2', 'ra2ne', 'tight', 'ultra',
> - 'tls', 'vencrypt', 'sasl' ] }
> + 'tls', 'vencrypt', 'sasl' ],
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VncVencryptSubAuth:
> @@ -1757,7 +1762,8 @@
> 'tls-none', 'x509-none',
> 'tls-vnc', 'x509-vnc',
> 'tls-plain', 'x509-plain',
> - 'tls-sasl', 'x509-sasl' ] }
> + 'tls-sasl', 'x509-sasl' ],
> + 'if': 'defined(CONFIG_VNC)' }
>
>
> ##
> @@ -1775,7 +1781,8 @@
> { 'struct': 'VncServerInfo2',
> 'base': 'VncBasicInfo',
> 'data': { 'auth' : 'VncPrimaryAuth',
> - '*vencrypt' : 'VncVencryptSubAuth' } }
> + '*vencrypt' : 'VncVencryptSubAuth' },
> + 'if': 'defined(CONFIG_VNC)' }
>
>
> ##
> @@ -1808,7 +1815,8 @@
> 'clients' : ['VncClientInfo'],
> 'auth' : 'VncPrimaryAuth',
> '*vencrypt' : 'VncVencryptSubAuth',
> - '*display' : 'str' } }
> + '*display' : 'str' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @query-vnc:
> @@ -1839,7 +1847,8 @@
> # }
> #
> ##
> -{ 'command': 'query-vnc', 'returns': 'VncInfo' }
> +{ 'command': 'query-vnc', 'returns': 'VncInfo',
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @query-vnc-servers:
> @@ -1850,7 +1859,8 @@
> #
> # Since: 2.3
> ##
> -{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'] }
> +{ 'command': 'query-vnc-servers', 'returns': ['VncInfo2'],
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @SpiceBasicInfo:
> @@ -3077,8 +3087,8 @@
> # Notes: An empty password in this command will set the password to the empty
> # string. Existing clients are unaffected by executing this command.
> ##
> -{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
> -
> +{ 'command': 'change-vnc-password', 'data': {'password': 'str'},
> + 'if': 'defined(CONFIG_VNC)' }
> ##
> # @change:
> #
> diff --git a/qapi/event.json b/qapi/event.json
> index 6d22b025cc..c8b8e9f384 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -263,7 +263,8 @@
> ##
> { 'event': 'VNC_CONNECTED',
> 'data': { 'server': 'VncServerInfo',
> - 'client': 'VncBasicInfo' } }
> + 'client': 'VncBasicInfo' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VNC_INITIALIZED:
> @@ -290,7 +291,8 @@
> ##
> { 'event': 'VNC_INITIALIZED',
> 'data': { 'server': 'VncServerInfo',
> - 'client': 'VncClientInfo' } }
> + 'client': 'VncClientInfo' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @VNC_DISCONNECTED:
> @@ -316,7 +318,8 @@
> ##
> { 'event': 'VNC_DISCONNECTED',
> 'data': { 'server': 'VncServerInfo',
> - 'client': 'VncClientInfo' } }
> + 'client': 'VncClientInfo' },
> + 'if': 'defined(CONFIG_VNC)' }
>
> ##
> # @SPICE_CONNECTED:
> diff --git a/hmp.c b/hmp.c
> index fd80dce758..9454c634bd 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -605,6 +605,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
> qapi_free_BlockStatsList(stats_list);
> }
>
> +#ifdef CONFIG_VNC
> /* Helper for hmp_info_vnc_clients, _servers */
> static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
> const char *name)
> @@ -692,6 +693,12 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> qapi_free_VncInfo2List(info2l);
>
> }
> +#else
> +void hmp_info_vnc(Monitor *mon, const QDict *qdict)
> +{
> + warn_report("VNC support is disabled");
> +}
> +#endif
>
> #ifdef CONFIG_SPICE
> void hmp_info_spice(Monitor *mon, const QDict *qdict)
> @@ -1708,12 +1715,14 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
> hmp_handle_error(mon, &err);
> }
>
> +#ifdef CONFIG_VNC
> static void hmp_change_read_arg(void *opaque, const char *password,
> void *readline_opaque)
> {
> qmp_change_vnc_password(password, NULL);
> monitor_read_command(opaque, 1);
> }
> +#endif
>
> void hmp_change(Monitor *mon, const QDict *qdict)
> {
> @@ -1724,6 +1733,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> BlockdevChangeReadOnlyMode read_only_mode = 0;
> Error *err = NULL;
>
> +#ifdef CONFIG_VNC
> if (strcmp(device, "vnc") == 0) {
> if (read_only) {
> monitor_printf(mon,
> @@ -1738,7 +1748,9 @@ void hmp_change(Monitor *mon, const QDict *qdict)
> }
> }
> qmp_change("vnc", target, !!arg, arg, &err);
> - } else {
> + } else
> +#endif
> + {
> if (read_only) {
> read_only_mode =
> qapi_enum_parse(BlockdevChangeReadOnlyMode_lookup,
On HMP, see my reply to Dave's review.
> diff --git a/qmp.c b/qmp.c
> index b86201e349..2c90dacb56 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -130,22 +130,6 @@ void qmp_cpu_add(int64_t id, Error **errp)
> }
> }
>
> -#ifndef CONFIG_VNC
> -/* If VNC support is enabled, the "true" query-vnc command is
> - defined in the VNC subsystem */
> -VncInfo *qmp_query_vnc(Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> - return NULL;
> -};
> -
> -VncInfo2List *qmp_query_vnc_servers(Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> - return NULL;
> -};
> -#endif
> -
> #ifndef CONFIG_SPICE
> /*
> * qmp-commands.hx ensures that QMP command query-spice exists only
> @@ -403,23 +387,17 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> qmp_change_vnc_listen(target, errp);
> }
> }
> -#else
> -void qmp_change_vnc_password(const char *password, Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -}
> -static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> - Error **errp)
> -{
> - error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> -}
> #endif /* !CONFIG_VNC */
>
> void qmp_change(const char *device, const char *target,
> bool has_arg, const char *arg, Error **errp)
> {
> if (strcmp(device, "vnc") == 0) {
> +#ifdef CONFIG_VNC
> qmp_change_vnc(target, has_arg, arg, errp);
> +#else
> + error_setg(errp, QERR_FEATURE_DISABLED, "vnc");
> +#endif
> } else {
> qmp_blockdev_change_medium(true, device, false, NULL, target,
> has_arg, arg, false, 0, errp);
Commands you make conditional:
* query-vnc, query-vnc-servers, change-vnc-password
Before the patch, the commands for !CONFIG_VNC are stubs that fail
like this:
{"error": {"class": "GenericError",
"desc": "The feature 'vnc' is not enabled"}}
Afterwards, they fail like this:
{"error": {"class": "CommandNotFound",
"desc": "The command FOO has not been found"}}
I call that an improvement, because it lets clients distinguish
between command unavailable (class CommandNotFound) and command failed
(class GenericError).
Events you make conditional:
* VNC_CONNECTED, VNC_INITIALIZED, VNC_DISCONNECTED
Now let me check for completeness. Occurrences of VNC (case
insensitive) in the schema that aren't covered by your changes:
* add_client
Command has other uses, including "socket bases character devices".
These are unconditional as far as I can tell. Good.
* set_password, expire_password
In theory, these commands could be used for managing any service's
password. In practice, they're used for VNC and SPICE services.
They're documented for "remote display session" / "remote display
server".
The service is selected by argument @protocol. The code special-cases
protocol-specific argument checking, then calls a protocol-specific
function to do the work. If it fails, the command fails with "Could
not set password". It does when the service isn't compiled in (it's a
stub then).
We could make these commands conditional on the conjunction of all
services [currently: defined(CONFIG_VNC) || defined(CONFIG_SPICE)],
but I doubt it's worthwhile.
Okay.
* change
Command has other uses, namely changing media.
Your patch inlines a stub; no functional change. Good.
* QCryptoCipherAlgorithm
This:
# @des-rfb: RFB specific variant of single DES. Do not use except in VNC.
I guess we could compile this out if we wanted to. I doubt we do, but
Dan might have other ideas.
Some of this analysis should perhaps be worked into the commit message.
Overall, the schema syntax works nicely for me. A bit on the verbose
side perhaps, but I like that the conditions are locally obvious.
Observation: we got >250 lines of VNC stuff in qapi-schema.json. Moving
them into qapi/vnc.json would permit proper MAINTAINERS coverage. Gerd,
what do you think?
Gerd, can we delete the vnc_init_func() stub? Things still compile for
me when I do.
diff --git a/include/ui/console.h b/include/ui/console.h
index 7262bef..2c3b2cd 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -484,11 +484,6 @@ static inline QemuOpts *vnc_parse(const char *str, Error **errp)
error_setg(errp, "VNC support is disabled");
return NULL;
}
-static inline int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
-{
- error_setg(errp, "VNC support is disabled");
- return -1;
-}
#endif
/* curses.c */
On Thu, 2017-08-17 at 10:56 +0200, Markus Armbruster wrote:
> Gerd, can we delete the vnc_init_func() stub? Things still compile
> for
> me when I do.
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 7262bef..2c3b2cd 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -484,11 +484,6 @@ static inline QemuOpts *vnc_parse(const char
> *str, Error **errp)
> error_setg(errp, "VNC support is disabled");
> return NULL;
> }
> -static inline int vnc_init_func(void *opaque, QemuOpts *opts, Error
> **errp)
> -{
> - error_setg(errp, "VNC support is disabled");
> - return -1;
> -}
> #endif
Looking at a663fbd9e2f65fae81018d81f231ad79510cf9fb. Guess we want fix
qemu_opts_foreach to not barf on NULL, then revert that commit, so
f8c75b2486 (adding Eduardo to Cc) works as intended.
Beside that: The inline for vnc_init_func looks pointless (it's used
via function pointer). Also it can be empty as it will never be
actually called. Which is fine, vnc_parse will throw an error if
someone tries to use vnc with a binary without vnc support.
cheers,
Gerd
On Wed, Aug 23, 2017 at 05:07:12PM +0200, Gerd Hoffmann wrote:
> On Thu, 2017-08-17 at 10:56 +0200, Markus Armbruster wrote:
> > Gerd, can we delete the vnc_init_func() stub? Things still compile
> > for
> > me when I do.
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 7262bef..2c3b2cd 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -484,11 +484,6 @@ static inline QemuOpts *vnc_parse(const char
> > *str, Error **errp)
> > error_setg(errp, "VNC support is disabled");
> > return NULL;
> > }
> > -static inline int vnc_init_func(void *opaque, QemuOpts *opts, Error
> > **errp)
> > -{
> > - error_setg(errp, "VNC support is disabled");
> > - return -1;
> > -}
> > #endif
>
> Looking at a663fbd9e2f65fae81018d81f231ad79510cf9fb. Guess we want fix
> qemu_opts_foreach to not barf on NULL, then revert that commit, so
> f8c75b2486 (adding Eduardo to Cc) works as intended.
I agree with that plan, but I wouldn't mind having the stub
removed in the meantime.
>
> Beside that: The inline for vnc_init_func looks pointless (it's used
> via function pointer). Also it can be empty as it will never be
> actually called. Which is fine, vnc_parse will throw an error if
> someone tries to use vnc with a binary without vnc support.
True.
--
Eduardo
Hi, > Observation: we got >250 lines of VNC stuff in qapi- > schema.json. Moving > them into qapi/vnc.json would permit proper MAINTAINERS > coverage. Gerd, > what do you think? Yes, looks useful to me. cheers, Gerd
On Thu, Aug 17, 2017 at 09:04:38AM +0200, Markus Armbruster wrote: > Copying our resident VNC maintainer^Wodd fixer Gerd. > > Also copying Dan for QCryptoCipherAlgorithm. > > Gerd, Dan, this patch is about making VNC support visible in > query-qmp-schema, by having the QAPI generators generate suitable > ifdeffery. Bonus: no need for QMP command stubs for > !defined(CONFIG_VNC). [snip] > * QCryptoCipherAlgorithm > > This: > > # @des-rfb: RFB specific variant of single DES. Do not use except in VNC. > > I guess we could compile this out if we wanted to. I doubt we do, but > Dan might have other ideas. It isn't worth the effort to make that conditionally compiled. We getting DES from nettle/gcrypt these days, instead of our custom in-tree desrfb.c file, so its no burden to have it. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi ----- Original Message ----- > On Thu, Aug 17, 2017 at 09:04:38AM +0200, Markus Armbruster wrote: > > Copying our resident VNC maintainer^Wodd fixer Gerd. > > > > Also copying Dan for QCryptoCipherAlgorithm. > > > > Gerd, Dan, this patch is about making VNC support visible in > > query-qmp-schema, by having the QAPI generators generate suitable > > ifdeffery. Bonus: no need for QMP command stubs for > > !defined(CONFIG_VNC). > > [snip] > > > * QCryptoCipherAlgorithm > > > > This: > > > > # @des-rfb: RFB specific variant of single DES. Do not use except in > > VNC. > > > > I guess we could compile this out if we wanted to. I doubt we do, but > > Dan might have other ideas. > > It isn't worth the effort to make that conditionally compiled. We getting > DES from nettle/gcrypt these days, instead of our custom in-tree desrfb.c > file, so its no burden to have it. fyi, the last iteration of the patch made it conditional too.
© 2016 - 2026 Red Hat, Inc.