[Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP. It should not be used elsewhere. 

v2 -> v1:
 - update including head files, remove qerror.h and monitor.h,
   add error-report.h (Markus)
 - add 'Reviewed-by' tag.

Gonglei (2):
  redirect.c: Don't use qerror_report()
  dev-network: Don't use qerror_report_err()

 hw/usb/dev-network.c | 4 ++--
 hw/usb/redirect.c    | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 1/2] redirect.c: Don't use qerror_report()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

qerror_report() is a transitional interface to help with converting
existing HMP commands to QMP.  It should not be used elsewhere.
Replace by error_report().

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/redirect.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 44522d9..959a43c 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -27,7 +27,7 @@
 
 #include "qemu-common.h"
 #include "qemu/timer.h"
-#include "monitor/monitor.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "qemu/iov.h"
 #include "sysemu/char.h"
@@ -1363,7 +1363,7 @@ static int usbredir_initfn(USBDevice *udev)
     int i;
 
     if (dev->cs == NULL) {
-        qerror_report(QERR_MISSING_PARAMETER, "chardev");
+        error_report("Parameter 'chardev' is missing");
         return -1;
     }
 
@@ -1372,8 +1372,8 @@ static int usbredir_initfn(USBDevice *udev)
                                            &dev->filter_rules,
                                            &dev->filter_rules_count);
         if (i) {
-            qerror_report(QERR_INVALID_PARAMETER_VALUE, "filter",
-                          "a usb device filter string");
+            error_report("Parameter 'filter' expects "
+                         "a usb device filter string");
             return -1;
         }
     }
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 2/2] dev-network: Don't use qerror_report_err()

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

qerror_report_err() is a transitional interface to help with
converting existing HMP commands to QMP.  It should not be used
elsewhere.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/dev-network.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 518d536..429ff58 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -27,7 +27,7 @@
 #include "hw/usb.h"
 #include "hw/usb/desc.h"
 #include "net/net.h"
-#include "qapi/qmp/qerror.h"
+#include "qemu/error-report.h"
 #include "qemu/queue.h"
 #include "qemu/config-file.h"
 #include "sysemu/sysemu.h"
@@ -1392,7 +1392,7 @@ static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
 
     idx = net_client_init(opts, 0, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
+        error_report("%s", error_get_pretty(local_err));
         error_free(local_err);
         return NULL;
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Paolo Bonzini]

Il 12/09/2014 09:30, arei.gonglei@huawei.com ha scritto:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> qerror_report() is a transitional interface to help with converting
> existing HMP commands to QMP. It should not be used elsewhere. 
> 
> v2 -> v1:
>  - update including head files, remove qerror.h and monitor.h,
>    add error-report.h (Markus)
>  - add 'Reviewed-by' tag.
> 
> Gonglei (2):
>   redirect.c: Don't use qerror_report()
>   dev-network: Don't use qerror_report_err()
> 
>  hw/usb/dev-network.c | 4 ++--
>  hw/usb/redirect.c    | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 

This is not an improvement I think; patch 1 especially for patch 2 where
you are discarding the Error *.

Any chance you could convert USB from init to realize, instead?  Then
you can remove the error printing altogether, and just the obsolete
functions.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gonglei (Arei)]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Friday, September 12, 2014 4:35 PM
> To: Gonglei (Arei); qemu-devel@nongnu.org
> Cc: Huangweidong (C); kraxel@redhat.com; armbru@redhat.com
> Subject: Re: [PATCH v2 0/2] usb: Don't use qerror_report
> 
> Il 12/09/2014 09:30, arei.gonglei@huawei.com ha scritto:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > qerror_report() is a transitional interface to help with converting
> > existing HMP commands to QMP. It should not be used elsewhere.
> >
> > v2 -> v1:
> >  - update including head files, remove qerror.h and monitor.h,
> >    add error-report.h (Markus)
> >  - add 'Reviewed-by' tag.
> >
> > Gonglei (2):
> >   redirect.c: Don't use qerror_report()
> >   dev-network: Don't use qerror_report_err()
> >
> >  hw/usb/dev-network.c | 4 ++--
> >  hw/usb/redirect.c    | 8 ++++----
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> 
> This is not an improvement I think; patch 1 especially for patch 2 where
> you are discarding the Error *.
> 
> Any chance you could convert USB from init to realize, instead?  Then
> you can remove the error printing altogether, and just the obsolete
> functions.
> 
As far as I can tell, almost all devices belong to USB sub-system are
using init method. It may be a big surgery if change all those devices
from init to realize. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Paolo Bonzini]

Il 12/09/2014 10:50, Gonglei (Arei) ha scritto:
>> > Any chance you could convert USB from init to realize, instead?  Then
>> > you can remove the error printing altogether, and just the obsolete
>> > functions.
>> > 
> As far as I can tell, almost all devices belong to USB sub-system are
> using init method. It may be a big surgery if change all those devices
> from init to realize. :)

