[Qemu-devel] [PATCH] Document Qemu coding style

[Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

With the help of some Limoncino I noted several aspects of the Qemu coding
style, particularly where it differs from the Linux coding style as many
contributors work on both projects.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 CODING_STYLE |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100644 CODING_STYLE

diff --git a/CODING_STYLE b/CODING_STYLE
new file mode 100644
index 0000000..54fdeff
--- /dev/null
+++ b/CODING_STYLE
@@ -0,0 +1,77 @@
+Qemu Coding Style
+=================
+
+1. Whitespace
+
+Of course, the most important aspect in any coding style is whitespace.
+Crusty old coders who have trouble spotting the glasses on their noses
+can tell the difference between a tab and eight spaces from a distance
+of approximately fifteen parsecs.  Many a flamewar have been fought and
+lost on this issue.
+
+Qemu indents are four spaces.  Tabs are never used, except in Makefiles
+where they have been irreversibly coded into the syntax by some moron.
+Spaces of course are superior to tabs because:
+
+ - You have just one way to specify whitespace, not two.  Ambiguity breeds
+   mistakes.
+ - The confusion surrounding 'use tabs to indent, spaces to justify' is gone.
+ - Tab indents push your code to the right, making your screen seriously
+   unbalanced.
+ - Tabs will be rendered incorrectly on editors who are misconfigured not
+   to use tab stops of eight positions.
+ - Tabs are rendered badly in patches, causing off-by-one errors in almost
+   every line.
+ - It is the Qemu coding style.
+
+2. Line width
+
+Lines are 80 characters wide plus some slop.  Try to fit your code into
+eighty characters, but if it makes a snippet particularly ugly, allow
+yourself some slack.  Don't overdo it though.
+
+Rationale:
+ - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
+   xterms and use vi in all of them.  The best way to punish them is to
+   let them keep doing it.
+ - Code and especially patches is much more readable if limited to a sane
+   line length.  Eighty is traditional.
+ - It is the Qemu coding style.
+
+3. Naming
+
+Variables are lower_case_with_underscores; easy to type and read.  Structured
+type names are in CamelCase; harder to type but standing out.  Scalar type
+names are lower_case_with_underscores_ending_with_a_t, like the Posix
+uint64_t and family.
+
+Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
+Qemu coding style.
+
+4. Block structure
+
+Every indented statement is braced; even if the block contains just one
+statement.  The opening brace is on the line that contains the control
+flow statement that introduces the new block; the closing brace is on the
+same line as the else keyword, or on a line by itself if there is no else
+keyword.  Example:
+
+    if (a == 5) {
+        printf("a was 5.\n");
+    } else if (a == 6) {
+        printf("a was 6.\n");
+    } else {
+        printf("a was something else entirely.\n");
+    }
+
+An exception is the opening brace for a function; for reasons of tradition
+and clarity it comes on a line by itself:
+
+    void a_function(void)
+    {
+        do_something();
+    }
+
+Rationale: a consistent (except for functions...) bracing style reduces
+ambiguity and avoids needless churn when lines are added or removed.
+Furthermore, it is the Qemu coding style.
-- 
1.6.1.1

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[malc]

On Mon, 30 Mar 2009, Avi Kivity wrote:

> With the help of some Limoncino I noted several aspects of the Qemu coding

QEMU

> style, particularly where it differs from the Linux coding style as many
> contributors work on both projects.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  CODING_STYLE |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 77 insertions(+), 0 deletions(-)
>  create mode 100644 CODING_STYLE
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> new file mode 100644
> index 0000000..54fdeff
> --- /dev/null
> +++ b/CODING_STYLE
> @@ -0,0 +1,77 @@
> +Qemu Coding Style
> +=================
> +
> +1. Whitespace
> +
> +Of course, the most important aspect in any coding style is whitespace.
> +Crusty old coders who have trouble spotting the glasses on their noses
> +can tell the difference between a tab and eight spaces from a distance
> +of approximately fifteen parsecs.  Many a flamewar have been fought and
> +lost on this issue.
> +
> +Qemu indents are four spaces.  Tabs are never used, except in Makefiles
> +where they have been irreversibly coded into the syntax by some moron.
> +Spaces of course are superior to tabs because:
> +
> + - You have just one way to specify whitespace, not two.  Ambiguity breeds
> +   mistakes.
> + - The confusion surrounding 'use tabs to indent, spaces to justify' is gone.
> + - Tab indents push your code to the right, making your screen seriously
> +   unbalanced.
> + - Tabs will be rendered incorrectly on editors who are misconfigured not
> +   to use tab stops of eight positions.
> + - Tabs are rendered badly in patches, causing off-by-one errors in almost
> +   every line.
> + - It is the Qemu coding style.
> +
> +2. Line width
> +
> +Lines are 80 characters wide plus some slop.  Try to fit your code into
> +eighty characters, but if it makes a snippet particularly ugly, allow
> +yourself some slack.  Don't overdo it though.
> +
> +Rationale:
> + - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
> +   xterms and use vi in all of them.  The best way to punish them is to
> +   let them keep doing it.
> + - Code and especially patches is much more readable if limited to a sane
> +   line length.  Eighty is traditional.
> + - It is the Qemu coding style.
> +
> +3. Naming
> +
> +Variables are lower_case_with_underscores; easy to type and read.  Structured
> +type names are in CamelCase; harder to type but standing out.  Scalar type
> +names are lower_case_with_underscores_ending_with_a_t, like the Posix
> +uint64_t and family.

POSIX

anything_than_ends_with_a_t is reserved
(http://www.opengroup.org/pubs/online/7908799/xns/namespace.html)

QEMU does use quite a few of types ending with a _t and this should
be fixed.

> +
> +Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
> +Qemu coding style.
> +
> +4. Block structure
> +
> +Every indented statement is braced; even if the block contains just one
> +statement.  The opening brace is on the line that contains the control
> +flow statement that introduces the new block; the closing brace is on the
> +same line as the else keyword, or on a line by itself if there is no else
> +keyword.  Example:
> +
> +    if (a == 5) {
> +        printf("a was 5.\n");
> +    } else if (a == 6) {
> +        printf("a was 6.\n");
> +    } else {
> +        printf("a was something else entirely.\n");
> +    }
> +
> +An exception is the opening brace for a function; for reasons of tradition
> +and clarity it comes on a line by itself:
> +
> +    void a_function(void)
> +    {
> +        do_something();
> +    }
> +
> +Rationale: a consistent (except for functions...) bracing style reduces
> +ambiguity and avoids needless churn when lines are added or removed.
> +Furthermore, it is the Qemu coding style.
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Blue Swirl]

On 3/30/09, Avi Kivity <avi@redhat.com> wrote:
> With the help of some Limoncino I noted several aspects of the Qemu coding
>  style, particularly where it differs from the Linux coding style as many
>  contributors work on both projects.

Ok, I'm armed with some Foster's.

>
>  Signed-off-by: Avi Kivity <avi@redhat.com>
>  ---
>   CODING_STYLE |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 77 insertions(+), 0 deletions(-)
>   create mode 100644 CODING_STYLE
>
>  diff --git a/CODING_STYLE b/CODING_STYLE
>  new file mode 100644
>  index 0000000..54fdeff
>  --- /dev/null
>  +++ b/CODING_STYLE
>  @@ -0,0 +1,77 @@
>  +Qemu Coding Style
>  +=================
>  +
>  +1. Whitespace
>  +
>  +Of course, the most important aspect in any coding style is whitespace.
>  +Crusty old coders who have trouble spotting the glasses on their noses
>  +can tell the difference between a tab and eight spaces from a distance
>  +of approximately fifteen parsecs.  Many a flamewar have been fought and
>  +lost on this issue.
>  +
>  +Qemu indents are four spaces.  Tabs are never used, except in Makefiles
>  +where they have been irreversibly coded into the syntax by some moron.
>  +Spaces of course are superior to tabs because:
>  +
>  + - You have just one way to specify whitespace, not two.  Ambiguity breeds
>  +   mistakes.
>  + - The confusion surrounding 'use tabs to indent, spaces to justify' is gone.
>  + - Tab indents push your code to the right, making your screen seriously
>  +   unbalanced.
>  + - Tabs will be rendered incorrectly on editors who are misconfigured not
>  +   to use tab stops of eight positions.
>  + - Tabs are rendered badly in patches, causing off-by-one errors in almost
>  +   every line.
>  + - It is the Qemu coding style.

Never leave whitespace at the end of line, it annoys the Quilt. Empty
lines at the end of file just make files bigger.

>  +
>  +2. Line width
>  +
>  +Lines are 80 characters wide plus some slop.  Try to fit your code into
>  +eighty characters, but if it makes a snippet particularly ugly, allow
>  +yourself some slack.  Don't overdo it though.
>  +
>  +Rationale:
>  + - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
>  +   xterms and use vi in all of them.  The best way to punish them is to
>  +   let them keep doing it.
>  + - Code and especially patches is much more readable if limited to a sane
>  +   line length.  Eighty is traditional.
>  + - It is the Qemu coding style.
>  +
>  +3. Naming
>  +
>  +Variables are lower_case_with_underscores; easy to type and read.  Structured
>  +type names are in CamelCase; harder to type but standing out.  Scalar type
>  +names are lower_case_with_underscores_ending_with_a_t, like the Posix
>  +uint64_t and family.
>  +
>  +Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>  +Qemu coding style.
>  +
>  +4. Block structure
>  +
>  +Every indented statement is braced; even if the block contains just one
>  +statement.

I'd remove this, braces are not used consistently for one statement blocks.

>  The opening brace is on the line that contains the control
>  +flow statement that introduces the new block; the closing brace is on the
>  +same line as the else keyword, or on a line by itself if there is no else
>  +keyword.  Example:
>  +
>  +    if (a == 5) {
>  +        printf("a was 5.\n");
>  +    } else if (a == 6) {
>  +        printf("a was 6.\n");
>  +    } else {
>  +        printf("a was something else entirely.\n");
>  +    }
>  +
>  +An exception is the opening brace for a function; for reasons of tradition
>  +and clarity it comes on a line by itself:
>  +
>  +    void a_function(void)
>  +    {
>  +        do_something();
>  +    }
>  +
>  +Rationale: a consistent (except for functions...) bracing style reduces
>  +ambiguity and avoids needless churn when lines are added or removed.
>  +Furthermore, it is the Qemu coding style.

No, this is the K&R style. Quoting linux/Documentation/CodingStyle:

Heretic people all over the world have claimed that this inconsistency
is ...  well ...  inconsistent, but all right-thinking people know that
(a) K&R are _right_ and (b) K&R are right.  Besides, functions are
special anyway (you can't nest them in C).

Cheers!

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[M. Warner Losh]

In message: <f43fc5580903301128y2a8432f4u6b7d9375e29a5c82@mail.gmail.com>
            Blue Swirl <blauwirbel@gmail.com> writes:
: >  +An exception is the opening brace for a function; for reasons of tradition
: >  +and clarity it comes on a line by itself:
: >  +
: >  +    void a_function(void)
: >  +    {
: >  +        do_something();
: >  +    }
: >  +
: >  +Rationale: a consistent (except for functions...) bracing style reduces
: >  +ambiguity and avoids needless churn when lines are added or removed.
: >  +Furthermore, it is the Qemu coding style.
: 
: No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
: 
: Heretic people all over the world have claimed that this inconsistency
: is ...  well ...  inconsistent, but all right-thinking people know that
: (a) K&R are _right_ and (b) K&R are right.  Besides, functions are
: special anyway (you can't nest them in C).

And besides, there's lots of almost-smart tools that operate on source
code that know this is always true...  Or at least historically that's
been the case, don't know of any widely used ones today, but I doubt
they were all fixed...

Warner

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

Blue Swirl wrote:
>>  +4. Block structure
>>  +
>>  +Every indented statement is braced; even if the block contains just one
>>  +statement.
>>     
>
> I'd remove this, braces are not used consistently for one statement blocks.
>
>   

While that's true, I'd like to keep this.  I found (after initially 
being annoyed by this; I dislike punctuation) that it's nice not to need 
to rebrace after adding or removing lines.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

M. Warner Losh wrote:
> In message: <f43fc5580903301128y2a8432f4u6b7d9375e29a5c82@mail.gmail.com>
>             Blue Swirl <blauwirbel@gmail.com> writes:
> : >  +An exception is the opening brace for a function; for reasons of tradition
> : >  +and clarity it comes on a line by itself:
> : >  +
> : >  +    void a_function(void)
> : >  +    {
> : >  +        do_something();
> : >  +    }
> : >  +
> : >  +Rationale: a consistent (except for functions...) bracing style reduces
> : >  +ambiguity and avoids needless churn when lines are added or removed.
> : >  +Furthermore, it is the Qemu coding style.
> : 
> : No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
> : 
> : Heretic people all over the world have claimed that this inconsistency
> : is ...  well ...  inconsistent, but all right-thinking people know that
> : (a) K&R are _right_ and (b) K&R are right.  Besides, functions are
> : special anyway (you can't nest them in C).
>
> And besides, there's lots of almost-smart tools that operate on source
> code that know this is always true...  Or at least historically that's
> been the case, don't know of any widely used ones today, but I doubt
> they were all fixed...
>   

Relax, no one wants to change this...

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

Blue Swirl wrote:
>>  +Rationale: a consistent (except for functions...) bracing style reduces
>>  +ambiguity and avoids needless churn when lines are added or removed.
>>  +Furthermore, it is the Qemu coding style.
>>     
>
> No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
>   

No, it is the Qemu^WQEMU coding style.  QEMU.  QEMU.  QEMUUUUUUUUUUUUUUUUUU!

Mu.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Glauber Costa]

On Mon, Mar 30, 2009 at 4:58 PM, Avi Kivity <avi@redhat.com> wrote:
> Blue Swirl wrote:
>>>
>>>  +Rationale: a consistent (except for functions...) bracing style reduces
>>>  +ambiguity and avoids needless churn when lines are added or removed.
>>>  +Furthermore, it is the Qemu coding style.
>>>
>>
>> No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
>>
>
> No, it is the Qemu^WQEMU coding style.  QEMU.  QEMU.  QEMUUUUUUUUUUUUUUUUUU!
>
> Mu.
Avi,

have you been drinking?

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Andreas Färber]

Am 30.03.2009 um 20:28 schrieb Blue Swirl:

> Besides, functions are
> special anyway (you can't nest them in C).

Nesting functions used to work in GCC... :)

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

Glauber Costa wrote:
> have you been drinking?
>   

No.  Why do you ask?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Glauber Costa]

On Mon, Mar 30, 2009 at 5:35 PM, Avi Kivity <avi@redhat.com> wrote:
> Glauber Costa wrote:
>>
>> have you been drinking?
>>
>
> No.  Why do you ask?

I would like to have some.


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Lennart Sorensen]

