[Qemu-devel] [PATCH RFC 0/5] Another patchset try to clean up around qerror_report_err()

[Qemu-devel] [PATCH RFC 0/5] Another patchset try to clean up around qerror_report_err()

[zhanghailiang]

Hi,

I have noticed Markus's previous patchset 
"Clean up around error_get_pretty(), qerror_report_err()".

This is another patch series trying to do the same thing.
I'm not sure if he has missed this places or these modifies are false.

So, please review... 

Thanks.

zhanghailiang (5):
  serial-pci: Avoid qerror_report_err() outside QMP command handlers
  qdev: Avoid qerror_report_err() outside QMP command handlers
  pci-hotplug-old: Avoid qerror_report_err() outside QMP command
    handlers
  pci-assign: Avoid qerror_report_err() outside QMP command handlers
  savevm: Replace error_report() & error_free() with error_report_err()

 hw/char/serial-pci.c     | 7 ++-----
 hw/core/qdev.c           | 6 ++----
 hw/i386/kvm/pci-assign.c | 3 +--
 hw/pci/pci-hotplug-old.c | 3 +--
 savevm.c                 | 3 +--
 5 files changed, 7 insertions(+), 15 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH RFC 1/5] serial-pci: Avoid qerror_report_err() outside QMP command handlers

[zhanghailiang]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in serial_pci_init() and
multi_serial_pci_init().

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/char/serial-pci.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index f05c9b4..2b0bf54 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -27,7 +27,6 @@
 
 #include "hw/char/serial.h"
 #include "hw/pci/pci.h"
-#include "qapi/qmp/qerror.h"
 
 #define PCI_SERIAL_MAX_PORTS 4
 
@@ -57,8 +56,7 @@ static int serial_pci_init(PCIDevice *dev)
     s->baudbase = 115200;
     serial_realize_core(s, &err);
     if (err != NULL) {
-        qerror_report_err(err);
-        error_free(err);
+        error_report_err(err);
         return -1;
     }
 
@@ -116,8 +114,7 @@ static int multi_serial_pci_init(PCIDevice *dev)
         s->baudbase = 115200;
         serial_realize_core(s, &err);
         if (err != NULL) {
-            qerror_report_err(err);
-            error_free(err);
+            error_report_err(err);
             return -1;
         }
         s->irq = pci->irqs[i];
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC 2/5] qdev: Avoid qerror_report_err() outside QMP command handlers

[zhanghailiang]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in qdev_init() and
device_post_init().

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/core/qdev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 44c6b93..9adcd27 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -181,8 +181,7 @@ int qdev_init(DeviceState *dev)
 
     object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
     if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         object_unparent(OBJECT(dev));
         return -1;
     }
@@ -1189,8 +1188,7 @@ static void device_post_init(Object *obj)
     Error *err = NULL;
     qdev_prop_set_globals(DEVICE(obj), &err);
     if (err) {
-        qerror_report_err(err);
-        error_free(err);
+        error_report_err(err);
         exit(EXIT_FAILURE);
     }
 }
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC 3/5] pci-hotplug-old: Avoid qerror_report_err() outside QMP command handlers

[zhanghailiang]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP. It should not be used
elsewhere.  Replace by error_report_err() in qemu_pci_hot_add_nic().

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/pci/pci-hotplug-old.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
index beea6d2..477bafc 100644
--- a/hw/pci/pci-hotplug-old.c
+++ b/hw/pci/pci-hotplug-old.c
@@ -91,8 +91,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
 
     ret = net_client_init(opts, 0, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         return NULL;
     }
     if (nd_table[ret].devaddr) {
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC 4/5] pci-assign: Avoid qerror_report_err() outside QMP command handlers

[zhanghailiang]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.  Replace by error_report_err() in assigned_initfn().

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hw/i386/kvm/pci-assign.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index bd92c69..f2f4e07 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1831,8 +1831,7 @@ out:
 
 exit_with_error:
     assert(local_err);
-    qerror_report_err(local_err);
-    error_free(local_err);
+    error_report_err(local_err);
     return -1;
 }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH RFC 5/5] savevm: Replace error_report() & error_free() with error_report_err()

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 savevm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index ce2b6a2..c4f8c39 100644
--- a/savevm.c
+++ b/savevm.c
@@ -932,8 +932,7 @@ int qemu_loadvm_state(QEMUFile *f)
     int ret;
 
     if (qemu_savevm_state_blocked(&local_err)) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_report_err(local_err);
         return -EINVAL;
     }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH RFC 0/5] Another patchset try to clean up around qerror_report_err()

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> Hi,
>
> I have noticed Markus's previous patchset 
> "Clean up around error_get_pretty(), qerror_report_err()".
>
> This is another patch series trying to do the same thing.
> I'm not sure if he has missed this places or these modifies are false.
>
> So, please review... 
>
> Thanks.

