Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Markus Armbruster <armbru@redhat.com>]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/11/2010 10:57 AM, Markus Armbruster wrote:
>> Yes, that's a sensible argument.  It's also quite impractical at this
>> time.  In fact, I had the same idea, and dropped it like a hot potato
>> when I realized how much code we'd have to touch for it.
>>    
>
> Can you give me an example of where it gets particularly ugly?  It
> doesn't look so bad to me.

Here's one (very) partial call graph:

* qemu_socket() uses system call convention: non-negative value on
| success, -1 and errno set on error.  It doesn't print.
|
+-* unix_listen_opts() returns non-negative value on success, -1 on
| | error.  It trashes errno.  It reports errors to stderr.
. |
. +-* qemu_chr_open_socket() returns CharDriverState on success, null on
. | | error. It doesn't report errors.  It can print an informational
  | | message to stdout.
  | |
  | |<--- indirectly through backend_table[].open
  | |
  | +-* qemu_chr_open_opts() returns CharDriverState on success, null on
  |   | error.  It reports errors to stderr.
  |   |
  |   +-* qemu_chr_open() returns CharDriverState on success, null on
  |   | | error.  It doesn't report errors.
  |   | |
  |   | +-* gdbserver_start() ...
  |   | | ...
  |   | |
  |   | +-* serial_parse() ...
  |   | | ...
  |   | |
  |   | +-* parallel_parse() ...
  |   | | ...
  |   | |
  |   | +-* virtcon_parse() ...
  |   | | ...
  |   | |
  |   | +-* debugcon_parse() ...
  |   | | ...
  |   | |
  |   | +-* malta_fpga_init() ...
  |   | | ...
  |   | |
  |   | +-* mips_malta_init() ...
  |   | | ...
  |   | |
  |   | +-* omap_uart_init() ...
  |   | | ...
  |   | |
  |   | +-* omap_uart_attach() ...
  |   | | ...
  |   | |
  |   | +-* omap_sti_init() ...
  |   | | ...
  |   | |
  |   | +-* usb_serial_init() returns USBDevice on success, null on
  |   | | | error.  It reports errors via qemu_error().
  |   | | |
  |   | | |<--- indirectly through serial_info.usbdevice_init
  |   | | |
  |   | | +-* usbdevice_create() returns USBDevice on success, null on
  |   | |   | error.  It reports errors via qemu_error().
  |   | |   |
  |   | |   +-* usb_device_add() returns 0 on success, -1 on error.  It
  |   | |     | doesn't report errors.
  |   | |     |
  |   | |     +-* usb_parse() returns 0 on success, -1 on error.  Ir
  |   | |     | | reports errors to stderr.
  |   | |     | |
  |   | |     | +-* main() calls it for -usbdevice (legacy), and wants
  |   | |     |     it to report errors to stderr. 
  |   | |     |
  |   | |     +-* do_usb_add() wants it to print errors to the monitor.
  |   | |         This handler will never be converted, because
  |   | |         device_add is more general.
  |   | |
  |   | +-* usb_braille_init() ...
  |   | | ...
  |   | |
  |   | +-* slirp_guestfwd() returns 0 on success, -1 on error.  It
  |   |   | reports errors via qemu_error().
  |   |   |
  |   |   +-* net_slirp_init() returns 0 on success, -1 on error.  It
  |   |   | | doesn't report errors.
  |   |   | |
  |   |   | +-* net_init_slirp()
  |   |   |   |
  |   |   |   |<-- indirectly through net_client_types[].init()
  |   |   |   |
  |   |   |   +-* net_client_init() returns 0 on success, -1 on error.
  |   |   |     | It reports errors via qemu_error().
  |   |   |     |
  |   |   |     +-* net_host_device_add() ...
  |   |   |     | ...
  |   |   |     |
  |   |   |     +-* net_init_client() ...
  |   |   |     | ...
  |   |   |     |
  |   |   |     +-* net_init_netdev() ...
  |   |   |     | ...
  |   |   |     |
  |   |   |     +-* qemu_pci_hot_add_nic() returns PCIDevice on success,
  |   |   |     | | null on error.  It prints errors to the monitor.
  |   |   |     | |
  |   |   |     | +-* pci_device_hot_add() wants it to use QError.
  |   |   |     |
  |   |   |     +-* usb_net_init() ...
  |   |   |       ...
  |   |   |
  |   |   +-* net_slirp_parse_legacy()
  |   |     |
  |   |     +-* net_client_parse()
  |   |       |
  |   |       +- main() calls it for -netdev and -net, and wants it to
  |   |          report errors to stderr.
  |   |
  |   +-* chardev_init_func() returns 0 on success, -1 on error.  It
  |     | doesn't report errors.
  |     |
  |     +-* main() calls it for -chardev, and wants it to report errors
  |         to stderr.
  |
  +-* unix_listen() returns non-negative value on success, -1 on
    | error.  It doesn't report errors.
    |
    +-* vnc_display_open() returns 0 on success, -1 on error.  It
      | reports errors to stderr.
      |
      +-* do_change_vnc() returns 0 on success, -1 on error.  It uses
      | | QError.
      | |
      | +-* do_change() wants it to use QError.
      |
      +-* main() calls it, and wants it to report errors to stderr.

qemu_socket() is just a system call wrapper, so let's ignore that.  The
error gets diagnosed in unix_listen_opts().  Its caller already doesn't
know (and doesn't care about) the exact error.

There are paths from unix_listen_opts() to main(), an unconverted
monitor handler that will never be converted, converted monitor
handlers, and whatever else is hiding in my dot-dot-dots.  main() wants
errors printed to stderr, unconverted handlers want them printed to the
monitor, and converted handlers want a QError object.

This is e-mail, so you get to do this for yourself: color the parts of
the graph that run on behalf of main()'s argument processing only, human
monitor only, QMP or human monitor only, all of them.  You'll see that
substantial parts have nothing to do with the monitor, and thus precious
little use for QError.

To create a QError more specific than "didn't work", you either have to
create it in unix_listen_opts(), or change it to return some other
richer error information to its callers.  Either way, you got churn
along the whole path up to the converted monitor handler.

But you have several monitor handlers, hence several paths.  You'll end
up with a big patch hairball, and get to figure out a smart way to split
it.

Sorry, I just can't do that now.


PS: The above is one of the reasons why I argued for much simpler error
reporting.  It would have let us get away with minimal changes to
qemu_error() calls, and printing to monitor or stderr would have been
trivial to convert to qemu_error().  I understand why you wanted QError,
but it makes it so much harder to get QMP into a useful state, and
before it reaches that state, it's worse than nothing: it's a drain on
precious resources, and a source of new bugs.  That's why I'm more
convinced than ever that doing QError *now* was a mistake.  But I lost
that debate, and I don't wish to reopen it.