[Qemu-devel] [PATCH] Add new function qemu_register_machines

[Qemu-devel] [PATCH] Add new function qemu_register_machines

[Stefan Weil]

Add new function qemu_register_machines.

The patch removes the unused prototype register_machines
and adds a new function which makes registration of
more than one machine a little easier.

The new function is applied to the machines in hw/spitz.c
(where a static keyword for akitapda_machine was missing),
but it can also be applied to several other QEMU source files.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/boards.h |    2 +-
 hw/spitz.c  |   48 ++++++++++++++++++++++--------------------------
 vl.c        |    9 +++++++++
 3 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/hw/boards.h b/hw/boards.h
index 3144017..f0e6932 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -20,7 +20,7 @@ typedef struct QEMUMachine {
 } QEMUMachine;
 
 int qemu_register_machine(QEMUMachine *m);
-void register_machines(void);
+int qemu_register_machines(QEMUMachine *machine, size_t number);
 
 extern QEMUMachine *current_machine;
 
diff --git a/hw/spitz.c b/hw/spitz.c
index dd16780..dd3a57e 100644
--- a/hw/spitz.c
+++ b/hw/spitz.c
@@ -1044,36 +1044,32 @@ static void terrier_init(ram_addr_t ram_size,
                 kernel_cmdline, initrd_filename, cpu_model, terrier,
0x33f);
 }
 
-QEMUMachine akitapda_machine = {
-    .name = "akita",
-    .desc = "Akita PDA (PXA270)",
-    .init = akita_init,
-};
-
-static QEMUMachine spitzpda_machine = {
-    .name = "spitz",
-    .desc = "Spitz PDA (PXA270)",
-    .init = spitz_init,
-};
-
-static QEMUMachine borzoipda_machine = {
-    .name = "borzoi",
-    .desc = "Borzoi PDA (PXA270)",
-    .init = borzoi_init,
-};
-
-static QEMUMachine terrierpda_machine = {
-    .name = "terrier",
-    .desc = "Terrier PDA (PXA270)",
-    .init = terrier_init,
+static QEMUMachine spitz_machines[] = {
+    {
+        .name = "akita",
+        .desc = "Akita PDA (PXA270)",
+        .init = akita_init,
+    },
+    {
+        .name = "spitz",
+        .desc = "Spitz PDA (PXA270)",
+        .init = spitz_init,
+    },
+    {
+        .name = "borzoi",
+        .desc = "Borzoi PDA (PXA270)",
+        .init = borzoi_init,
+    },
+    {
+        .name = "terrier",
+        .desc = "Terrier PDA (PXA270)",
+        .init = terrier_init,
+    }
 };
 
 static void spitz_machine_init(void)
 {
-    qemu_register_machine(&akitapda_machine);
-    qemu_register_machine(&spitzpda_machine);
-    qemu_register_machine(&borzoipda_machine);
-    qemu_register_machine(&terrierpda_machine);
+    qemu_register_machines(spitz_machines, ARRAY_SIZE(spitz_machines));
 }
 
 machine_init(spitz_machine_init);
diff --git a/vl.c b/vl.c
index 68c8514..d07a1b6 100644
--- a/vl.c
+++ b/vl.c
@@ -3488,6 +3488,15 @@ int qemu_register_machine(QEMUMachine *m)
     return 0;
 }
 
+int qemu_register_machines(QEMUMachine *machine, size_t number)
+{
+    size_t i;
+    for (i = 0; i < number; i++) {
+        qemu_register_machine(&machine[i]);
+    }
+    return 0;
+}
+
 static QEMUMachine *find_machine(const char *name)
 {
     QEMUMachine *m;
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH] Add new function qemu_register_machines

[Anthony Liguori]

Stefan Weil wrote:
> Add new function qemu_register_machines.
>
> The patch removes the unused prototype register_machines
> and adds a new function which makes registration of
> more than one machine a little easier.
>
> The new function is applied to the machines in hw/spitz.c
> (where a static keyword for akitapda_machine was missing),
> but it can also be applied to several other QEMU source files.
>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>   

Might as well eliminate qemu_register_machine() and convert everything 
to use qemu_register_machines().

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Add new function qemu_register_machines