Help with qerror elimination is of course appreciated!  However, there's
overlap with related patches in flight.  Easy to miss, because there are
so many of them.  I'll elaborate in replies to your patches.

Re: [Qemu-devel] [PATCH RFC 1/5] serial-pci: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> qerror_report_err() is a transitional interface to help with
> converting existing monitor commands to QMP.  It should not be used
> elsewhere.  Replace by error_report_err() in serial_pci_init() and
> multi_serial_pci_init().
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/char/serial-pci.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> index f05c9b4..2b0bf54 100644
> --- a/hw/char/serial-pci.c
> +++ b/hw/char/serial-pci.c
> @@ -27,7 +27,6 @@
>  
>  #include "hw/char/serial.h"
>  #include "hw/pci/pci.h"
> -#include "qapi/qmp/qerror.h"
>  
>  #define PCI_SERIAL_MAX_PORTS 4
>  
> @@ -57,8 +56,7 @@ static int serial_pci_init(PCIDevice *dev)
>      s->baudbase = 115200;
>      serial_realize_core(s, &err);
>      if (err != NULL) {
> -        qerror_report_err(err);
> -        error_free(err);
> +        error_report_err(err);
>          return -1;
>      }
>  
> @@ -116,8 +114,7 @@ static int multi_serial_pci_init(PCIDevice *dev)
>          s->baudbase = 115200;
>          serial_realize_core(s, &err);
>          if (err != NULL) {
> -            qerror_report_err(err);
> -            error_free(err);
> +            error_report_err(err);
>              return -1;
>          }
>          s->irq = pci->irqs[i];

This breaks the error reply when you hot plug the devices via QMP
device_add.

The correct solution is my "[PATCH 06/10] serial-pci: Convert to
realize".

Re: [Qemu-devel] [PATCH RFC 2/5] qdev: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> qerror_report_err() is a transitional interface to help with
> converting existing monitor commands to QMP.  It should not be used
> elsewhere.  Replace by error_report_err() in qdev_init() and
> device_post_init().
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/core/qdev.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 44c6b93..9adcd27 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -181,8 +181,7 @@ int qdev_init(DeviceState *dev)
>  
>      object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
>      if (local_err != NULL) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +        error_report_err(local_err);
>          object_unparent(OBJECT(dev));
>          return -1;
>      }

This breaks QMP device_add's error reply.

The patches I posted together remove all calls of qdev_init().  Once
they're all merged, the function should be simply dropped.

> @@ -1189,8 +1188,7 @@ static void device_post_init(Object *obj)
>      Error *err = NULL;
>      qdev_prop_set_globals(DEVICE(obj), &err);
>      if (err) {
> -        qerror_report_err(err);
> -        error_free(err);
> +        error_report_err(err);
>          exit(EXIT_FAILURE);
>      }
>  }

This one is part of a wider problem I solved in

[PATCH] qdev: Don't exit when running into bad -global

Re: [Qemu-devel] [PATCH RFC 3/5] pci-hotplug-old: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> qerror_report_err() is a transitional interface to help with
> converting existing monitor commands to QMP. It should not be used
> elsewhere.  Replace by error_report_err() in qemu_pci_hot_add_nic().
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/pci/pci-hotplug-old.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index beea6d2..477bafc 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -91,8 +91,7 @@ static PCIDevice *qemu_pci_hot_add_nic(Monitor *mon,
>  
>      ret = net_client_init(opts, 0, &local_err);
>      if (local_err) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +        error_report_err(local_err);
>          return NULL;
>      }
>      if (nd_table[ret].devaddr) {

Hard to see, but this is actually dead code.

[PATCH v2 0/2] pci: Bury dead legacy commands pci_add, pci_del

Re: [Qemu-devel] [PATCH RFC 4/5] pci-assign: Avoid qerror_report_err() outside QMP command handlers

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> qerror_report_err() is a transitional interface to help with
> converting existing monitor commands to QMP.  It should not be used
> elsewhere.  Replace by error_report_err() in assigned_initfn().
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hw/i386/kvm/pci-assign.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index bd92c69..f2f4e07 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -1831,8 +1831,7 @@ out:
>  
>  exit_with_error:
>      assert(local_err);
> -    qerror_report_err(local_err);
> -    error_free(local_err);
> +    error_report_err(local_err);
>      return -1;
>  }

This breaks the error reply when you hot plug the devices via QMP
device_add.

A correct solution is my "[PATCH 10/10] pci-assign: Convert to realize".

Re: [Qemu-devel] [PATCH RFC 5/5] savevm: Replace error_report() & error_free() with error_report_err()

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  savevm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index ce2b6a2..c4f8c39 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -932,8 +932,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      int ret;
>  
>      if (qemu_savevm_state_blocked(&local_err)) {
> -        error_report("%s", error_get_pretty(local_err));
> -        error_free(local_err);
> +        error_report_err(local_err);
>          return -EINVAL;
>      }