Yes, that would have to be done in the whole USB subsystem.  I
understand that you don't feel like doing that. :)

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gonglei (Arei)]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Friday, September 12, 2014 4:59 PM
> Subject: Re: [PATCH v2 0/2] usb: Don't use qerror_report
> 
> Il 12/09/2014 10:50, Gonglei (Arei) ha scritto:
> >> > Any chance you could convert USB from init to realize, instead?  Then
> >> > you can remove the error printing altogether, and just the obsolete
> >> > functions.
> >> >
> > As far as I can tell, almost all devices belong to USB sub-system are
> > using init method. It may be a big surgery if change all those devices
> > from init to realize. :)
> 
> Yes, that would have to be done in the whole USB subsystem.  I
> understand that you don't feel like doing that. :)
> 
Haw-haw, maybe Gerd has a plan in the future...

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gonglei (Arei)]

> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Friday, September 12, 2014 3:31 PM
> To: qemu-devel@nongnu.org
> Cc: kraxel@redhat.com; Huangweidong (C); armbru@redhat.com; Gonglei
> (Arei)
> Subject: [PATCH v2 0/2] usb: Don't use qerror_report
> 
> From: Gonglei <arei.gonglei@huawei.com>
> 
> qerror_report() is a transitional interface to help with converting
> existing HMP commands to QMP. It should not be used elsewhere.
> 
> v2 -> v1:
>  - update including head files, remove qerror.h and monitor.h,
>    add error-report.h (Markus)
>  - add 'Reviewed-by' tag.
> 
> Gonglei (2):
>   redirect.c: Don't use qerror_report()
>   dev-network: Don't use qerror_report_err()
> 
>  hw/usb/dev-network.c | 4 ++--
>  hw/usb/redirect.c    | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> --
> 1.7.12.4
> 
Hi, Gerd 
Would you like to apply this patch series at present? Thanks!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Paolo Bonzini]

Il 16/09/2014 15:54, Gonglei (Arei) ha scritto:
>> -----Original Message-----
>> From: Gonglei (Arei)
>> Sent: Friday, September 12, 2014 3:31 PM
>> To: qemu-devel@nongnu.org
>> Cc: kraxel@redhat.com; Huangweidong (C); armbru@redhat.com; Gonglei
>> (Arei)
>> Subject: [PATCH v2 0/2] usb: Don't use qerror_report
>>
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> qerror_report() is a transitional interface to help with converting
>> existing HMP commands to QMP. It should not be used elsewhere.
>>
>> v2 -> v1:
>>  - update including head files, remove qerror.h and monitor.h,
>>    add error-report.h (Markus)
>>  - add 'Reviewed-by' tag.
>>
>> Gonglei (2):
>>   redirect.c: Don't use qerror_report()
>>   dev-network: Don't use qerror_report_err()
>>
>>  hw/usb/dev-network.c | 4 ++--
>>  hw/usb/redirect.c    | 8 ++++----
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> --
>> 1.7.12.4
>>
> Hi, Gerd 
> Would you like to apply this patch series at present? Thanks!

I'm not sure what the improvement is?

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gerd Hoffmann]

  Hi,

> > Gonglei (2):
> >   redirect.c: Don't use qerror_report()
> >   dev-network: Don't use qerror_report_err()

> Hi, Gerd 
> Would you like to apply this patch series at present? Thanks!

Picked it up now.  Originally dropped due to review comments from paolo,
but switching all usb over to realize is a bigger project indeed.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 16/09/2014 15:54, Gonglei (Arei) ha scritto:
>>> -----Original Message-----
>>> From: Gonglei (Arei)
>>> Sent: Friday, September 12, 2014 3:31 PM
>>> To: qemu-devel@nongnu.org
>>> Cc: kraxel@redhat.com; Huangweidong (C); armbru@redhat.com; Gonglei
>>> (Arei)
>>> Subject: [PATCH v2 0/2] usb: Don't use qerror_report
>>>
>>> From: Gonglei <arei.gonglei@huawei.com>
>>>
>>> qerror_report() is a transitional interface to help with converting
>>> existing HMP commands to QMP. It should not be used elsewhere.
>>>
>>> v2 -> v1:
>>>  - update including head files, remove qerror.h and monitor.h,
>>>    add error-report.h (Markus)
>>>  - add 'Reviewed-by' tag.
>>>
>>> Gonglei (2):
>>>   redirect.c: Don't use qerror_report()
>>>   dev-network: Don't use qerror_report_err()
>>>
>>>  hw/usb/dev-network.c | 4 ++--
>>>  hw/usb/redirect.c    | 8 ++++----
>>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> --
>>> 1.7.12.4
>>>
>> Hi, Gerd 
>> Would you like to apply this patch series at present? Thanks!
>
> I'm not sure what the improvement is?

The improvement is obvious: three calls of qerror_report() gone.

The regression isn't as obvious (I missed it, but fortunately Paolo
spotted it): when we run in QMP context, we screw up the error reply.

