| Message ID | 1360534332-6549-1-git-send-email-hpoussin@reactos.org |
|---|---|
| State | New |
| Headers | show |
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
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
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?
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
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
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
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
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
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
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é
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
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;
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> --- hw/ide/core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)