From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1KRT3z-0003NI-MV
	for qemu-devel@nongnu.org; Fri, 08 Aug 2008 10:35:31 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1KRT3y-0003N6-73
	for qemu-devel@nongnu.org; Fri, 08 Aug 2008 10:35:31 -0400
Received: from [199.232.76.173] (port=50638 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1KRT3y-0003N3-3O
	for qemu-devel@nongnu.org; Fri, 08 Aug 2008 10:35:30 -0400
Received: from smtp.eu.citrix.com ([62.200.22.115]:58190)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <Stefano.Stabellini@eu.citrix.com>)
	id 1KRT3x-000102-Rc
	for qemu-devel@nongnu.org; Fri, 08 Aug 2008 10:35:30 -0400
Message-ID: <489C5A02.6080101@eu.citrix.com>
Date: Fri, 08 Aug 2008 15:36:50 +0100
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] upgrading emulated UART to 16550A
References: <489B3278.4080001@eu.citrix.com> <489B45E3.9040005@codemonkey.ws>
In-Reply-To: <489B45E3.9040005@codemonkey.ws>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: qemu-devel@nongnu.org

Anthony Liguori wrote:

> 
> I think the idea was to add a new copyright entry, not to modify
> Fabrice's copyright.


Oops, I am going to fix that.

>> +
>> +#include <termios.h>
>> +#include <sys/ioctl.h>
>>   
> 
> This doesn't look Windows friendly.


I am going to follow Samuel advice and declare more constants in
qemu-char.h so that we don't need those two headers anymore.

 
> 
> minor nit, but the '{' should be on a new line.
> 


...

> More nits, lose the whitespace in the conditions.  For instance:

> 
> } else if ( ( s->ier & UART_IER_RDI )  =>  } else if ((s->ier &
> UART_IER_RDI).
> 
> The rest of the file uses the later style so it's a little weird to have
> portions of the code be different.
> 

...

> 
> More bad whitespace.
> 

...

> 
> This is just nutty :-)  Please rewrite this if() statement to be a
> little less obscure.
> 

All these style problems will be fixed in the next version.

>> +    serial_init_core(s, irq, baudbase, chr);
>>      register_savevm("serial", base, 2, serial_save, serial_load, s);
>>   
> 
> Isn't it necessary to bump the savevm() version number since you've
> changed the format.
> 

Yes, you are right.
Thanks for this very helpful review.