Therefore, we can't take these patches.  We really have to convert to
realize to get rid of qerror_report() here.

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> > Gonglei (2):
>> >   redirect.c: Don't use qerror_report()
>> >   dev-network: Don't use qerror_report_err()
>
>> Hi, Gerd 
>> Would you like to apply this patch series at present? Thanks!
>
> Picked it up now.  Originally dropped due to review comments from paolo,
> but switching all usb over to realize is a bigger project indeed.

Please don't, it degrades QMP errors.  Example:

    { "execute": "device_add", "arguments": { "driver": "usb-redir" } }

Reply before:

    {"error": {"class": "GenericError", "desc": "Parameter 'chardev' is missing"}}

After:

    {"error": {"class": "GenericError", "desc": "Device initialization failed."}}

with "Parameter 'chardev' is missing" barfed to stderr.

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gonglei (Arei)]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, September 16, 2014 10:26 PM
> To: Paolo Bonzini
> Cc: Gonglei (Arei); qemu-devel@nongnu.org; Huangweidong (C);
> kraxel@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 16/09/2014 15:54, Gonglei (Arei) ha scritto:
> >>> -----Original Message-----
> >>> From: Gonglei (Arei)
> >>> Sent: Friday, September 12, 2014 3:31 PM
> >>> To: qemu-devel@nongnu.org
> >>> Cc: kraxel@redhat.com; Huangweidong (C); armbru@redhat.com; Gonglei
> >>> (Arei)
> >>> Subject: [PATCH v2 0/2] usb: Don't use qerror_report
> >>>
> >>> From: Gonglei <arei.gonglei@huawei.com>
> >>>
> >>> qerror_report() is a transitional interface to help with converting
> >>> existing HMP commands to QMP. It should not be used elsewhere.
> >>>
> >>> v2 -> v1:
> >>>  - update including head files, remove qerror.h and monitor.h,
> >>>    add error-report.h (Markus)
> >>>  - add 'Reviewed-by' tag.
> >>>
> >>> Gonglei (2):
> >>>   redirect.c: Don't use qerror_report()
> >>>   dev-network: Don't use qerror_report_err()
> >>>
> >>>  hw/usb/dev-network.c | 4 ++--
> >>>  hw/usb/redirect.c    | 8 ++++----
> >>>  2 files changed, 6 insertions(+), 6 deletions(-)
> >>>
> >>> --
> >>> 1.7.12.4
> >>>
> >> Hi, Gerd
> >> Would you like to apply this patch series at present? Thanks!
> >
> > I'm not sure what the improvement is?
> 
> The improvement is obvious: three calls of qerror_report() gone.
> 
> The regression isn't as obvious (I missed it, but fortunately Paolo
> spotted it): when we run in QMP context, we screw up the error reply.
> 
> Therefore, we can't take these patches.  We really have to convert to
> realize to get rid of qerror_report() here.

OK, understand it. Thank you, guys :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gonglei (Arei)]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Wednesday, September 17, 2014 12:16 AM
> To: Gerd Hoffmann
> Cc: Gonglei (Arei); Huangweidong (C); qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report
> 
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >   Hi,
> >
> >> > Gonglei (2):
> >> >   redirect.c: Don't use qerror_report()
> >> >   dev-network: Don't use qerror_report_err()
> >
> >> Hi, Gerd
> >> Would you like to apply this patch series at present? Thanks!
> >
> > Picked it up now.  Originally dropped due to review comments from paolo,
> > but switching all usb over to realize is a bigger project indeed.
> 
> Please don't, it degrades QMP errors.  Example:
> 
>     { "execute": "device_add", "arguments": { "driver": "usb-redir" } }
> 
> Reply before:
> 
>     {"error": {"class": "GenericError", "desc": "Parameter 'chardev' is
> missing"}}
> 
> After:
> 
>     {"error": {"class": "GenericError", "desc": "Device initialization failed."}}
> 
> with "Parameter 'chardev' is missing" barfed to stderr.

Hum.. Thanks for your information :) 

Gerd, please drop this patch series, Thanks!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 0/2] usb: Don't use qerror_report

[Gonglei (Arei)]

> but switching all usb over to realize is a bigger project indeed.
> 

Hi, Gerd

I want to do this job which converting USB devices from init to realize.
So all the implementations can use error_setg instead of error_report
for reporting error.

Include those devices as below:

name "usb-host", bus usb-bus
name "usb-hub", bus usb-bus
name "usb-bot", bus usb-bus
name "usb-storage", bus usb-bus
name "usb-uas", bus usb-bus
name "usb-bt-dongle", bus usb-bus
name "usb-net", bus usb-bus
name "usb-braille", bus usb-bus
name "usb-ccid", bus usb-bus, desc "CCID Rev 1.1 smartcard reader"
name "usb-kbd", bus usb-bus
name "usb-mouse", bus usb-bus
name "usb-serial", bus usb-bus
name "usb-tablet", bus usb-bus
name "usb-wacom-tablet", bus usb-bus, desc "QEMU PenPartner Tablet"
name "usb-audio", bus usb-bus
name "usb-redir", bus usb-bus
name "usb-mtp", bus usb-bus

What's your opinion? Thanks!

Best regards,
-Gonglei