[Qemu-devel] Re: [PATCH v2] Make VNC support optional

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/11/2011 08:37 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Per default VNC is enabled.
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   Makefile.objs |   19 ++++++++++---------
>   configure     |   37 +++++++++++++++++++++++++------------
>   console.h     |   26 ++++++++++++++++++++++++--
>   monitor.c     |   22 ++++++++++------------
>   qerror.h      |    3 +++
>   ui/vnc.c      |   14 ++++++++++----
>   vl.c          |   10 +++++++++-
>   7 files changed, 91 insertions(+), 40 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 9e98a66..58388e2 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -127,19 +127,20 @@ common-obj-y += $(addprefix audio/, $(audio-obj-y))
>   ui-obj-y += keymaps.o
>   ui-obj-$(CONFIG_SDL) += sdl.o sdl_zoom.o x_keymap.o
>   ui-obj-$(CONFIG_CURSES) += curses.o
> -ui-obj-y += vnc.o d3des.o
> -ui-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
> -ui-obj-y += vnc-enc-tight.o vnc-palette.o
> -ui-obj-y += vnc-enc-zrle.o
> -ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
> -ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
> -ui-obj-$(CONFIG_COCOA) += cocoa.o
> +vnc-obj-y += vnc.o d3des.o
> +vnc-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
> +vnc-obj-y += vnc-enc-tight.o vnc-palette.o
> +vnc-obj-y += vnc-enc-zrle.o
> +vnc-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
> +vnc-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
> +vnc-obj-$(CONFIG_COCOA) += cocoa.o
>   ifdef CONFIG_VNC_THREAD
> -ui-obj-y += vnc-jobs-async.o
> +vnc-obj-y += vnc-jobs-async.o
>   else
> -ui-obj-y += vnc-jobs-sync.o
> +vnc-obj-y += vnc-jobs-sync.o
>   endif
>   common-obj-y += $(addprefix ui/, $(ui-obj-y))
> +common-obj-$(CONFIG_VNC) += $(addprefix ui/, $(vnc-obj-y))
>
>   common-obj-y += iov.o acl.o
>   common-obj-$(CONFIG_THREAD) += qemu-thread.o
> diff --git a/configure b/configure
> index 39cdf2b..a64b750 100755
> --- a/configure
> +++ b/configure
> @@ -117,6 +117,7 @@ kvm=""
>   kvm_para=""
>   nptl=""
>   sdl=""
> +vnc="yes"
>   sparse="no"
>   uuid=""
>   vde=""
> @@ -539,6 +540,10 @@ for opt do
>     ;;
>     --enable-sdl) sdl="yes"
>     ;;
> +  --disable-vnc) vnc="no"
> +  ;;
> +  --enable-vnc) vnc="yes"
> +  ;;
>     --fmod-lib=*) fmod_lib="$optarg"
>     ;;
>     --fmod-inc=*) fmod_inc="$optarg"
> @@ -836,6 +841,8 @@ echo "  --disable-strip          disable stripping binaries"
>   echo "  --disable-werror         disable compilation abort on warning"
>   echo "  --disable-sdl            disable SDL"
>   echo "  --enable-sdl             enable SDL"
> +echo "  --disable-vnc            disable VNC"
> +echo "  --enable-vnc             enable VNC"
>   echo "  --enable-cocoa           enable COCOA (Mac OS X only)"
>   echo "  --audio-drv-list=LIST    set audio drivers list:"
>   echo "                           Available drivers: $audio_possible_drivers"
> @@ -1273,7 +1280,7 @@ fi
>
>   ##########################################
>   # VNC TLS detection
> -if test "$vnc_tls" != "no" ; then
> +if test "$vnc" = "yes" -a "$vnc_tls" != "no" ; then
>     cat>  $TMPC<<EOF
>   #include<gnutls/gnutls.h>
>   int main(void) { gnutls_session_t s; gnutls_init(&s, GNUTLS_SERVER); return 0; }
> @@ -1293,7 +1300,7 @@ fi
>
>   ##########################################
>   # VNC SASL detection
> -if test "$vnc_sasl" != "no" ; then
> +if test "$vnc" = "yes" -a "$vnc_sasl" != "no" ; then
>     cat>  $TMPC<<EOF
>   #include<sasl/sasl.h>
>   #include<stdio.h>
> @@ -1315,7 +1322,7 @@ fi
>
>   ##########################################
>   # VNC JPEG detection
> -if test "$vnc_jpeg" != "no" ; then
> +if test "$vnc" = "yes" -a "$vnc_jpeg" != "no" ; then
>   cat>  $TMPC<<EOF
>   #include<stdio.h>
>   #include<jpeglib.h>
> @@ -1336,7 +1343,7 @@ fi
>
>   ##########################################
>   # VNC PNG detection
> -if test "$vnc_png" != "no" ; then
> +if test "$vnc" = "yes" -a "$vnc_png" != "no" ; then
>   cat>  $TMPC<<EOF
>   //#include<stdio.h>
>   #include<png.h>
> @@ -2495,11 +2502,14 @@ echo "Audio drivers     $audio_drv_list"
>   echo "Extra audio cards $audio_card_list"
>   echo "Block whitelist   $block_drv_whitelist"
>   echo "Mixer emulation   $mixemu"
> -echo "VNC TLS support   $vnc_tls"
> -echo "VNC SASL support  $vnc_sasl"
> -echo "VNC JPEG support  $vnc_jpeg"
> -echo "VNC PNG support   $vnc_png"
> -echo "VNC thread        $vnc_thread"
> +echo "VNC support       $vnc"
> +if test "$vnc" = "yes" ; then
> +    echo "VNC TLS support   $vnc_tls"
> +    echo "VNC SASL support  $vnc_sasl"
> +    echo "VNC JPEG support  $vnc_jpeg"
> +    echo "VNC PNG support   $vnc_png"
> +    echo "VNC thread        $vnc_thread"
> +fi
>   if test -n "$sparc_cpu"; then
>       echo "Target Sparc Arch $sparc_cpu"
>   fi
> @@ -2649,6 +2659,9 @@ echo "CONFIG_BDRV_WHITELIST=$block_drv_whitelist">>  $config_host_mak
>   if test "$mixemu" = "yes" ; then
>     echo "CONFIG_MIXEMU=y">>  $config_host_mak
>   fi
> +if test "$vnc" = "yes" ; then
> +  echo "CONFIG_VNC=y">>  $config_host_mak
> +fi
>   if test "$vnc_tls" = "yes" ; then
>     echo "CONFIG_VNC_TLS=y">>  $config_host_mak
>     echo "VNC_TLS_CFLAGS=$vnc_tls_cflags">>  $config_host_mak
> @@ -2657,15 +2670,15 @@ if test "$vnc_sasl" = "yes" ; then
>     echo "CONFIG_VNC_SASL=y">>  $config_host_mak
>     echo "VNC_SASL_CFLAGS=$vnc_sasl_cflags">>  $config_host_mak
>   fi
> -if test "$vnc_jpeg" != "no" ; then
> +if test "$vnc_jpeg" = "yes" ; then
>     echo "CONFIG_VNC_JPEG=y">>  $config_host_mak
>     echo "VNC_JPEG_CFLAGS=$vnc_jpeg_cflags">>  $config_host_mak
>   fi
> -if test "$vnc_png" != "no" ; then
> +if test "$vnc_png" = "yes" ; then
>     echo "CONFIG_VNC_PNG=y">>  $config_host_mak
>     echo "VNC_PNG_CFLAGS=$vnc_png_cflags">>  $config_host_mak
>   fi
> -if test "$vnc_thread" != "no" ; then
> +if test "$vnc_thread" = "yes" ; then
>     echo "CONFIG_VNC_THREAD=y">>  $config_host_mak
>     echo "CONFIG_THREAD=y">>  $config_host_mak
>   fi
> diff --git a/console.h b/console.h
> index 87d3271..50942bb 100644
> --- a/console.h
> +++ b/console.h
> @@ -4,6 +4,8 @@
>   #include "qemu-char.h"
>   #include "qdict.h"
>   #include "notify.h"
> +#include "qerror.h"
> +#include "monitor.h"
>
>   /* keyboard/mouse support */
>
> @@ -371,12 +373,32 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
>   void vnc_display_init(DisplayState *ds);
>   void vnc_display_close(DisplayState *ds);
>   int vnc_display_open(DisplayState *ds, const char *display);
> -int vnc_display_password(DisplayState *ds, const char *password);
>   int vnc_display_disable_login(DisplayState *ds);
> +char *vnc_display_local_addr(DisplayState *ds);
> +#ifdef CONFIG_VNC
> +int vnc_display_password(DisplayState *ds, const char *password);
>   int vnc_display_pw_expire(DisplayState *ds, time_t expires);
>   void do_info_vnc_print(Monitor *mon, const QObject *data);
>   void do_info_vnc(Monitor *mon, QObject **ret_data);
> -char *vnc_display_local_addr(DisplayState *ds);
> +#else
> +static inline int vnc_display_password(DisplayState *ds, const char *password)
> +{
> +    qerror_report(QERR_FEATURE_DISABLED, "vnc");
> +    return -ENODEV;
> +}
> +static inline int vnc_display_pw_expire(DisplayState *ds, time_t expires)
> +{
> +    qerror_report(QERR_FEATURE_DISABLED, "vnc");
> +    return -ENODEV;
> +};
> +static inline void do_info_vnc(Monitor *mon, QObject **ret_data)
> +{
> +};
> +static inline void do_info_vnc_print(Monitor *mon, const QObject *data)
> +{
> +    monitor_printf(mon, "VNC support disabled\n");
> +};
> +#endif
>
>   /* curses.c */
>   void curses_display_init(DisplayState *ds, int full_screen);
> diff --git a/monitor.c b/monitor.c
> index 22ae3bb..4a54c55 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1016,6 +1016,7 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
>       return 0;
>   }
>
> +#ifdef CONFIG_VNC
>   static int change_vnc_password(const char *password)
>   {
>       if (!password || !password[0]) {
> @@ -1062,6 +1063,13 @@ static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
>
>       return 0;
>   }
> +#else
> +static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
> +{
> +    qerror_report(QERR_FEATURE_DISABLED, "vnc");
> +    return -ENODEV;
> +}
> +#endif
>
>   /**
>    * do_change(): Change a removable medium, or VNC configuration
> @@ -1127,12 +1135,7 @@ static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>           }
>           /* Note that setting an empty password will not disable login through
>            * this interface. */
> -        rc = vnc_display_password(NULL, password);
> -        if (rc != 0) {
> -            qerror_report(QERR_SET_PASSWD_FAILED);
> -            return -1;
> -        }
> -        return 0;
> +        return vnc_display_password(NULL, password);
>       }
>
>       qerror_report(QERR_INVALID_PARAMETER, "protocol");
> @@ -1171,12 +1174,7 @@ static int expire_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>       }
>
>       if (strcmp(protocol, "vnc") == 0) {
> -        rc = vnc_display_pw_expire(NULL, when);
> -        if (rc != 0) {
> -            qerror_report(QERR_SET_PASSWD_FAILED);
> -            return -1;
> -        }
> -        return 0;
> +        return vnc_display_pw_expire(NULL, when);
>       }
>
>       qerror_report(QERR_INVALID_PARAMETER, "protocol");
> diff --git a/qerror.h b/qerror.h
> index f732d45..df61d2c 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -171,4 +171,7 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_VNC_SERVER_FAILED \
>       "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
>
> +#define QERR_FEATURE_DISABLED \
> +    "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
> +
>   #endif /* QERROR_H */
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 34dc0cd..dd7a44a 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2638,16 +2638,19 @@ int vnc_display_disable_login(DisplayState *ds)
>
>   int vnc_display_password(DisplayState *ds, const char *password)
>   {
> +    int ret = 0;
>       VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
>
>       if (!vs) {
> -        return -1;
> +        ret = -EINVAL;
> +        goto out;
>       }
>
>       if (!password) {
>           /* This is not the intention of this interface but err on the side
>              of being safe */
> -        return vnc_display_disable_login(ds);
> +        ret = vnc_display_disable_login(ds);
> +        goto out;
>       }
>
>       if (vs->password) {
> @@ -2656,8 +2659,11 @@ int vnc_display_password(DisplayState *ds, const char *password)
>       }
>       vs->password = qemu_strdup(password);
>       vs->auth = VNC_AUTH_VNC;
> -
> -    return 0;
> +out:
> +    if (ret != 0) {
> +        qerror_report(QERR_SET_PASSWD_FAILED);
> +    }
> +    return ret;
>   }
>
>   int vnc_display_pw_expire(DisplayState *ds, time_t expires)
> diff --git a/vl.c b/vl.c
> index a1faf65..08a1d72 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -208,7 +208,9 @@ int smp_cpus = 1;
>   int max_cpus = 0;
>   int smp_cores = 1;
>   int smp_threads = 1;
> +#ifdef CONFIG_VNC
>   const char *vnc_display;
> +#endif
>   int shmem_video = 0;
>   int acpi_enabled = 1;
>   int no_hpet = 0;
> @@ -1940,7 +1942,9 @@ int main(int argc, char **argv, char **envp)
>       int tb_size;
>       const char *pid_file = NULL;
>       const char *incoming = NULL;
> +#ifdef CONFIG_VNC
>       int show_vnc_port = 0;
> +#endif
>       int defconfig = 1;
>
>   #ifdef CONFIG_SIMPLE_TRACE
> @@ -2585,10 +2589,12 @@ int main(int argc, char **argv, char **envp)
>                       exit(1);
>                   }
>                   break;
> +#ifdef CONFIG_VNC
>   	    case QEMU_OPTION_vnc:
>                   display_remote++;
>   		vnc_display = optarg;
>   		break;
> +#endif

Sorry, should have mentioned it in the last note, but same rules apply 
to command line options.  Instead of #if 0's thing out, we should print 
a friendly message saying that this feature is disabled.

>               case QEMU_OPTION_shmem_video:
>                   shmem_video = 1;
>                   break;
> @@ -3049,7 +3055,7 @@ int main(int argc, char **argv, char **envp)
>       if (display_type == DT_DEFAULT&&  !display_remote) {
>   #if defined(CONFIG_SDL) || defined(CONFIG_COCOA)
>           display_type = DT_SDL;
> -#else
> +#elif defined(CONFIG_VNC)
>           vnc_display = "localhost:0,to=99";
>           show_vnc_port = 1;
>   #endif
> @@ -3078,6 +3084,7 @@ int main(int argc, char **argv, char **envp)
>           break;
>       }
>
> +#ifdef CONFIG_VNC
>       /* init remote displays */
>       if (vnc_display) {
>           vnc_display_init(ds);
> @@ -3088,6 +3095,7 @@ int main(int argc, char **argv, char **envp)
>               printf("VNC server running on `%s'\n", vnc_display_local_addr(ds));
>           }
>       }
> +#endif

So what ends up being the default display if VNC and SDL are both 
disabled?  Have you tested this?

Regards,

Anthony Liguori

>   #ifdef CONFIG_SPICE
>       if (using_spice&&  !qxl_enabled) {
>           qemu_spice_display_init(ds);