On Mon, Mar 30, 2009 at 10:54:58PM +0300, Avi Kivity wrote:
> Blue Swirl wrote:
>>>  +4. Block structure
>>>  +
>>>  +Every indented statement is braced; even if the block contains just one
>>>  +statement.
>>>     
>>
>> I'd remove this, braces are not used consistently for one statement blocks.
>>
>>   
>
> While that's true, I'd like to keep this.  I found (after initially  
> being annoyed by this; I dislike punctuation) that it's nice not to need  
> to rebrace after adding or removing lines.

I hate having to add braces to add a printf when debuging something.
If you forget the braces, you change the meaning of the code.  To me
that's a disaster and enough reason that leaving out braces should
be banned.  Code style that encourages creation of bugs is bad style.
Surprisingly many code styles don't get this however.

I also like consistency, and since you need braces for multiline
statements, why not always use them?

-- 
Len Sorensen

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Lennart Sorensen]

On Mon, Mar 30, 2009 at 09:28:54PM +0300, Blue Swirl wrote:
> I'd remove this, braces are not used consistently for one statement blocks.

Then fix that.  Making the coding style worse just because some people
have failed to follow it is not the right solution.

> No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
> 
> Heretic people all over the world have claimed that this inconsistency
> is ...  well ...  inconsistent, but all right-thinking people know that
> (a) K&R are _right_ and (b) K&R are right.  Besides, functions are
> special anyway (you can't nest them in C).

And Linus is wrong for not requiring braces at all times in the kernel. :)
I don't have much hope of convincing him of that, but he is still wrong.