[Stefan Weil]

Anthony Liguori schrieb:
> Stefan Weil wrote:
>> Add new function qemu_register_machines.
>>
>> The patch removes the unused prototype register_machines
>> and adds a new function which makes registration of
>> more than one machine a little easier.
>>
>> The new function is applied to the machines in hw/spitz.c
>> (where a static keyword for akitapda_machine was missing),
>> but it can also be applied to several other QEMU source files.
>>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>   
>
> Might as well eliminate qemu_register_machine() and convert everything
> to use qemu_register_machines().
>

Yes. But I prefer to have both interfaces.
qemu_register_machine(&my_machine) looks nicer
than qemu_register_machines(&my_machine, 1).

[Qemu-devel] Re: [PATCH] Add new function qemu_register_machines

[Jan Kiszka]

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

Stefan Weil wrote:
> Anthony Liguori schrieb:
>> Stefan Weil wrote:
>>> Add new function qemu_register_machines.
>>>
>>> The patch removes the unused prototype register_machines
>>> and adds a new function which makes registration of
>>> more than one machine a little easier.
>>>
>>> The new function is applied to the machines in hw/spitz.c
>>> (where a static keyword for akitapda_machine was missing),
>>> but it can also be applied to several other QEMU source files.
>>>
>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>>   
>> Might as well eliminate qemu_register_machine() and convert everything
>> to use qemu_register_machines().
>>
> 
> Yes. But I prefer to have both interfaces.
> qemu_register_machine(&my_machine) looks nicer
> than qemu_register_machines(&my_machine, 1).
> 

static inline void qemu_register_machine(QEMUMachine *m)
{
    qemu_register_machines(m, 1);
}

BTW, this is also a good chance to drop the now unused return value.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] Add new function qemu_register_machines

[Stefan Weil]

Jan Kiszka schrieb:
> Stefan Weil wrote:
>   
>> Anthony Liguori schrieb:
>>     
>>> Stefan Weil wrote:
>>>       
>>>> Add new function qemu_register_machines.
>>>>
>>>> The patch removes the unused prototype register_machines
>>>> and adds a new function which makes registration of
>>>> more than one machine a little easier.
>>>>
>>>> The new function is applied to the machines in hw/spitz.c
>>>> (where a static keyword for akitapda_machine was missing),
>>>> but it can also be applied to several other QEMU source files.
>>>>
>>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>>>   
>>>>         
>>> Might as well eliminate qemu_register_machine() and convert everything
>>> to use qemu_register_machines().
>>>
>>>       
>> Yes. But I prefer to have both interfaces.
>> qemu_register_machine(&my_machine) looks nicer
>> than qemu_register_machines(&my_machine, 1).
>>
>>     
>
> static inline void qemu_register_machine(QEMUMachine *m)
> {
>     qemu_register_machines(m, 1);
> }
>
>   

No. Inline for functions which are only called once
is a (very small, I admit) waste of code.

> BTW, this is also a good chance to drop the now unused return value.
>
> Jan
>
>   

Yes. But one never knows,  maybe some day it will be used again? :-)

[Qemu-devel] Re: [PATCH] Add new function qemu_register_machines

[Jan Kiszka]

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

Stefan Weil wrote:
> Jan Kiszka schrieb:
>> Stefan Weil wrote:
>>   
>>> Anthony Liguori schrieb:
>>>     
>>>> Stefan Weil wrote:
>>>>       
>>>>> Add new function qemu_register_machines.
>>>>>
>>>>> The patch removes the unused prototype register_machines
>>>>> and adds a new function which makes registration of
>>>>> more than one machine a little easier.
>>>>>
>>>>> The new function is applied to the machines in hw/spitz.c
>>>>> (where a static keyword for akitapda_machine was missing),
>>>>> but it can also be applied to several other QEMU source files.
>>>>>
>>>>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>>>>>   
>>>>>         
>>>> Might as well eliminate qemu_register_machine() and convert everything
>>>> to use qemu_register_machines().
>>>>
>>>>       
>>> Yes. But I prefer to have both interfaces.
>>> qemu_register_machine(&my_machine) looks nicer
>>> than qemu_register_machines(&my_machine, 1).
>>>
>>>     
>> static inline void qemu_register_machine(QEMUMachine *m)
>> {
>>     qemu_register_machines(m, 1);
>> }
>>
>>   
> 
> No. Inline for functions which are only called once
> is a (very small, I admit) waste of code.