This one's good.  My commit 565f65d "error: Use error_report_err() where
appropriate" cleaned up this pattern, but a few more instances have
since crept in, and this is one.

I can see another one in hw/arm/virt.c machvirt_init().

Re: [Qemu-devel] [PATCH RFC 0/5] Another patchset try to clean up around qerror_report_err()

[zhanghailiang]

On 2015/3/2 18:01, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> Hi,
>>
>> I have noticed Markus's previous patchset
>> "Clean up around error_get_pretty(), qerror_report_err()".
>>
>> This is another patch series trying to do the same thing.
>> I'm not sure if he has missed this places or these modifies are false.
>>
>> So, please review...
>>
>> Thanks.
>
> Help with qerror elimination is of course appreciated!  However, there's
> overlap with related patches in flight.  Easy to miss, because there are
> so many of them.  I'll elaborate in replies to your patches.
>
>

Hmm, thanks for your reply, and i saw most of these problems have been fixed by you,
this is great :)

Re: [Qemu-devel] [PATCH RFC 5/5] savevm: Replace error_report() & error_free() with error_report_err()

[zhanghailiang]

On 2015/3/2 18:26, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   savevm.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index ce2b6a2..c4f8c39 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -932,8 +932,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>       int ret;
>>
>>       if (qemu_savevm_state_blocked(&local_err)) {
>> -        error_report("%s", error_get_pretty(local_err));
>> -        error_free(local_err);
>> +        error_report_err(local_err);
>>           return -EINVAL;
>>       }
>
> This one's good.  My commit 565f65d "error: Use error_report_err() where
> appropriate" cleaned up this pattern, but a few more instances have
> since crept in, and this is one.
>
> I can see another one in hw/arm/virt.c machvirt_init().
>
>

Er, do you mean the follow place ?

        /* Handle any CPU options specified by the user */
         cc->parse_features(CPU(cpuobj), cpustr[1], &err);
         if (err) {
             error_report("%s", error_get_pretty(err));
             exit(1);
         }

Re: [Qemu-devel] [PATCH RFC 5/5] savevm: Replace error_report() & error_free() with error_report_err()

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> On 2015/3/2 18:26, Markus Armbruster wrote:
>> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   savevm.c | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/savevm.c b/savevm.c
>>> index ce2b6a2..c4f8c39 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -932,8 +932,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>>       int ret;
>>>
>>>       if (qemu_savevm_state_blocked(&local_err)) {
>>> -        error_report("%s", error_get_pretty(local_err));
>>> -        error_free(local_err);
>>> +        error_report_err(local_err);
>>>           return -EINVAL;
>>>       }
>>
>> This one's good.  My commit 565f65d "error: Use error_report_err() where
>> appropriate" cleaned up this pattern, but a few more instances have
>> since crept in, and this is one.
>>
>> I can see another one in hw/arm/virt.c machvirt_init().
>>
>>
>
> Er, do you mean the follow place ?
>
>        /* Handle any CPU options specified by the user */
>         cc->parse_features(CPU(cpuobj), cpustr[1], &err);
>         if (err) {
>             error_report("%s", error_get_pretty(err));
>             exit(1);
>         }

Yes.

Re: [Qemu-devel] [PATCH RFC 5/5] savevm: Replace error_report() & error_free() with error_report_err()

[zhanghailiang]

On 2015/3/3 17:19, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> On 2015/3/2 18:26, Markus Armbruster wrote:
>>> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    savevm.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/savevm.c b/savevm.c
>>>> index ce2b6a2..c4f8c39 100644
>>>> --- a/savevm.c
>>>> +++ b/savevm.c
>>>> @@ -932,8 +932,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>>>        int ret;
>>>>
>>>>        if (qemu_savevm_state_blocked(&local_err)) {
>>>> -        error_report("%s", error_get_pretty(local_err));
>>>> -        error_free(local_err);
>>>> +        error_report_err(local_err);
>>>>            return -EINVAL;
>>>>        }
>>>
>>> This one's good.  My commit 565f65d "error: Use error_report_err() where
>>> appropriate" cleaned up this pattern, but a few more instances have
>>> since crept in, and this is one.
>>>
>>> I can see another one in hw/arm/virt.c machvirt_init().
>>>
>>>
>>
>> Er, do you mean the follow place ?
>>
>>         /* Handle any CPU options specified by the user */
>>          cc->parse_features(CPU(cpuobj), cpustr[1], &err);
>>          if (err) {
>>              error_report("%s", error_get_pretty(err));
>>              exit(1);
>>          }
>
> Yes.

OK, i will send a patch fix these two places. Thanks. :)