From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:50748)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1Us959-0006j6-5W
	for qemu-devel@nongnu.org; Thu, 27 Jun 2013 06:05:41 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1Us957-0001UI-Oi
	for qemu-devel@nongnu.org; Thu, 27 Jun 2013 06:05:39 -0400
Received: from mx1.redhat.com ([209.132.183.28]:54490)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1Us957-0001U7-HI
	for qemu-devel@nongnu.org; Thu, 27 Jun 2013 06:05:37 -0400
Message-ID: <51CC0E6D.1000808@redhat.com>
Date: Thu, 27 Jun 2013 12:05:33 +0200
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
References: <1372246684-12499-1-git-send-email-kraxel@redhat.com>
	<1372246684-12499-2-git-send-email-kraxel@redhat.com>
	<20130626115635.5271afb3@redhat.com>
In-Reply-To: <20130626115635.5271afb3@redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 2/2] fbdev: add monitor commands to
	enable/disable/query
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>, qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>

  Hi,

>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3608,3 +3608,46 @@
>>              '*cpuid-input-ecx': 'int',
>>              'cpuid-register': 'X86CPURegister32',
>>              'features': 'int' } }
>> +
>> +##
>> +# @framebuffer-display:
> 
> Let me bike-shed: we're trying to make command's names verbs. So, we
> could call this framebuffer-display-set or maybe have two commands,
> framebuffer-display-enable and framebuffer-display-disable. I prefer
> the latter.

Can do that.

>> +#
>> +# Enable/disable linux console framebuffer display.
>> +#
>> +# @enable: whenever the framebuffer display should be enabled or disabled.
>> +#
>> +# @scale: #optional enables display scaling, default: off
>> +#
>> +# @device: #optional specifies framebuffer device, default: /dev/fb0
> 
> Actually, it will try to get the device name from an env variable first,
> which sounds too automatic for a building block API like QMP.

The env variable is icky indeed.  But a fixed default (which will be the
one you need in 99% of the cases) is fine IMO.

>> +# Since 1.6
>> +##
>> +{ 'type': 'FramebufferInfo',
>> +  'data': { 'enabled': 'bool',
>> +            '*scale' : 'bool',
>> +            '*device': 'str',
> 
> Why is device optional?

When the framebuffer is'nt active you'll get just
{ "enabled" : "false" }, thats why the other ones are optional.  They
all are filled in case the framebuffer is active.

>> +void qmp_framebuffer_display(bool enable,
>> +                             bool has_scale, bool scale,
>> +                             bool has_device, const char *device,
>> +                             Error **errp)
>> +{
>> +#if defined(CONFIG_FBDEV)
>> +    if (enable) {
>> +        if (fbdev_display_init(has_device ? device : NULL,
>> +                               has_scale  ? scale  : false,
>> +                               errp) != 0) {
>> +            if (!error_is_set(errp)) {
>> +                error_setg(errp, "fbdev initialization failed");
> 
> You should use error_propagate() in order to propagate the error
> information filled by fbdev_display_init().

Why?  What is wrong with simply passing down errp?

> Also, I'd move
> the generic error_setg() above to fbdev_display_init() and make it
> return void.

Makes sense indeed.

cheers,
  Gerd