-- 
Len Sorensen

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[M. Warner Losh]

In message: <20090330214321.GP3795@csclub.uwaterloo.ca>
            lsorense@csclub.uwaterloo.ca (Lennart Sorensen) writes:
: On Mon, Mar 30, 2009 at 10:54:58PM +0300, Avi Kivity wrote:
: > Blue Swirl wrote:
: >>>  +4. Block structure
: >>>  +
: >>>  +Every indented statement is braced; even if the block contains just one
: >>>  +statement.
: >>>     
: >>
: >> I'd remove this, braces are not used consistently for one statement blocks.
: >>
: >>   
: >
: > While that's true, I'd like to keep this.  I found (after initially  
: > being annoyed by this; I dislike punctuation) that it's nice not to need  
: > to rebrace after adding or removing lines.
: 
: I hate having to add braces to add a printf when debuging something.
: If you forget the braces, you change the meaning of the code.  To me
: that's a disaster and enough reason that leaving out braces should
: be banned.  Code style that encourages creation of bugs is bad style.
: Surprisingly many code styles don't get this however.

With editors like emacs, this isn't an issue.

: I also like consistency, and since you need braces for multiline
: statements, why not always use them?

Because it stretches the code vertically.  More extra useless 'blank'
lines makes it harder to get more code on the screen, which makes the
code harder to understand.

Anyway, this is a highly religious issue.  Either you think that {}
are the bee's knees and people are morons that don't use them, or you
hate them with a huge passion and can't believe people are stupid
enough to require it.  There's a very small set of folks in between,
and often little common ground: usually one camp tolerates the
practices of the other...

Warner

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[M. Warner Losh]

In message: <20090330214547.GQ3795@csclub.uwaterloo.ca>
            lsorense@csclub.uwaterloo.ca (Lennart Sorensen) writes:
: On Mon, Mar 30, 2009 at 09:28:54PM +0300, Blue Swirl wrote:
: > I'd remove this, braces are not used consistently for one statement blocks.
: 
: Then fix that.  Making the coding style worse just because some people
: have failed to follow it is not the right solution.
: 
: > No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
: > 
: > Heretic people all over the world have claimed that this inconsistency
: > is ...  well ...  inconsistent, but all right-thinking people know that
: > (a) K&R are _right_ and (b) K&R are right.  Besides, functions are
: > special anyway (you can't nest them in C).
: 
: And Linus is wrong for not requiring braces at all times in the kernel. :)
: I don't have much hope of convincing him of that, but he is still wrong.

And this is an example of the religious nature of this argument :)

Warner

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Lennart Sorensen]

On Mon, Mar 30, 2009 at 04:15:14PM -0600, M. Warner Losh wrote:
> With editors like emacs, this isn't an issue.

Who gives a @#$ what emacs does.

> Because it stretches the code vertically.  More extra useless 'blank'
> lines makes it harder to get more code on the screen, which makes the
> code harder to understand.

So what?  Compared to the bugs this often causes, go buy a bigger screen.

> Anyway, this is a highly religious issue.  Either you think that {}
> are the bee's knees and people are morons that don't use them, or you
> hate them with a huge passion and can't believe people are stupid
> enough to require it.  There's a very small set of folks in between,
> and often little common ground: usually one camp tolerates the
> practices of the other...

I just hate the mistakes the lack of the braces cause, and they do
cause mistakes.  It is a huge mistake that C even allowed them to be
optional in the first place.  A bit late to fix that now.

-- 
Len Sorensen

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[M. Warner Losh]

In message: <20090330233853.GT3795@csclub.uwaterloo.ca>
            lsorense@csclub.uwaterloo.ca (Lennart Sorensen) writes:
: On Mon, Mar 30, 2009 at 04:15:14PM -0600, M. Warner Losh wrote:
: > With editors like emacs, this isn't an issue.
: 
: Who gives a @#$ what emacs does.
: 
: > Because it stretches the code vertically.  More extra useless 'blank'
: > lines makes it harder to get more code on the screen, which makes the
: > code harder to understand.
: 
: So what?  Compared to the bugs this often causes, go buy a bigger screen.

When used with emacs, the number of bugs introduced is about nil,
while the bugs introduced through lack of understanding of the code is
non-nil.  At least that's been my experience over the past 20 years of
doing this.  Your mileage may vary.  Objects in mirror are closer than
they appear.

: > Anyway, this is a highly religious issue.  Either you think that {}
: > are the bee's knees and people are morons that don't use them, or you
: > hate them with a huge passion and can't believe people are stupid
: > enough to require it.  There's a very small set of folks in between,
: > and often little common ground: usually one camp tolerates the
: > practices of the other...
: 
: I just hate the mistakes the lack of the braces cause, and they do
: cause mistakes.  It is a huge mistake that C even allowed them to be
: optional in the first place.  A bit late to fix that now.

Again, this is clearly a religious argument.  Nobody is going to
settle it here.

Warner

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Gleb Natapov]

On Mon, Mar 30, 2009 at 05:45:47PM -0400, Lennart Sorensen wrote:
> On Mon, Mar 30, 2009 at 09:28:54PM +0300, Blue Swirl wrote:
> > I'd remove this, braces are not used consistently for one statement blocks.
> 
> Then fix that.  Making the coding style worse just because some people
> have failed to follow it is not the right solution.
> 
> > No, this is the K&R style. Quoting linux/Documentation/CodingStyle:
> > 
> > Heretic people all over the world have claimed that this inconsistency
> > is ...  well ...  inconsistent, but all right-thinking people know that
> > (a) K&R are _right_ and (b) K&R are right.  Besides, functions are
> > special anyway (you can't nest them in C).
> 
> And Linus is wrong for not requiring braces at all times in the kernel. :)
> I don't have much hope of convincing him of that, but he is still wrong.
> 
Try it anyway. Do it on LKML. It will be entertaining.

--
			Gleb.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Laurent Desnogues]

On Tue, Mar 31, 2009 at 12:38 AM, Lennart Sorensen
<lsorense@csclub.uwaterloo.ca> wrote:
> On Mon, Mar 30, 2009 at 04:15:14PM -0600, M. Warner Losh wrote:
>> With editors like emacs, this isn't an issue.
>
> Who gives a @#$ what emacs does.

Ha at last!  vi was put in the original document, then someone mentions
emacs and then some insults.  This finally becomes interesting.


Laurent

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Very frankly, I don't think that a coding style, even strictly applied, is
going to make the QEMU code
easier to understand.

The real barriers to understanding are the lack of structure in the code,
liberal use of global macros
scattered randomly in the source code, exceedingly liberally named
functions, and sometimes obscure
implementation of simple concepts (*cough* CharDriverState), cramming
totally unrelated stuff in single
largish source files (vl.c for the win !), and a blatant lack of
documentation comments for a lot of subtle
stuff in there to explain the magic.

Braces and indentation are sometimes annoying, but frankly these are such
minor issues I wonder
why people waste their time venting about them given the source code's
state.

On Tue, Mar 31, 2009 at 7:59 AM, Laurent Desnogues <
laurent.desnogues@gmail.com> wrote:

> On Tue, Mar 31, 2009 at 12:38 AM, Lennart Sorensen
> <lsorense@csclub.uwaterloo.ca> wrote:
> > On Mon, Mar 30, 2009 at 04:15:14PM -0600, M. Warner Losh wrote:
> >> With editors like emacs, this isn't an issue.
> >
> > Who gives a @#$ what emacs does.
>
> Ha at last!  vi was put in the original document, then someone mentions
> emacs and then some insults.  This finally becomes interesting.
>
>
> Laurent
>
>
>

[-- Attachment #2: Type: text/html, Size: 1740 bytes --]

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

David Turner wrote:
> Very frankly, I don't think that a coding style, even strictly 
> applied, is going to make the QEMU code
> easier to understand.
>

The coding style is not intended to make the code clearer, just to make 
it more uniform.

> The real barriers to understanding are the lack of structure in the 
> code, liberal use of global macros
> scattered randomly in the source code, exceedingly liberally named 
> functions, and sometimes obscure
> implementation of simple concepts (*cough* CharDriverState), cramming 
> totally unrelated stuff in single
> largish source files (vl.c for the win !), and a blatant lack of 
> documentation comments for a lot of subtle
> stuff in there to explain the magic.

This is the QEMU coding style.

Seriously, what you say is largely true, but the way to fix it is to 
sent patches or to review posted patches.  Nothing else will make a 
difference.



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Paul Brook]

