Re: [Qemu-trivial] [Qemu-devel] [PATCH] ide: log error when trying to use ATAPI overlapping features

[Andreas Färber]

Am 11.02.2013 15:57, schrieb Stefan Hajnoczi:
> On Mon, Feb 11, 2013 at 3:34 PM, Peter Maydell <address@hidden> wrote:
>> On 11 February 2013 14:19, Andreas Färber <address@hidden> wrote:
>>> Am 11.02.2013 15:01, schrieb Markus Armbruster:
>>>> Kevin Wolf <address@hidden> writes:
>>>>
>>>>> Am 11.02.2013 14:27, schrieb Stefan Hajnoczi:
>>>>>> I think we need to side-track this patch email to figure out what to
>>>>>> use:
>>>>>>
>>>>>> fprintf(stderr) - some warnings/errors use this
>>>>>> error_report() - goes to the monitor, if possible, otherwise stderr
>>>>>
>>>>> These look wrong to me.
>>>>
>>>> "Wrong" is a bit strong, in particular since there's ample precedence
>>>> for these uses.
>>
>> Certainly for fprintf() I would say "deprecated" wherever
>> we have a better API available.
>>
>>>>>> qemu_log_*() - goes to the qemu log, seems a little TCG-centric
>>>>>
>>>>> I would suggest either this or just trace points. (And by the way, it's
>>>>> a pity that -d is so TCG-centric, it's been more than once the reason
>>>>> why I disabled KVM when debugging a guest... Having at least -d int
>>>>> would be so useful.)
>>>>
>>>> Tracepoints don't really fit when we want to report the guest does
>>>> something we don't handle.  Users deserve fair warning then, don't they?
>>>>
>>>> Could qemu_log() & friends be made fit for general use?  What's missing?
>>>
>>> Blue already did some work to make it more usable, and I believe Peter
>>> adopted LOG_UNIMPL for ARM devices in place of hw_error()
>>
>> Yes; in particular where we have classes of error message which the
>> user may wish to enable or disable (of which "QEMU doesn't implement
>> this" and "the guest just did something that's probably a guest bug"
>> are two common ones) qemu_log_mask(LOG_*, ...) is the preferred
>> API for devices IMHO. So I think Herve's patch is entirely the
>> right thing.
> 
> qemu_log_mask() can replace fprintf() but it needs to default to
> stderr and a reasonable default mask.  It should be used for
> non-monitor command errors and warnings.
> 
> Having said that, I think we should use hw_error() or fprintf() in
> this patch until qemu_log_mask() replaces them.

Nack. Like I just pointed out, hw_error() is a fatal abort and not a
replacement for logging. I don't mind which of the non-fatal logging
methods you choose, but guest-triggerable aborting is a semantic change
that we should not choose just because someone doesn't like logging API
or defaults. Especially not for 1.4.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg