Re: [Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp

[Cole Robinson <crobinso@redhat.com>]

On 03/12/2014 04:56 AM, Markus Armbruster wrote:
> Cole Robinson <crobinso@redhat.com> writes:
> 
>> error_printf is just a wrapper around monitor_printf, which already
>> drops the requested output if cur_mon is qmp.
> 
> Since commit 74ee59a:
> 
>     monitor: drop unused monitor debug code
>     
>     In the old QMP days, this code was used to find out QMP commands that
>     might be calling monitor_printf() down its call chain.
>     
>     This is almost impossible to happen today, because the qapi converted
>     commands don't even have a monitor object. Besides, it's been more than
>     a year since I used this last time.
>     
>     Let's just drop it.
>     
>     Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>     Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> I gave my R-by then, but I'm no longer sure it was a sensible move.
> Attempting to print in QMP context is as much a sign of trouble as it
> ever was.  I hate sweeping signs of trouble under the carpet.  If misuse
> is really "almost impossible", then we should assert() it is, and fix
> the bugs caught by it.
> 
> I can see error_printf() / error_printf_unless_qmp() used in a couple of
> ways:
> 
> 1. Print hints after qerror_report(), with error_printf_unless_qmp()
> 
>    qerror_report() is a transitional interface to help with converting
>    existing HMP commands to QMP.  It should not be used elsewhere.
> 
>    We suppress the hints in QMP, because the QMP wire format doesn't
>    provide for hints.
> 
>    We can't add hints to an error when using error_set(), because that
>    API lacks support for hints.  Pity.
> 
>    Examples: see your patch below.
> 
> 2. Print hints after error_report(), with error_printf()
> 
>    Use of error_report() in QMP context is a sign of trouble just like
>    any other non-JSON output to the monitor.
> 
>    error_printf() rather than error_printf_unless_qmp(), because the
>    latter explicitly signals intent "skip this in QMP", while the former
>    signals "QMP should not happen".
> 
>    The difference in intent is what makes me wary of this patch.
> 
>    Example: vfio_pci_load_rom().
> 
> 3. Ordinary output in code shared between command line and HMP, with
>    error_printf()
> 
>    error_printf() was pressed into use as convenient "print to monitor
>    in HMP context, else to tty" function.  Inappropriate, because it
>    prints to stderr rather than stdout.
> 
>    Examples: many help texts under is_help_option().
> 
> 4. Warnings, with error_printf()
> 
>    I figure these should use error_report() instead.
> 
>    Examples: block/ssh.c, hw/misc/vfio.c, ...

Thanks, I didn't think about any of that. I'll drop this patch

- Cole