> +2. Line width
> +
> +Lines are 80 characters wide plus some slop.  Try to fit your code into
> +eighty characters, but if it makes a snippet particularly ugly, allow
> +yourself some slack.  Don't overdo it though.

I'd remove the "plus some slop". My guess is that most people using a 
non-graphical editor (and probably many of those using graphical variants of 
vi/emacs) still wrap at 80 characters. If you can't find a convenient place 
to break the line, then maybe you're doing something else wrong (e.g. using 
40-character variable names :-)

Other than that, looks ok.

Paul

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Blue Swirl]

On 3/31/09, David Turner <digit@google.com> wrote:
> Very frankly, I don't think that a coding style, even strictly applied, is
> going to make the QEMU code
> easier to understand.
>
> The real barriers to understanding are the lack of structure in the code,
> liberal use of global macros
>  scattered randomly in the source code, exceedingly liberally named
> functions, and sometimes obscure
> implementation of simple concepts (*cough* CharDriverState), cramming
> totally unrelated stuff in single
> largish source files (vl.c for the win !), and a blatant lack of
> documentation comments for a lot of subtle
>  stuff in there to explain the magic.

True enough for the comments part. There are still some areas that I
don't understand well, for example TB handling and its inherent
limitations.

Which part of the source you find subtle and magical but not commented enough?

Maybe we should use Doxygen or hxtool to combine source code and documentation.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

On Tue, Mar 31, 2009 at 3:31 PM, Avi Kivity <avi@redhat.com> wrote:

> The coding style is not intended to make the code clearer, just to make it
> more uniform.
>

I'm just saying that uniformity is overrated given the state of the sources
:-)

[-- Attachment #2: Type: text/html, Size: 543 bytes --]

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[David Turner]

[-- Attachment #1.1: Type: text/plain, Size: 2914 bytes --]

On Tue, Mar 31, 2009 at 6:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:

>
> True enough for the comments part. There are still some areas that I
> don't understand well, for example TB handling and its inherent
> limitations.
>
> Which part of the source you find subtle and magical but not commented
> enough?
>

ok, here are a few things that ring a bell, there are probably a lot others:

the software mmu implementation in system mode is *really* hard to
understand at
first. It took me a long time to grasp the various aspects of it, including
these:

   - loads/stores in kernel or userspace map to different translated code
   fragments
   - loads/stores in different emulated CPUs also map to different
   translated code.
   - the way i/o memory access is controlled in fine details and relates to
   the rest of the MMU.

most of what happens in exe.c is black-magic :-)

how the audio-subsystem works and its relationship with hardware emulation
is subtle.
For my work on the Android emulator, I have modified three audio-backends
and wrote
one from scratch. It took me several tries to get things to an acceptable
state. What I
didn't understand first is that there is no common time-based used by all
components
involved.

dyngen used to be pretty radical too. Thanks god it is gone for any target
and host platform
combination I care about :-)

slirp is an hideous pile of goo which mixes network-ordered fields and
host-ordered ones,
including pointers, into the same structures, liberally and at different
times, depending on
context. Which is probably why it took so long to fix 64-bit issues in it. I
would like to add
support for IPv6 to this code, but sadly, I have the feeling that rewriting
most of it from
scratch would be slightly easier.

the CharDriverState interface takes some time to fully understand. In the
Android emulator,
I sometimes need to connect two CharDriverState users together, and had to
write a special
purpose object just to do that, but was surprised how hard writing a
bug-less one was.

I also think that the event loop implementation is confusing compared to
more common interfaces
provides by things like libevent. It is also extremely tied to select(),
which prevents using better
mechanisms on various platforms, and even performs rather poorly on Windows,
but I digress.

qemu_get_clock(vm_clock) will return time in nano-seconds, but
qemu_get_clock(rt_clock)
will return time in milli-seconds. This is totally undocumented, and the
code that uses the result
tend to use magic numbers like 1000000 or 1000 to perform conversions which
are never clear
on first sight. Maybe this is fixed in upstream QEMU but, my, how this
hurted me in the past.

For the record, here are attached a few documents I wrote to detail what
I've "discovered"
until now. Hope some one can find them useful (Sorry if some of them are
focused on ARM
system emulation only).

Regards

