From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37320 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Py2tj-0000Lt-4N for qemu-devel@nongnu.org; Fri, 11 Mar 2011 09:00:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Py2ti-00016e-8V for qemu-devel@nongnu.org; Fri, 11 Mar 2011 09:00:55 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:49052) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Py2ti-00016H-5J for qemu-devel@nongnu.org; Fri, 11 Mar 2011 09:00:54 -0500 Received: by iym7 with SMTP id 7so3097893iym.4 for ; Fri, 11 Mar 2011 06:00:52 -0800 (PST) Message-ID: <4D7A2B11.2080807@codemonkey.ws> Date: Fri, 11 Mar 2011 08:00:49 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] Make VNC support optional References: <1299847134-5854-1-git-send-email-Jes.Sorensen@redhat.com> <4D7A255B.4000409@codemonkey.ws> <4D7A26F7.1010901@redhat.com> In-Reply-To: <4D7A26F7.1010901@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes Sorensen Cc: qemu-devel@nongnu.org On 03/11/2011 07:43 AM, Jes Sorensen wrote: > On 03/11/11 14:36, Anthony Liguori wrote: >>> diff --git a/monitor.c b/monitor.c >>> index 22ae3bb..4425315 100644 >>> --- a/monitor.c >>> +++ b/monitor.c >>> @@ -441,6 +441,7 @@ void monitor_protocol_event(MonitorEvent event, >>> QObject *data) >>> case QEVENT_RESUME: >>> event_name = "RESUME"; >>> break; >>> +#ifdef CONFIG_VNC >>> case QEVENT_VNC_CONNECTED: >>> event_name = "VNC_CONNECTED"; >>> break; >>> @@ -450,6 +451,7 @@ void monitor_protocol_event(MonitorEvent event, >>> QObject *data) >>> case QEVENT_VNC_DISCONNECTED: >>> event_name = "VNC_DISCONNECTED"; >>> break; >>> +#endif >> No need to if this out. > I realized that, but I figured it would reduce the code a bit more. I'll > leave it in. fwiw, it's all gone in my tree so you don't need to worry about it sticking around :-) > @@ -3002,6 +3014,7 @@ static const mon_cmd_t info_cmds[] = { >>> .user_print = do_info_mice_print, >>> .mhandler.info_new = do_info_mice, >>> }, >>> +#ifdef CONFIG_VNC >>> { >>> .name = "vnc", >>> .args_type = "", >>> @@ -3010,6 +3023,7 @@ static const mon_cmd_t info_cmds[] = { >>> .user_print = do_info_vnc_print, >>> .mhandler.info_new = do_info_vnc, >>> }, >>> +#endif >> We don't want to hide commands based on compile settings. >> >> Otherwise, looks good. > I am not sure I follow you here, you prefer to have the commands visible > even though they are disabled? There are other commands which get > disabled like this as well. Yeah and that's wrong and I've fixed that in my QAPI branch. If a command is #if 0'd out, then it doesn't show up in query-commands and if you try to use it, it returns a CommandNotFound which is the same error as any garbage command would produce. But query-commands is not just useful for feature checking, but also for getting the schema to generate bindings. We have this problem with command line options today, there's no good way to ask QEMU for every possible command line option if you're trying to build tooling around it because it depends on how QEMU is configured. Having the command and returning FeatureDisabled means that QEMU still can advertise the command's schema and a user can differentiate between a feature that's explicitly disabled, for instance, in RHEL for support concerns, and a command that this QEMU just knows nothing about. This is nice for a GUI because you can expose a different error message to the user. For CommandNotFound, you might say, "This version of QEMU is too old for this feature" whereas for FeatureDisabled, you would say, "This version of QEMU disables this feature, contact your vendor." Regards, Anthony Liguori > Cheers, > Jes >