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

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

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/ide/core.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3743dc3..f0ab1a8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         break;
     case WIN_PACKETCMD:
         /* overlapping commands not supported */
-        if (s->feature & 0x02)
+        if (s->feature & 0x02) {
+            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");
             goto abort_cmd;
+        }
         s->status = READY_STAT | SEEK_STAT;
         s->atapi_dma = s->feature & 1;
         s->nsector = 1;
-- 
1.7.10.4

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

[Stefan Hajnoczi]

On Sun, Feb 10, 2013 at 11:12:11PM +0100, Hervé Poussineau wrote:
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/ide/core.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 3743dc3..f0ab1a8 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>          break;
>      case WIN_PACKETCMD:
>          /* overlapping commands not supported */
> -        if (s->feature & 0x02)
> +        if (s->feature & 0x02) {
> +            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");

qemu_log_*() isn't used much in hw/.

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
qemu_log_*() - goes to the qemu log, seems a little TCG-centric

Thoughts?

Stefan

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

[Kevin Wolf]

Am 11.02.2013 14:27, schrieb Stefan Hajnoczi:
> On Sun, Feb 10, 2013 at 11:12:11PM +0100, Hervé Poussineau wrote:
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>  hw/ide/core.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 3743dc3..f0ab1a8 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>          break;
>>      case WIN_PACKETCMD:
>>          /* overlapping commands not supported */
>> -        if (s->feature & 0x02)
>> +        if (s->feature & 0x02) {
>> +            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");
> 
> qemu_log_*() isn't used much in hw/.
> 
> 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.

> 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.)

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.02.2013 14:27, schrieb Stefan Hajnoczi:
>> On Sun, Feb 10, 2013 at 11:12:11PM +0100, Hervé Poussineau wrote:
>>>
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> ---
>>>  hw/ide/core.c |    4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 3743dc3..f0ab1a8 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>>          break;
>>>      case WIN_PACKETCMD:
>>>          /* overlapping commands not supported */
>>> -        if (s->feature & 0x02)
>>> +        if (s->feature & 0x02) {
>>> +            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");
>> 
>> qemu_log_*() isn't used much in hw/.
>> 
>> 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.

>> 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?

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

[Andreas Färber]

Am 11.02.2013 15:01, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 11.02.2013 14:27, schrieb Stefan Hajnoczi:
>>> On Sun, Feb 10, 2013 at 11:12:11PM +0100, Hervé Poussineau wrote:
>>>>
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>> ---
>>>>  hw/ide/core.c |    4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>>> index 3743dc3..f0ab1a8 100644
>>>> --- a/hw/ide/core.c
>>>> +++ b/hw/ide/core.c
>>>> @@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>>>          break;
>>>>      case WIN_PACKETCMD:
>>>>          /* overlapping commands not supported */
>>>> -        if (s->feature & 0x02)
>>>> +        if (s->feature & 0x02) {
>>>> +            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");
>>>
>>> qemu_log_*() isn't used much in hw/.
>>>
>>> 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.
> 
>>> 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(), which is
another reporting option people have chosen in the past (but which
allows a guest to exit the QEMU process).

Andreas

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

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

[Peter Maydell]

On 11 February 2013 14:19, Andreas Färber <afaerber@suse.de> wrote:
> Am 11.02.2013 15:01, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> 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.

Tracepoints are IMHO pretty useless for this kind of message
because they're a compile time setup, not runtime, and they don't
have useful message categories. We should be expanding the use
of the qemu_log() API. If we need more categories and/or more
flexibility about where the logging goes [eg an option to direct
the logs to monitor?] we can add them.

-- PMM

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

[Stefan Hajnoczi]

On Mon, Feb 11, 2013 at 3:34 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 February 2013 14:19, Andreas Färber <afaerber@suse.de> wrote:
>> Am 11.02.2013 15:01, schrieb Markus Armbruster:
>>> Kevin Wolf <kwolf@redhat.com> 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.

Stefan

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

[Peter Maydell]

On 11 February 2013 14:57, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Feb 11, 2013 at 3:34 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 11 February 2013 14:19, Andreas Färber <afaerber@suse.de> wrote:
>>> 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.

Feel free to send in a patch... Also, which errors did you want
to enable by default? At the moment none of the categories are
IMHO reasonable to default to enabled.

> 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.

It will never replace them unless we start accepting patches
like this that use qemu_log_mask() in the places where it is
appropriate.

-- PMM

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 <peter.maydell@linaro.org> wrote:
>> On 11 February 2013 14:19, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 11.02.2013 15:01, schrieb Markus Armbruster:
>>>> Kevin Wolf <kwolf@redhat.com> 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

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

[Stefan Hajnoczi]

On Mon, Feb 11, 2013 at 04:10:46PM +0100, Andreas Färber wrote:
> Am 11.02.2013 15:57, schrieb Stefan Hajnoczi:
> > On Mon, Feb 11, 2013 at 3:34 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On 11 February 2013 14:19, Andreas Färber <afaerber@suse.de> wrote:
> >>> Am 11.02.2013 15:01, schrieb Markus Armbruster:
> >>>> Kevin Wolf <kwolf@redhat.com> 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.

You are right.

Stefan

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

[Peter Maydell]

On 10 February 2013 22:12, Hervé Poussineau <hpoussin@reactos.org> wrote:
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>          break;
>      case WIN_PACKETCMD:
>          /* overlapping commands not supported */
> -        if (s->feature & 0x02)
> +        if (s->feature & 0x02) {
> +            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");

Messages for qemu_log_mask() need to supply the trailing \n.

Otherwise looks good.

-- PMM

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

[Hervé Poussineau]

Peter Maydell a écrit :
> On 10 February 2013 22:12, Hervé Poussineau <hpoussin@reactos.org> wrote:
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -1394,8 +1394,10 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
>>          break;
>>      case WIN_PACKETCMD:
>>          /* overlapping commands not supported */
>> -        if (s->feature & 0x02)
>> +        if (s->feature & 0x02) {
>> +            qemu_log_mask(LOG_UNIMP, "ide: overlapping command not supported");
> 
> Messages for qemu_log_mask() need to supply the trailing \n.

OK to add the '\n' in the v2 (if required), once I know if I can keep 
the qemu_log_mask call, or if I have to change it to hw_error/fprintf.

Regards,

Hervé