[-- Attachment #1.2: Type: text/html, Size: 3409 bytes --]

[-- Attachment #2: AUDIO.TXT --]
[-- Type: text/plain, Size: 8296 bytes --]

HOW AUDIO EMULATION WORKS IN QEMU:
==================================

Things are a bit tricky, but here's a rough description:

  QEMUSoundCard: models a given emulated sound card
  SWVoiceOut:    models an audio output from a QEMUSoundCard
  SWVoiceIn:     models an audio input from a QEMUSoundCard

  HWVoiceOut:    models an audio output (backend) on the host.
  HWVoiceIn:     models an audio input (backend) on the host.

Each voice can have its own settings in terms of sample size, endianess, rate, etc...


Emulation for a given soundcard typically does:

  1/ Create a QEMUSoundCard object and register it with AUD_register_card()
  2/ For each emulated output, call AUD_open_out() to create a SWVoiceOut object.
  3/ For each emulated input, call AUD_open_in() to create a SWVoiceIn object.

  Note that you must pass a callback function to AUD_open_out() and AUD_open_in();
  more on this later.

  Each SWVoiceOut is associated to a single HWVoiceOut, each SWVoiceIn is
  associated to a single HWVoiceIn.

  However you can have several SWVoiceOut associated to the same HWVoiceOut
  (same thing for SWVoiceIn/HWVoiceIn).

SOUND PLAYBACK DETAILS:
=======================

Each HWVoiceOut has the following too:

  - A fixed-size circular buffer of stereo samples (for stereo).
    whose format is either floats or int64_t per sample (depending on build
    configuration).

  - A 'samples' field giving the (constant) number of sample pairs in the stereo buffer.

  - A target conversion function, called 'clip()' that is used to read from the stereo
    buffer and write into a platform-specific sound buffers (e.g. WinWave-managed buffers
    on Windows).

  - A 'rpos' offset into the circular buffer which tells where to read the next samples
    from the stereo buffer for the next conversion through 'clip'.


            |<----------------- samples ----------------------->|

            |                                                   |

            |       rpos                                        |
                    |
            |_______v___________________________________________|
            |       |                                           |
            |       |                                           |
            |_______|___________________________________________|


  - A 'run_out' method that is called each time to tell the output backend to
    send samples from the stereo buffer to the host sound card/server. This method
    shall also modify 'rpos' and returns the number of samples 'played'. A more detailed
    description of this process appears below.

  - A 'write' method callback used to write a buffer of emulated sound samples from
    a SWVoiceOut into the stereo buffer. *All* backends simply call the generic
    function audio_pcm_sw_write() to implement this. It's difficult to see why
    it's needed at all ?

    (Similarly, all backends have a 'read' methods which simply calls 'audio_pcm_sw_read')

Each SWVoiceOut has the following:

  - a 'conv()' function used to read sound samples from the emulated sound card and
    copy/mix them to the corresponding HWVoiceOut's stereo buffer.

  - a 'total_hw_samples_mixed' which correspond to the number of samples that have
    already been mixed into the target HWVoiceOut stereo buffer (starting from the
    HWVoiceOut's 'rpos' offset). NOTE: this is a count of samples in the HWVoiceOut
    stereo buffer, not emulated hardware sound samples, which can have different
    properties (frequency, size, endianess).
                                         ______________
                                        |              |
                                        |  SWVoiceOut2 |
                                        |______________|
                  ______________           |
                 |              |          |
                 |  SWVoiceOut1 |          |     thsm<N> := total_hw_samples_mixed
                 |______________|          |                for SWVoiceOut<N>
                           |               |
                           |               |
                    |<-----|------------thsm2-->|
                    |      |                    |
                    |<---thsm1-------->|        |
             _______|__________________v________|_______________ 
            |       |111111111111111111|        v               |
            |       |222222222222222222222222222|               |
            |_______|___________________________________________|
                    ^
                    |         HWVoiceOut stereo buffer
                    rpos


  - a 'ratio' value, which is the ratio of the target HWVoiceOut's frequency by
    the SWVoiceOut's frequency, multiplied by (1 << 32), as a 64-bit integer.

    So, if the HWVoiceOut has a frequency of 44kHz, and the SWVoiceOut has a frequency
    of 11kHz, then ratio will be (44/11*(1 << 32)) = 0x4_0000_0000

  - a callback provided by the emulated hardware when the SWVoiceOut is created.
    This function is used to mix the SWVoiceOut's samples into the target
    HWVoiceOut stereo buffer (it must also perform frequency interpolation,
    volume adjustment, etc..).

    This callback normally calls another helper functions in the audio subsystem
    (AUD_write()) to to the mixing/volume-adjustment from emulated hardware sample
    buffers.

Here's a small graphics that explains it better:

   SWVoiceOut:  emulated hardware sound buffers:

          |
          |   (mixed through AUD_write() from user-provided callback
          |    which is called on each audio timer tick).
          v

   HWVoiceOut: stereo sample circular buffer

          |
          |   (through HWVoiceOut's 'clip' function, invoked from the
          |    'run_out' method)
          v

   backend-specific sound buffers

THERE IS NO COMMON TIMEBASE BETWEEN ALL LAYERS. DON'T EXPECT ANY HIGH-ACCURACY /
LOW-LATENCY IN THIS IMPLEMENTATION.


The function audio_timer() in audio/audio.c is called periodically and it is used as
a pulse to perform sound buffer transfers and mixing. More specifically for audio
output voices:

- For each HWVoiceOut, find the number of active SWVoiceOut, and the minimum number
  of 'total_hw_samples_mixed' that have already been written to the buffer. We will
  call this value the number of 'live' samples in the stereo buffer.

- if 'live' is 0, call the callback of each active SWVoiceOut to fill the stereo
  buffer, if needed, then exit.

- otherwise, call the 'run_out' method of the HWVoiceOut object. This will change
  the value of 'rpos' and return the number of samples played. Then the
  'total_hw_samples_mixed' field of all active SWVoiceOuts is decremented by
  'played', and the callback is called to re-fill the stereo buffer.

It's important to note that the SWVoiceOut callback:

- takes a 'free' parameter which is the number of stereo sound samples that can
  be sent to the hardware stereo buffer (before rate adjustment, i.e. not the number
  of sound samples in the SWVoiceOut emulated hardware sound buffer).

- must call AUD_write(sw, buff, count), where 'buff' points to emulated sound
  samples, and their 'count', which must be <= the 'free' parameter.

- the implementation of AUD_write() will call the 'write' method of the target
  HWVoiceOut, which in turns calls the function audio_pcm_sw_write() which does
  standard rate/volume adjustment before mixing the conversion into the target
  stereo buffer. It also increases the 'total_hw_samples_mixed' value of the
  SWVoiceOut.

- audio_pcm_sw_write() returns the number of sound sample *bytes* that have
  been mixed into the stereo buffer, and so does AUD_write().

So, in the end, we have the pseudo-code:

    every sound timer ticks:
      for hw in list_HWVoiceOut:
         live = MIN([sw.total_hw_samples_mixed for sw in hw.list_SWVoiceOut ])
         if live > 0:
            played = hw.run_out(live)
            for sw in hw.list_SWVoiceOut:
                sw.total_hw_samples_mixed -= played

        for sw in hw.list_SWVoiceOut:
            free = hw.samples - sw.total_hw_samples_mixed
            if free > 0:
                sw.callback(sw, free)

SOUND RECORDING DETAILS:
========================

Things are similar but in reverse order.

[-- Attachment #3: CHAR-DEVICES.TXT --]
[-- Type: text/plain, Size: 8264 bytes --]

QEMU CHARACTER "DEVICES" MANAGEMENT

I. CharDriverState objects:
---------------------------

One of the strangest abstraction in QEMU is the "CharDriverState"
(abbreviated here as "CS").

The CS is essentially an object used to model a character stream that
can be connected to things like a host serial port, a host network socket,
an emulated device, etc...

What's really unusual is its interface though, which comes from the fact
that QEMU implements a big event loop with no blocking i/o allowed. You
can see "qemu-char.h" for the full interface, but here we will only describe
a few important functions:

  - qemu_chr_write() is used to try to write data into a CS object. Note that
    success is not guaranteed: the function returns the number of bytes that
    were really written (which can be 0) and the caller must deal with it.
    This is very similar to writing to a non-blocking BSD socket on Unix.

       int  qemu_chr_read( CharDriverState*  cs,
                           const uint8_t*    data,
                           int               datalen );

    This function may return -1 in case of error, but this depends entirely
    on the underlying implementation (some of them will just return 0 instead).
    In practice, this means it's not possible to reliably differentiate between
    a "connection reset by peer" and an "operation in progress" :-(

    There is no way to know in advance how many bytes a given CharDriverState
    can accept, nor to be notified when its underlying implementation is ready
    to accept data again.


  - qemu_chr_add_handler() is used to add "read" and "event" handlers
    to a CS object. We will ignore "events" here and focus on the
    "read" part.

    Thing is, you cannot directly read from a CS object. Instead, you provide
    two functions that will be called whenever the object has something for
    you:

        - a 'can_read' function that shall return the number of bytes
          that you are ready to accept from the CharDriverState. It's
          interface is:

             typedef int  IOCanRWHandler (void*  opaque);

        - a 'read' function that will send you bytes from the CharDriverState

             typedef void IOReadHandler  (void*           opaque,
                                          const uint8_t*  data,
                                          int             datalen);

          normally, the value of 'datalen' cannot be larger than the result
          of a previous 'can_read' call.

    For both callbacks, 'opaque' is a value that you pass to the function
    qemu_chr_add_handler() which signature is:

         void qemu_chr_add_handlers(CharDriverState *s,
                                    IOCanRWHandler  *fd_can_read,
                                    IOReadHandler   *fd_read,
                                    IOEventHandler  *fd_event,
                                    void            *opaque);

  - qemu_chr_open() is used to create a new CharDriverState object from a
    descriptive string, it's interface is:

         CharDriverState*  qemu_chr_open(const char*  filename);

    there are various formats for acceptable 'filenames', and they correspond
    to the parameters of the '-serial' QEMU option described here:

       http://www.nongnu.org/qemu/qemu-doc.html#SEC10

    For example:

       "/dev/<file>" (Linux and OS X only):
            connect to a host character device file (e.g. /dev/ttyS0)

       "file:<filename>":
            Write output to a given file (write only)

       "stdio":
            Standard input/output

       "udp:[<remote_host>]:<remote_port>[@[<src_ip>]:<src_port>]":
            Connect to a UDP socket for both read/write.

       "tcp:[<host>]:<port>[,server][,nowait][,nodelay]"
            Connect to a TCP socket either as a client or a server.

            The 'nowait' option is used to avoid waiting for a client
            connection.

            The 'nodelay' is used to disable the TCP Nagle algorithm to
            improve throughput.

    for Android, a few special names have been added to the internal
    implementation and redirect to program functions:

       "android-kmsg":
            A CharDriverState that is used to receive kernel log messages
            from the emulated /dev/ttyS0 serial port.

       "android-qemud":
            A CharDriverState that is used to exchange messages between the
            emulator program and the "qemud" multiplexing daemon that runs in
            the emulated system.

            The "qemud" daemon is used to allow one or more clients in the
            system to connect to various services running in the emulator
            program. This is mainly used to bypass the kernel in order to
            implement certain features with ease.

       "android-gsm":
            A CharDriverState that is used to connect the emulated system to
            a host modem device with the -radio <device> option. Otherwise,
            the system uses qemud to connect to the emulator's internal modem
            emulation.

        "android-gps":
            A CharDriverState that is used to connect the emulated system to a
            host GPS device with the -gps <device> option. Otherwise the
            system uses qemud to connect to the emulator's internal GPS
            emulation.


II. CharDriverState users:
--------------------------

As described above, a CharDriverState "user" is a piece of code that can write
to a CharDriverState (by calling qemu_chr_write() explicitely) and can also
read from it after registering can_read/read handlers for it through
qemu_chr_add_handlers().

Typical examples are the following:

  - The hardware serial port emulation (e.g. hw/goldfish_tty.c) will read data
    from the kernel then send it to a CS. It also uses a small buffer that is
    used to read data from the CS and send it back to the kernel.

  - The Android emulated modem also uses a CS to talk with its client,
    which will in most cases an emulated serial port.


III. CharBuffer objects:
------------------------

The Android emulator provides an object called a CharBuffer which acts as
a CharDriverState object that implements a *write* buffer to send data to a
given CS object, called the endpoint. You can create one with:

    #include "charpipe.h"
    CharDriverState*  qemu_chr_open_buffer( CharDriverState*  endpoint );

This function returns a new CS object that will buffer in the heap any data
that is sent to it, but cannot be sent to the endpoint yet. On each event loop
iteration, the CharBuffer will try to send data to the endpoint untill it
doesn't have any data left.

This can be useful to simplify certain CS users who don't want to maintain
their own emit buffer. Note that writing to a CharBuffer always succeeds.

Note also that calling qemu_chr_add_handler() on the CharBuffer will do the
same on the endpoint. Any endpoint-initiated calls to can_read()/read()
callbacks are passed directly to your handler functions.


IV. CharPipe objects:
---------------------

The Android emulator also provides a convenient abstraction called a "charpipe"
used to connect two CharDriverState users together. For example, this is used
to connect a serial port emulation (in hw/goldfish_tty.c) to the internal
GSM modem emulation (see telephony/modem_driver.c).

Essentially, a "charpipe" is a bi-directionnal communication pipe whose two
endpoints are both CS objects. You call "qemu_chr_open_pipe()" to create the
pipe, and this function will return the two endpoints to you:

    #include "charpipe.h"
    int  qemu_chr_open_pipe(CharDriverState* *pfirst,
                            CharDriverState* *psecond);

When you write to one end of the pipe (with qemu_chr_write()), the charpipe
will try to write as much data as possible to the other end. Any remaining data
is stored in a heap-allocated buffer.

The charpipe will try to re-send the buffered data on the next event loop
iteration by calling the can_read/read functions of the corresponding user,
if there is one.

Note that there is no limit on the amount of data buffered in a charpipe,
and writing to it is never blocking. This simplifies CharDriverState
users who don't need to worry about buffering issues.

[-- Attachment #4: CPU-EMULATION.TXT --]
[-- Type: text/plain, Size: 3949 bytes --]

HOW THE QEMU EXECUTION ENGINE WORKS:
====================================

Translating ARM to x86 machine code:
------------------------------------

QEMU starts by isolating code "fragments" from the emulated machine code.
Each "fragment" corresponds to a series of ARM instructions ending with a
branch (e.g. jumps, conditional branches, returns).

Each fragment is translated into a "translated block" (a.k.a. TB) of host
machine code (e.g. x86). All TBs are put in a cache and each time the
instruction pointer changes (i.e. at the end of TB execution), a hash
table lookup is performed to find the next TB to execute.

If none exists, a new one is generated. As a special exception, it is
sometimes possible to 'link' the end of a given TB to the start of
another one by tacking an explicit jump instruction.

Note that due to differences in translations of memory-related operations
(described below in "MMU emulation"), there are actually two TB caches per
emulated CPU: one for translated kernel code, and one for translated
user-space code.

When a cache fills up, it is simply totally emptied and translation starts
again.

CPU state is kept in a single global structure which the generated code
can access directly (with direct memory addressing).

The file target-arm/translate.c is in charge of translating the ARM or
Thumb instructions starting at the current instruction pointer position
into a TB. This is done by decomposing each instruction into a series of
micro-operations supported by the TCG code generator.

TCG stands for "Tiny Code Generator" and is specific to QEMU. It supports
several host machine code backends. See source files under tcg/ for details.


MMU Emulation:
--------------

The ARM Memory Management Unit is emulated in software, since it is so
different from the one on the host. Essentially, a single ARM memory load/store
instruction is translated into a series of host machine instructions that will
translate virtual addresses into physical ones by performing the following:

- first lookup in a global 256-entries cache for the current page and see if
  a corresponding value is already stored there. If this is the case, use it
  directly.

- otherwise, call a special helper function that will implement the full
  translation according to the emulated system's state, and modify the
  cache accordingly.

The page cache is called the "TLB" in the QEMU sources.

Note that there are actually two TLBs: one is used for host machine
instructions that correspond to kernel code, and the other for instructions
translated from user-level code.

This means that a memory load in the kernel will not be translated into the
same instructions than the same load in user space.

Each TLB is also implemented as a global per-emulated-CPU hash-table.
The user-level TLB is flushed on each process context switch.

When initializing the MMU emulation, one can define several zones of the
address space, with different access rights / type. This is how memory-mapped
I/O is implemented: the virtual->physical conversion helper function detects
that you're trying to read/write from an I/O memory region, and will then call
a callback function associated to it.


Hardware Emulation:
-------------------

Most hardware emulation code initializes by registering its own region of
I/O memory, as well as providing read/write callbacks for it. Then actions
will be based on which offset of the I/O memory is read from/written to and
eventually with which value.

You can have a look at hw/goldfish_tty.c that implements an emulated serial
port for the Goldfish platform.

"Goldfish" is simply the name of the virtual Linux platform used to build
the Android-emulator-specific kernel image. The corresponding sources are
located in the origin/android-goldfish-2.6.27 branch of
git://android.git.kernel.org/kernel/common.git. You can have a look at
arch/arm/mach-goldfish/ for the corresponding kernel driver sources.

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[malc]

On Tue, 31 Mar 2009, David Turner wrote:

> On Tue, Mar 31, 2009 at 6:18 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> 
> >
> > True enough for the comments part. There are still some areas that I
> > don't understand well, for example TB handling and its inherent
> > limitations.
> >
> > Which part of the source you find subtle and magical but not commented
> > enough?
> >
> 
> ok, here are a few things that ring a bell, there are probably a lot others:
> 
> the software mmu implementation in system mode is *really* hard to
> understand at
> first. It took me a long time to grasp the various aspects of it, including
> these:
> 
>    - loads/stores in kernel or userspace map to different translated code
>    fragments
>    - loads/stores in different emulated CPUs also map to different
>    translated code.
>    - the way i/o memory access is controlled in fine details and relates to
>    the rest of the MMU.
> 
> most of what happens in exe.c is black-magic :-)
> 
> how the audio-subsystem works and its relationship with hardware
> emulation is subtle.  For my work on the Android emulator, I have
> modified three audio-backends and wrote one from scratch. It took me
> several tries to get things to an acceptable state. What I didn't
> understand first is that there is no common time-based used by all
> components involved.
> 

For starters you could have asked about things you believe are subtle.

Now to the part i do not understand: what do you mean by time-based(sic)?

There's one clock involved, as far as audio is concerned, and it's the
one dervied from the speed with which host audio can consume/produce
audio, anything else just doesn't work (for scenarios i was interested
in anyway)

As for the question of why everything just calls audio_pcm_sw_write
raised in AUDIO.txt, the reason, if my memory serves and it might as
well not do that all that well, things are the way they are for the
reason that any given driver can, theoretically, use the respective
audio subsystems/libraries own, less naive, st_rate_flow equivalent.

[..snip..]

> For the record, here are attached a few documents I wrote to detail
> what I've "discovered" until now. Hope some one can find them useful
> (Sorry if some of them are focused on ARM system emulation only).
> 
> Regards
> 

P.S. Last/only time i looked at Android couldn't help but notice that,
     apart from other things, capture was implemented for coreaudio,
     it would have been nice, if for nothing else but the sake of
     completeness, to have that in main QEMU too.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

On Wed, Apr 1, 2009 at 12:38 AM, malc <av1474@comtv.ru> wrote:

>
> For starters you could have asked about things you believe are subtle.
>

True enough, but for a long time the project was confidential and alluding
to it publicly
was not recommended. I'll try to break the habit.


>
> Now to the part i do not understand: what do you mean by time-based(sic)?


> There's one clock involved, as far as audio is concerned, and it's the
> one dervied from the speed with which host audio can consume/produce
> audio, anything else just doesn't work (for scenarios i was interested
> in anyway)
>

I had some problems when running qemu on low-powered computers. If I
remember
correctly, the emulation part was taking so much of the CPU that audio could
very
well stutter in strange ways on some platforms. One of the reason for it was
that the
host audio output (e.g. esd) stopped consuming samples at a consistent rate,
which
forced SWVoiceOut buffers to fill up and delay audio production in the
emulated
system even more.

In certain cases, there is also a non-trivial latency between the moment the
emulated
system produces audio-samples (e.g. sends them through dma to the emulated
hardware), and the moment it is effectively sent to the host backend. This
is mostly
related to playing with the audio timer tick configuration which by default
is so small
that it should not matter.

What I mean by time-based means a way to deal with varying latencies in both
audio
production and consumption. I agree it's a hard problem and is not totally
required for
QEMU. It's just that I spent some time trying to understand how the audio
subsystem
dealt with the problem, except that it didn't.


>
> As for the question of why everything just calls audio_pcm_sw_write
> raised in AUDIO.txt, the reason, if my memory serves and it might as
> well not do that all that well, things are the way they are for the
> reason that any given driver can, theoretically, use the respective
> audio subsystems/libraries own, less naive, st_rate_flow equivalent.
>

Interesting, thanks for the explanation, this makes it clear.


> P.S. Last/only time i looked at Android couldn't help but notice that,
>     apart from other things, capture was implemented for coreaudio,
>     it would have been nice, if for nothing else but the sake of
>     completeness, to have that in main QEMU too.
>

Actually, the details of audio-related changes performed are:

- adding audio input to the CoreAudio backend

- adding dynamic linking support to the esd and alsa backends
(using dlopen/dlsym allows the emulator to run on platforms where
all the corresponding libraries / sound server are not available).

- rewriting the sdl backend run_out() method. For some reason the old one
tended
to lock up QEMU in certain weird cases I could never completely understand.

- modifying the sub-system to be able to use different backends for audio
input
  and output.

- adding a new "winaudio.c" backend for Windows that uses Wave functions
instead
  of DirectX (mainly to be able to build on old versions of mingw that
didn't provide
  directx-compatible headers, I'm not sure if it's still needed these days,
but at least
  the code is 10x simpler than the dxaudio.c one, talk about a complex sound
API).

I plan to provide patches for all of these for upstream. Sorry if this
hasn't been done yet.


> --
> mailto:av1474@comtv.ru
>
>
>

[-- Attachment #2: Type: text/html, Size: 4838 bytes --]

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[malc]

On Wed, 1 Apr 2009, David Turner wrote:

> On Wed, Apr 1, 2009 at 12:38 AM, malc <av1474@comtv.ru> wrote:
>
> >
> > For starters you could have asked about things you believe are subtle.
> >
>
> True enough, but for a long time the project was confidential and alluding
> to it publicly
> was not recommended. I'll try to break the habit.
>
>
> >
> > Now to the part i do not understand: what do you mean by time-based(sic)?
>
>
> > There's one clock involved, as far as audio is concerned, and it's the
> > one dervied from the speed with which host audio can consume/produce
> > audio, anything else just doesn't work (for scenarios i was interested
> > in anyway)
> >
>
> I had some problems when running qemu on low-powered computers. If I
> remember correctly, the emulation part was taking so much of the CPU
> that audio could very well stutter in strange ways on some
> platforms. One of the reason for it was that the host audio output
> (e.g. esd) stopped consuming samples at a consistent rate, which
> forced SWVoiceOut buffers to fill up and delay audio production in
> the emulated system even more.

Hmm.. please define low-powered.. Audio started being developed on a
1Ghz Athlon, granted the machine was equipped with Aureal Vortex 2 and
i had a luxory of using [ADA]C_FIXED_SETTINGS set to zero and
DAC_VOICES set to something reasonable thus basically avoiding
software mixing altogether, that said some of the old DOS games/demos
use Soundblaster/OPL[23] in such creative ways so doing things in
software even with au8830 was sometimes inevitable...

And FWIW nowadays it's MPC7447A at 1.3Ghz which is not speed demon
either. Not that i dont belive you, esp considering esd thrown into
equation, but it would be interesting to know what people deem
low-powered currently.

> In certain cases, there is also a non-trivial latency between the
> moment the emulated system produces audio-samples (e.g. sends them
> through dma to the emulated hardware), and the moment it is
> effectively sent to the host backend. This is mostly related to
> playing with the audio timer tick configuration which by default is
> so small that it should not matter.
>
> What I mean by time-based means a way to deal with varying latencies
> in both audio production and consumption. I agree it's a hard
> problem and is not totally required for QEMU. It's just that I spent
> some time trying to understand how the audio subsystem dealt with
> the problem, except that it didn't.

Well.. I certainly fail to see how adding some other clock would
overcome the fact that something can't keep up.. (rt priorties and
suchlike?)

>
> >
> > As for the question of why everything just calls audio_pcm_sw_write
> > raised in AUDIO.txt, the reason, if my memory serves and it might as
> > well not do that all that well, things are the way they are for the
> > reason that any given driver can, theoretically, use the respective
> > audio subsystems/libraries own, less naive, st_rate_flow equivalent.
> >
>
> Interesting, thanks for the explanation, this makes it clear.
>
>
> > P.S. Last/only time i looked at Android couldn't help but notice that,
> >     apart from other things, capture was implemented for coreaudio,
> >     it would have been nice, if for nothing else but the sake of
> >     completeness, to have that in main QEMU too.
> >
>
> Actually, the details of audio-related changes performed are:
>
> - adding audio input to the CoreAudio backend
>
> - adding dynamic linking support to the esd and alsa backends
> (using dlopen/dlsym allows the emulator to run on platforms where
> all the corresponding libraries / sound server are not available).

I don't think this is worth it for QEMU, after all shipping binaries
is not what it's best known for.

> - rewriting the sdl backend run_out() method. For some reason the old one
> tended to lock up QEMU in certain weird cases I could never
> completely understand.

Hmm..

> - modifying the sub-system to be able to use different backends for
>   audio input and output.

Yeah i recall seeing this, and was wondering why Android needed this
functionality.

> - adding a new "winaudio.c" backend for Windows that uses Wave
>   functions instead of DirectX (mainly to be able to build on old
>   versions of mingw that didn't provide directx-compatible headers,
>   I'm not sure if it's still needed these days, but at least the
>   code is 10x simpler than the dxaudio.c one, talk about a complex
>   sound API).
>

Complexity notwithstanding i happen to like the way things are done in
DSound, the conceptually simple approach that is.

> I plan to provide patches for all of these for upstream. Sorry if this
> hasn't been done yet.
>

Nice to hear that.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

On Wed, Apr 1, 2009 at 1:49 AM, malc <av1474@comtv.ru> wrote:

> Hmm.. please define low-powered..


No problem, people have been running the Android SDK on 700 MHz.
I used to run it on a 900Hz Celeron or something like that.

Today, I'm doing all my "minimal performance" tests on a venerable
1 GHz Pentium III laptop with an integrated Intel audio chipset running
Windows XP (and a Linux install under VMWare).

Just to be clear, I'm not suggesting to anyone to fix anything here..


>
> And FWIW nowadays it's MPC7447A at 1.3Ghz which is not speed demon
> either. Not that i dont belive you, esp considering esd thrown into
> equation, but it would be interesting to know what people deem
> low-powered currently.
>

Regarding esd, I unfortunately have to support the emulator binary running
on
Ubuntu 6.06 installations where the sound server will frequently lock up
when the
Android emulator is run. In some cases, this also totally freezes the whole
desktop,
though it is possible to recover by perfoming a "killall -9 esd" from a
console.

I initially thought that the main reason for that was that the esd client
library could
not handle the multiple SIGALRM-induced EINTR returned by write() and other
system calls, and left the sound server in a sorry state (maybe because it
was stuck
waiting for some data that would never arrive). However, I protected all
calls to the esd backend (playing with the signal mask to avoid that), but
this
still doesn't get rid of the problem. Fortunately, this doesn't seem to
happen with
later versions of Ubuntu.

If anyone has an explanation for this behaviour (which doesn't seem to
happen on
later Ubuntu releases), I'd be happy to share more info.

Well.. I certainly fail to see how adding some other clock would
> overcome the fact that something can't keep up.. (rt priorties and
> suchlike?)
>

the main idea is to avoid filling up buffers by dropping some frames when
that kind
of thing happens. Or to introduce silence in the output under other
conditions.
The main idea is to avoid increasing drift between the emulated and host
system
when it comes to audio. Again, I'm not suggesting to implement anything like
that.


> >
> > - adding dynamic linking support to the esd and alsa backends
> > (using dlopen/dlsym allows the emulator to run on platforms where
> > all the corresponding libraries / sound server are not available).
>
> I don't think this is worth it for QEMU, after all shipping binaries
> is not what it's best known for.
>

:-)


