Re: [Qemu-devel] [PATCH 3/4] vl: Clean up after previous commit

From: Markus Armbruster <armbru@redhat.com>
To: Wei Yang <richardw.yang@linux.intel.com>
Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com
Subject: Re: [Qemu-devel] [PATCH 3/4] vl: Clean up after previous commit
Date: Thu, 04 Apr 2019 17:49:52 +0200	[thread overview]
Message-ID: <87d0m16a8v.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20190403221003.GA18809@richard> (Wei Yang's message of "Thu, 4 Apr 2019 06:10:03 +0800")

Wei Yang <richardw.yang@linux.intel.com> writes:

> On Tue, Apr 02, 2019 at 03:26:49PM +0200, Markus Armbruster wrote:
>>Since the previous commit, find_machine() and find_default_machine()
>>don't have to deallocate on return.  This permits further
>>simplifications.
>>
>>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>---
>> vl.c | 25 +++++++------------------
>> 1 file changed, 7 insertions(+), 18 deletions(-)
>>
>>diff --git a/vl.c b/vl.c
>>index 126081f595..6a31e5bfac 100644
>>--- a/vl.c
>>+++ b/vl.c
>>@@ -1467,40 +1467,29 @@ MachineState *current_machine;
>> static MachineClass *find_machine(const char *name, GSList *machines)
>> {
>>     GSList *el;
>>-    MachineClass *mc = NULL;
>> 
>>     for (el = machines; el; el = el->next) {
>>-        MachineClass *temp = el->data;
>>+        MachineClass *mc = el->data;
>> 
>>-        if (!strcmp(temp->name, name)) {
>>-            mc = temp;
>>-            break;
>>-        }
>>-        if (temp->alias &&
>>-            !strcmp(temp->alias, name)) {
>>-            mc = temp;
>>-            break;
>>+        if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
>>+            return mc;
>>         }
>>     }
>> 
>>-    return mc;
>>+    return NULL;
>> }
>> 
>> static MachineClass *find_default_machine(GSList *machines)
>> {
>>     GSList *el;
>>-    MachineClass *mc = NULL;
>> 
>>     for (el = machines; el; el = el->next) {
>>-        MachineClass *temp = el->data;
>>-
>>-        if (temp->is_default) {
>>-            mc = temp;
>>-            break;
>>+        if (((MachineClass *)el->data)->is_default) {
>>+            return el->data;
>>         }
>>     }
>
> Generally it looks good to me.
>
> One tiny suggestion here is to define 
>
>         MachineClass *mc = el->data;
>
> just as it does in find_machin() and return mc instead of raw el->data.
>
> If you agree with that I will modify this at my place.

No objection.

>> 
>>-    return mc;
>>+    return NULL;
>> }
>> 
>> MachineInfoList *qmp_query_machines(Error **errp)
>>-- 
>>2.17.2