Re: [Qemu-devel] [PATCH v1 1/1] char-socket: Don't report TCP socket waiting as an error

[Alistair Francis]

On Thu, Jun 8, 2017 at 10:16 PM, Markus Armbruster <address@hidden> wrote:
> Alistair Francis <address@hidden> writes:
>
>> On Thu, Jun 8, 2017 at 10:56 AM, Markus Armbruster <address@hidden> wrote:
>>> Alistair Francis <address@hidden> writes:
>>>
>>>> On Wed, Jun 7, 2017 at 11:03 PM, Markus Armbruster <address@hidden> wrote:
>>>>> Alistair Francis <address@hidden> writes:
>>>>>
>>>>>> On Wed, Jun 7, 2017 at 12:19 AM, Markus Armbruster <address@hidden> 
>>>>>> wrote:
>>>>>>> Paolo Bonzini <address@hidden> writes:
>>>>>>>
>>>>>>>> On 06/06/2017 18:30, Alistair Francis wrote:
>>>>>>>>>>
>>>>>>>>>> This is somehow confusing. I don't think it is worth having another
>>>>>>>>>> qemu_log_stderr() function rather than using error_report() but this 
>>>>>>>>>> very
>>>>>>>>>> call might deserve a comment explaining this unusual use. What do 
>>>>>>>>>> you think?
>>>>>>>>>
>>>>>>>>> The problem with stderr is that this isn't an error. Some uses of QEMU
>>>>>>>>> (inside Eclipse for example) flag everything printed on stderr as red
>>>>>>>>> which confuses users that they are seeing an error when they really
>>>>>>>>> aren't.
>>>>>>>>
>>>>>>>> But they are wrong.
>>>>>>>
>>>>>>> Concur.  We also print warnings and informational messages to stderr.
>>>>>>>
>>>>>>> We should make errors easy to recognize.  Fortunately, error_report()
>>>>>>> prints errors to stderr in a rigid format.  Unfortunately, error
>>>>>>> messages bypassing error_report() still exist in places.  We suck.
>>>>>>>
>>>>>>> The format is
>>>>>>>
>>>>>>>     timestamp-if-enabled progname ':' location message
>>>>>>>
>>>>>>> timestamp-if-enabled is normally empty.  With -msg timestamp=on, it's
>>>>>>> the current time in ISO 8601 format, followed by a space.
>>>>>>>
>>>>>>> progname is the program name (main()'s argv[0]).
>>>>>>>
>>>>>>> location is either empty, or a reference to the command line or a
>>>>>>> configuration file.
>>>>>>>
>>>>>>> See error_vreport() for details.
>>>>>>
>>>>>> Ok, but this isn't an error, it's more information. So it sounds like
>>>>>> we should still print to stderr but not print in the format described
>>>>>> above?
>>>>>
>>>>> Yes.
>>>>>
>>>>> I explained the error message format to show how to distinguish actual
>>>>> errors from other stuff.
>>>>
>>>> Sorry, I should have been more clear. I meant we should not use the
>>>> error_report() function here. I don't think we have any
>>>> warning_report() function though, is that something worth having?
>>>
>>> So far we simply use error_printf() for such things.
>>>
>>> A function to report a warning would let us report them more uniformly,
>>> but only if we actually use it uniformly.  In other words, adding one
>>> without also converting the existing warnings to use it would create yet
>>> another open-ended incremental conversion job.  Are we up to it?
>>
>> Yeah! Why not. I am happy to give it a shot changing some errors to warnings.
>>
>> First thing though, what is the format for printing warnings?
>
> We make one up.
>
> For what it's worth, gcc uses the same format as for errors with the
> message prefixed either by "warning: " or by "error: ".  Also common is
> prefixing warnings, but not errors.

Ok, I think that is what I'm going to do. I will send a patch series
out soon that basically copies the error_report() format but prefixes
it with warning.

I also removed the timestamp for the warning, although I'm not sure if
that is what we want to do. I'll CC you when I send it out.

Thanks,
Alistair

>
> We already have several error_report() calls with messages that start
> with (a variation of) "warning: ".
>