>
> > - modifying the sub-system to be able to use different backends for
> >   audio input and output.
>
> Yeah i recall seeing this, and was wondering why Android needed this
> functionality.
>

this was to be able to get audio input from a wave file while sending the
output
to the host audio system. Turned out to be very useful to test the
VoiceRecorder
application. I also played with it to debug esd/also related problems.
It's not exactly something that is worth it for upstream QEMU.


> Complexity notwithstanding i happen to like the way things are done in
> DSound, the conceptually simple approach that is.
>

I must admit I totally fail to see any simplicity in DirectSound :-)

Regards

[-- Attachment #2: Type: text/html, Size: 4697 bytes --]

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[malc]

On Wed, 1 Apr 2009, David Turner wrote:

> On Wed, Apr 1, 2009 at 1:49 AM, malc <av1474@comtv.ru> wrote:
> 
> > Hmm.. please define low-powered..
> 
> 
> No problem, people have been running the Android SDK on 700 MHz.
> I used to run it on a 900Hz Celeron or something like that.
> 
> Today, I'm doing all my "minimal performance" tests on a venerable
> 1 GHz Pentium III laptop with an integrated Intel audio chipset running
> Windows XP (and a Linux install under VMWare).
> 
> Just to be clear, I'm not suggesting to anyone to fix anything here..
> 
> 
> >
> > And FWIW nowadays it's MPC7447A at 1.3Ghz which is not speed demon
> > either. Not that i dont belive you, esp considering esd thrown into
> > equation, but it would be interesting to know what people deem
> > low-powered currently.
> >

This thing is actually slower than the old Athlon.. Perhaps on
par/slightly faster than 1Ghz Pentium III.. And if ARM Android
emulates is big-endian - Pentium has some advantage even..

> Regarding esd, I unfortunately have to support the emulator binary
> running on Ubuntu 6.06 installations where the sound server will
> frequently lock up when the Android emulator is run. In some cases,
> this also totally freezes the whole desktop, though it is possible
> to recover by perfoming a "killall -9 esd" from a console.

I have very hard time parsing that, you are suspecting emulator binary
locking up esd daemon?
 
> I initially thought that the main reason for that was that the esd
> client library could not handle the multiple SIGALRM-induced EINTR
> returned by write() and other system calls, and left the sound
> server in a sorry state (maybe because it was stuck waiting for some
> data that would never arrive). However, I protected all calls to the
> esd backend (playing with the signal mask to avoid that), but this
> still doesn't get rid of the problem. Fortunately, this doesn't seem
> to happen with later versions of Ubuntu.

esdaudio.c blocks all signals for both playback and recording.. So
hum..

> If anyone has an explanation for this behaviour (which doesn't seem
> to happen on later Ubuntu releases), I'd be happy to share more
> info.
> 
> > Well.. I certainly fail to see how adding some other clock would
> > overcome the fact that something can't keep up.. (rt priorties and
> > suchlike?)
> >
> 
> the main idea is to avoid filling up buffers by dropping some frames
> when that kind of thing happens. Or to introduce silence in the
> output under other conditions.  The main idea is to avoid increasing
> drift between the emulated and host system when it comes to
> audio. Again, I'm not suggesting to implement anything like that.
> 

