Re: [Qemu-devel] [PATCH] upgrading emulated UART to 16550A

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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.