Anthony Liguori <anth...@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.