Drift can only grow as big as the ring buffer in any case...

> > >
> > > - adding dynamic linking support to the esd and alsa backends
> > > (using dlopen/dlsym allows the emulator to run on platforms where
> > > all the corresponding libraries / sound server are not available).
> >
> > I don't think this is worth it for QEMU, after all shipping binaries
> > is not what it's best known for.
> >
> 
> :-)
> 
> 
> >
> > > - modifying the sub-system to be able to use different backends for
> > >   audio input and output.
> >
> > Yeah i recall seeing this, and was wondering why Android needed this
> > functionality.
> >
> 
> this was to be able to get audio input from a wave file while
> sending the output to the host audio system. Turned out to be very
> useful to test the VoiceRecorder application. I also played with it
> to debug esd/also related problems.  It's not exactly something that
> is worth it for upstream QEMU.

Oh.. I never needed that, otherwise there would probably be a
wavplayback alongside wavcapture monitor command.

> 
> > Complexity notwithstanding i happen to like the way things are done in
> > DSound, the conceptually simple approach that is.
> >
> 
> I must admit I totally fail to see any simplicity in DirectSound :-)

Oh it's just overly complicated oss+mmap with locking and the ability
to get the current position, FMOD is almost exactly the same but the
wrapping is more palatable.

P.S. Your mails (raw text versions of them) have their lines wrapped
     weirdly.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Richard W.M. Jones]

