Re: [Qemu-devel] [PATCH v2 40/47] hw/usb: Replace fprintf(stderr, "*\n" with error_report()

[Thomas Huth]

On 01.10.2017 03:36, Philippe Mathieu-Daudé wrote:
> On 09/29/2017 09:17 PM, Alistair Francis wrote:
>> Replace a large number of the fprintf(stderr, "*\n" calls with
>> error_report(). The functions were renamed with these commands and then
>> compiler issues where manually fixed.
[...]
>> diff --git a/hw/usb/desc.c b/hw/usb/desc.c
>> index 85c15addc5..afae910f8e 100644
>> --- a/hw/usb/desc.c
>> +++ b/hw/usb/desc.c
>> @@ -1,5 +1,5 @@
>>   #include "qemu/osdep.h"
>> -
>> +#include "qemu/error-report.h"
>>   #include "hw/usb.h"
>>   #include "hw/usb/desc.h"
>>   #include "trace.h"
>> @@ -688,7 +688,7 @@ int usb_desc_get_descriptor(USBDevice *dev,
>> USBPacket *p,
>>           break;
>>         default:
>> -        fprintf(stderr, "%s: %d unknown type %d (len %zd)\n", __func__,
>> +        error_report("%s: %d unknown type %d (len %zd)", __func__,
>>                   dev->addr, type, len);

Bad indentation.

>>           break;
>>       }
>> diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c
>> index 343345235c..43fc20469a 100644
>> --- a/hw/usb/dev-audio.c
>> +++ b/hw/usb/dev-audio.c
>> @@ -30,6 +30,7 @@
>>    */
>>     #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "qemu-common.h"
>>   #include "hw/usb.h"
>>   #include "hw/usb/desc.h"
>> @@ -398,7 +399,7 @@ static int
>> usb_audio_set_output_altset(USBAudioState *s, int altset)
>>       }
>>         if (s->debug) {
>> -        fprintf(stderr, "usb-audio: set interface %d\n", altset);
>> +        error_report("usb-audio: set interface %d", altset);
>>       }
>>       s->out.altset = altset;
>>       return 0;
>> @@ -478,7 +479,7 @@ static int usb_audio_set_control(USBAudioState *s,
>> uint8_t attrib,
>>               uint16_t vol = data[0] + (data[1] << 8);
>>                 if (s->debug) {
>> -                fprintf(stderr, "usb-audio: vol %04x\n", (uint16_t)vol);
>> +                error_report("usb-audio: vol %04x", (uint16_t)vol);
>>               }
>>                 vol -= 0x8000;
>> @@ -496,7 +497,7 @@ static int usb_audio_set_control(USBAudioState *s,
>> uint8_t attrib,
>>         if (set_vol) {
>>           if (s->debug) {
>> -            fprintf(stderr, "usb-audio: mute %d, lvol %3d, rvol %3d\n",
>> +            error_report("usb-audio: mute %d, lvol %3d, rvol %3d",
>>                       s->out.mute, s->out.vol[0], s->out.vol[1]);

Bad indentation.

>>           }
>>           AUD_set_volume_out(s->out.voice, s->out.mute,
>> @@ -514,8 +515,8 @@ static void usb_audio_handle_control(USBDevice
>> *dev, USBPacket *p,
>>       int ret = 0;
>>         if (s->debug) {
>> -        fprintf(stderr, "usb-audio: control transaction: "
>> -                "request 0x%04x value 0x%04x index 0x%04x length
>> 0x%04x\n",
>> +        error_report("usb-audio: control transaction: "
>> +                "request 0x%04x value 0x%04x index 0x%04x length
>> 0x%04x",
>>                   request, value, index, length);

Bad indentation.

>>       }
>>   @@ -533,7 +534,7 @@ static void usb_audio_handle_control(USBDevice
>> *dev, USBPacket *p,
>>                                       length, data);
>>           if (ret < 0) {
>>               if (s->debug) {
>> -                fprintf(stderr, "usb-audio: fail: get control\n");
>> +                error_report("usb-audio: fail: get control");
>>               }
>>               goto fail;
>>           }
>> @@ -548,7 +549,7 @@ static void usb_audio_handle_control(USBDevice
>> *dev, USBPacket *p,
>>                                       length, data);
>>           if (ret < 0) {
>>               if (s->debug) {
>> -                fprintf(stderr, "usb-audio: fail: set control\n");
>> +                error_report("usb-audio: fail: set control");
>>               }
>>               goto fail;
>>           }
>> @@ -557,8 +558,8 @@ static void usb_audio_handle_control(USBDevice
>> *dev, USBPacket *p,
>>       default:
>>   fail:
>>           if (s->debug) {
>> -            fprintf(stderr, "usb-audio: failed control transaction: "
>> -                    "request 0x%04x value 0x%04x index 0x%04x length
>> 0x%04x\n",
>> +            error_report("usb-audio: failed control transaction: "
>> +                    "request 0x%04x value 0x%04x index 0x%04x length
>> 0x%04x",
>>                       request, value, index, length);

Bad indentation.

>>           }
>>           p->status = USB_RET_STALL;
>> @@ -581,7 +582,7 @@ static void usb_audio_handle_reset(USBDevice *dev)
>>       USBAudioState *s = USB_AUDIO(dev);
>>         if (s->debug) {
>> -        fprintf(stderr, "usb-audio: reset\n");
>> +        error_report("usb-audio: reset");
>>       }
>>       usb_audio_set_output_altset(s, ALTSET_OFF);
>>   }
>> @@ -595,7 +596,7 @@ static void usb_audio_handle_dataout(USBAudioState
>> *s, USBPacket *p)
>>         streambuf_put(&s->out.buf, p);
>>       if (p->actual_length < p->iov.size && s->debug > 1) {
>> -        fprintf(stderr, "usb-audio: output overrun (%zd bytes)\n",
>> +        error_report("usb-audio: output overrun (%zd bytes)",
>>                   p->iov.size - p->actual_length);

Bad indentation.

>>       }
>>   }
>> @@ -611,8 +612,8 @@ static void usb_audio_handle_data(USBDevice *dev,
>> USBPacket *p)
>>         p->status = USB_RET_STALL;
>>       if (s->debug) {
>> -        fprintf(stderr, "usb-audio: failed data transaction: "
>> -                        "pid 0x%x ep 0x%x len 0x%zx\n",
>> +        error_report("usb-audio: failed data transaction: "
>> +                        "pid 0x%x ep 0x%x len 0x%zx",
>>                           p->pid, p->ep->nr, p->iov.size);

Bad indentation.

>>       }
>>   }
[...]
>> @@ -1348,7 +1349,7 @@ static void usb_mtp_handle_control(USBDevice
>> *dev, USBPacket *p,
>>   static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)
>>   {
>>       /* we don't use async packets, so this should never be called */
>> -    fprintf(stderr, "%s\n", __func__);
>> +    error_report("%s", __func__);
> 
> what about using tracepoint instead? this report is probably confusing
> instead of being useful for the user imho.

A comment like "should never be called" rather sounds like we should be
using g_assert_not_reached() here instead.

 Thomas