OK, that's a matter of taste.

> 
>> BTW, this is also a good chance to drop the now unused return value.
>>
>> Jan
>>
>>   
> 
> Yes. But one never knows,  maybe some day it will be used again? :-)

Not a single existing user checks the return code.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] Add new function qemu_register_machines

[Anthony Liguori]

Jan Kiszka wrote:
>>> BTW, this is also a good chance to drop the now unused return value.
>>>
>>> Jan
>>>
>>>   
>>>       
>> Yes. But one never knows,  maybe some day it will be used again? :-)
>>     
>
> Not a single existing user checks the return code.
>   

It's not quite an interface worth worry about too deeply.  It should go 
away once we have machine configuration files in place which will 
hopefully be very soon.

> Jan
>
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Add new function qemu_register_machines

[Paul Brook]

On Thursday 21 May 2009, Stefan Weil wrote:
> Add new function qemu_register_machines.
>
> The patch removes the unused prototype register_machines
> and adds a new function which makes registration of
> more than one machine a little easier.

I don't see this as an improvement.

Paul

Re: [Qemu-devel] [PATCH] Add new function qemu_register_machines

[Stefan Weil]

Paul Brook schrieb:
> On Thursday 21 May 2009, Stefan Weil wrote:
>> Add new function qemu_register_machines.
>>
>> The patch removes the unused prototype register_machines
>> and adds a new function which makes registration of
>> more than one machine a little easier.
>
> I don't see this as an improvement.
>
> Paul

Hi Paul,

well, it's a matter of personal taste whether you prefer
to have one or two interfaces for machine registration.

The new function is an improvement if you want to reduce
lines of code and binary code size.

Removing unused entries like register_machines from
header files is also an improvement. This part of
my patch is fixed now in QEMU HEAD, thanks Anthony!

And adding "static" to a machine declaration which is
only used locally is also an improvement (hw/spitz.c).

I assume that you agree that those last two points are
an improvement.

So the patch improves two details and adds something
where people disagree about its usefulness.

What now? Will the patch be rejected because you
don't see an improvement? Will you fix the missing
"static" yourself? Do you want a new patch without
qemu_register_machines? Or a new patch which uses
qemu_register_machines for all files with more than
one machine?

Your short feedback is not really helpful because there
remain too many questions.

Stefan

Re: [Qemu-devel] [PATCH] Add new function qemu_register_machines

[Paul Brook]

> well, it's a matter of personal taste whether you prefer
> to have one or two interfaces for machine registration.

True. This is not a performance critical bit of code and has a well bounded 
number of occurrences so I'm inclined to go for simplicity/clarity over 
efficiency.  If we use arrays I recommend doing so consistently, even in the 
single machine case. Given your goal is to avoid repeated code I'd also use 
something along the lines of:
#define qemu_register_machines(x) qemu_do_register_machines(x, ARRAY_SIZE(x))

> The new function is an improvement if you want to reduce
> lines of code and binary code size.
>
> Removing unused entries like register_machines from
> header files is also an improvement. This part of
> my patch is fixed now in QEMU HEAD, thanks Anthony!
>
> And adding "static" to a machine declaration which is
> only used locally is also an improvement (hw/spitz.c).
>
> I assume that you agree that those last two points are
> an improvement.

Yes.

> So the patch improves two details and adds something
> where people disagree about its usefulness.
>
> What now? Will the patch be rejected because you
> don't see an improvement? Will you fix the missing
> "static" yourself? Do you want a new patch without
> qemu_register_machines? Or a new patch which uses
> qemu_register_machines for all files with more than
> one machine?

I'm never going to approve a patch until I'm happy with all of it. In general 
this is why it's a bad idea to mix cleanups and new features. In this case 
your new feature obsoletes some of your cleanups, so the case for separate 
patches is less clear.  However unless I'm feeling particularly bored or 
enthusiastic I'm unlikely to split up or rework the patch for you.

Paul