On Mon, Mar 30, 2009 at 12:23:43AM +0300, Avi Kivity wrote:
> With the help of some Limoncino I noted several aspects of the Qemu coding
> style, particularly where it differs from the Linux coding style as many
> contributors work on both projects.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  CODING_STYLE |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 77 insertions(+), 0 deletions(-)
>  create mode 100644 CODING_STYLE
> 
> diff --git a/CODING_STYLE b/CODING_STYLE

Just as a general comment, have you thought about providing specific
guidance for emacs and vi users?

In libvirt, the HACKING file contains the following useful snippet for
emacs users:

http://git.et.redhat.com/?p=libvirt.git;a=blob;f=HACKING;h=ca39d61d85f2aaa3991f13a540772932208be7cd;hb=f5c687f03d8313407448fc642f8fefe95a655645#l59

They just paste that into their ~/.emacs, and any time they edit
libvirt code it "just works".

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Daniel P. Berrange]

On Tue, Mar 31, 2009 at 02:58:50PM +0200, David Turner wrote:
> Very frankly, I don't think that a coding style, even strictly applied, is
> going to make the QEMU code
> easier to understand.
> 
> The real barriers to understanding are the lack of structure in the code,
> liberal use of global macros
> scattered randomly in the source code, exceedingly liberally named
> functions, and sometimes obscure
> implementation of simple concepts (*cough* CharDriverState), cramming
> totally unrelated stuff in single
> largish source files (vl.c for the win !), and a blatant lack of
> documentation comments for a lot of subtle
> stuff in there to explain the magic.

Much of that is true, but there has been very active work addressing
these problems in recent times. If you look at the history of vl.c
for example, you'll see it has dropped from 10,000 lines to just
5,800 today, with much code split out to separate modules. There's of
course much more still todo in this area, but this is no reason to
not try and keep a clean & consistent coding style at the same
time as this refactoring.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] Document Qemu coding style

[Avi Kivity]

Richard W.M. Jones wrote:
> On Mon, Mar 30, 2009 at 12:23:43AM +0300, Avi Kivity wrote:
>   
>> With the help of some Limoncino I noted several aspects of the Qemu coding
>> style, particularly where it differs from the Linux coding style as many
>> contributors work on both projects.
>>     
> Just as a general comment, have you thought about providing specific
> guidance for emacs and vi users?
>
> In libvirt, the HACKING file contains the following useful snippet for
> emacs users:
>
> http://git.et.redhat.com/?p=libvirt.git;a=blob;f=HACKING;h=ca39d61d85f2aaa3991f13a540772932208be7cd;hb=f5c687f03d8313407448fc642f8fefe95a655645#l59
>
> They just paste that into their ~/.emacs, and any time they edit
> libvirt code it "just works".
>
>   

Good idea.  Maybe not in CODING_STYLE, but it's certainly helpful.

I use something similar in my ~/.emacs.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.