[Qemu-devel] [RFC 0/5] Monitor handlers convertion to QObject

[Qemu-devel] [RFC 0/5] Monitor handlers convertion to QObject

[Luiz Capitulino]

Hi There,

 This is a RFC for the next phase of the QEMU Monitor Protocol project,
in this phase handlers will get fully "structurized".

 I was waiting for the first phase to be merged before sending this, but
as it's already in staging and as this is just a RFC, I've decided to send.

 This series adds the infrastructure needed to accommodate the new style
handlers and ports do_info_balloon() and do_balloon() to QObject style.

 Please, note that I've chosen to stay simple. I already have patches for
QList and advanced stuff (like the 'ninja action' suggested by Anthony) but
for now let's concentrate on design decisions that will impact the protocol
format.

 Besides the 'user_print' approach, it's very important to discuss the
following:

1. Return data
--------------

 For do_info_balloon() I'm always returning a QString. The 'actual' info
could be something else (QDict?), but I think QString is good, because it's
simple.

 Choosing the best set of data types to return is an issue and I don't
think we can have a general rule for this, the solution will be to review
*each* command handler port and be sure we are doing the right thing.

 I hope people don't get bored in reviewing.

2. Return code
--------------

 All handlers will have a return code. For this series they return 0
for success and -1 for error.

 My current plan is that the procotol will *always* emit something like:

 { "__result__": 0, "__data__": { ... } }

 Where the standard keys 'result' and 'data' will be pushed by common
code (monitor_handle_command(), in this case).

 Some people have suggested us to define errors codes, but I dislike this
because I have the impression we will end up defining errors per-handlers,
which is a lot of work and more protocol definitions to maintain.

 Another issue in this subject is that sometimes we will have to do
a not so small refactor to get the proper error code. I mean, sometimes
the functions which the handlers call return void, and those functions in
turn call other functions which also return void. This seem to be the
case of do_balloon(), for example.

 A possible solution is to only return error when it's easy, otherwise we
could always return 0 (which is what we have today), the problem though
is that changing this in the future will cause trouble.

 Suggestions?

[Qemu-devel] [PATCH 1/5] monitor: Add user_print() to mon_cmd_t

[Luiz Capitulino]

This new struct member will store a pointer to a function that
should be used to output data in the user protocol format.

Additionally, it will also serve as a flag to say if a given
handler has already been ported to support the machine protocol.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   69 +++++++++++++++---------------
 qemu-monitor.hx |  124 ++++++++++++++++++++++++++++--------------------------
 2 files changed, 99 insertions(+), 94 deletions(-)

diff --git a/monitor.c b/monitor.c
index a242b4b..b43a287 100644
--- a/monitor.c
+++ b/monitor.c
@@ -71,6 +71,7 @@ typedef struct mon_cmd_t {
     const char *name;
     const char *args_type;
     void *handler;
+    void (*user_print)(Monitor *mon, const QObject *data);
     const char *params;
     const char *help;
 } mon_cmd_t;
@@ -1807,80 +1808,80 @@ static const mon_cmd_t mon_cmds[] = {
 
 /* Please update qemu-monitor.hx when adding or changing commands */
 static const mon_cmd_t info_cmds[] = {
-    { "version", "", do_info_version,
+    { "version", "", do_info_version, NULL,
       "", "show the version of QEMU" },
-    { "network", "", do_info_network,
+    { "network", "", do_info_network, NULL,
       "", "show the network state" },
-    { "chardev", "", qemu_chr_info,
+    { "chardev", "", qemu_chr_info, NULL,
       "", "show the character devices" },
-    { "block", "", bdrv_info,
+    { "block", "", bdrv_info, NULL,
       "", "show the block devices" },
-    { "blockstats", "", bdrv_info_stats,
+    { "blockstats", "", bdrv_info_stats, NULL,
       "", "show block device statistics" },
-    { "registers", "", do_info_registers,
+    { "registers", "", do_info_registers, NULL,
       "", "show the cpu registers" },
-    { "cpus", "", do_info_cpus,
+    { "cpus", "", do_info_cpus, NULL,
       "", "show infos for each CPU" },
-    { "history", "", do_info_history,
+    { "history", "", do_info_history, NULL,
       "", "show the command line history", },
-    { "irq", "", irq_info,
+    { "irq", "", irq_info, NULL,
       "", "show the interrupts statistics (if available)", },
-    { "pic", "", pic_info,
+    { "pic", "", pic_info, NULL,
       "", "show i8259 (PIC) state", },
-    { "pci", "", pci_info,
+    { "pci", "", pci_info, NULL,
       "", "show PCI info", },
 #if defined(TARGET_I386) || defined(TARGET_SH4)
-    { "tlb", "", tlb_info,
+    { "tlb", "", tlb_info, NULL,
       "", "show virtual to physical memory mappings", },
 #endif
 #if defined(TARGET_I386)
-    { "mem", "", mem_info,
+    { "mem", "", mem_info, NULL,
       "", "show the active virtual memory mappings", },
-    { "hpet", "", do_info_hpet,
+    { "hpet", "", do_info_hpet, NULL,
       "", "show state of HPET", },
 #endif
-    { "jit", "", do_info_jit,
+    { "jit", "", do_info_jit, NULL,
       "", "show dynamic compiler info", },
-    { "kvm", "", do_info_kvm,
+    { "kvm", "", do_info_kvm, NULL,
       "", "show KVM information", },
-    { "numa", "", do_info_numa,
+    { "numa", "", do_info_numa, NULL,
       "", "show NUMA information", },
-    { "usb", "", usb_info,
+    { "usb", "", usb_info, NULL,
       "", "show guest USB devices", },
-    { "usbhost", "", usb_host_info,
+    { "usbhost", "", usb_host_info, NULL,
       "", "show host USB devices", },
-    { "profile", "", do_info_profile,
+    { "profile", "", do_info_profile, NULL,
       "", "show profiling information", },
-    { "capture", "", do_info_capture,
+    { "capture", "", do_info_capture, NULL,
       "", "show capture information" },
-    { "snapshots", "", do_info_snapshots,
+    { "snapshots", "", do_info_snapshots, NULL,
       "", "show the currently saved VM snapshots" },
-    { "status", "", do_info_status,
+    { "status", "", do_info_status, NULL,
       "", "show the current VM status (running|paused)" },
-    { "pcmcia", "", pcmcia_info,
+    { "pcmcia", "", pcmcia_info, NULL,
       "", "show guest PCMCIA status" },
-    { "mice", "", do_info_mice,
+    { "mice", "", do_info_mice, NULL,
       "", "show which guest mouse is receiving events" },
-    { "vnc", "", do_info_vnc,
+    { "vnc", "", do_info_vnc, NULL,
       "", "show the vnc server status"},
-    { "name", "", do_info_name,
+    { "name", "", do_info_name, NULL,
       "", "show the current VM name" },
-    { "uuid", "", do_info_uuid,
+    { "uuid", "", do_info_uuid, NULL,
       "", "show the current VM UUID" },
 #if defined(TARGET_PPC)
-    { "cpustats", "", do_info_cpu_stats,
+    { "cpustats", "", do_info_cpu_stats, NULL,
       "", "show CPU statistics", },
 #endif
 #if defined(CONFIG_SLIRP)
-    { "usernet", "", do_info_usernet,
+    { "usernet", "", do_info_usernet, NULL,
       "", "show user network stack connection states", },
 #endif
-    { "migrate", "", do_info_migrate, "", "show migration status" },
-    { "balloon", "", do_info_balloon,
+    { "migrate", "", do_info_migrate, NULL, "", "show migration status" },
+    { "balloon", "", do_info_balloon, NULL,
       "", "show balloon information" },
-    { "qtree", "", do_info_qtree,
+    { "qtree", "", do_info_qtree, NULL,
       "", "show device tree" },
-    { "qdm", "", do_info_qdm,
+    { "qdm", "", do_info_qdm, NULL,
       "", "show qdev device model list" },
     { NULL, NULL, },
 };
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9f91873..a6d0f2e 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -9,20 +9,20 @@ STEXI
 @table @option
 ETEXI
 
-    { "help|?", "name:s?", do_help_cmd, "[cmd]", "show the help" },
+    { "help|?", "name:s?", do_help_cmd, NULL, "[cmd]", "show the help" },
 STEXI
 @item help or ? [@var{cmd}]
 Show the help for all commands or just for command @var{cmd}.
 ETEXI
 
-    { "commit", "device:s", do_commit,
+    { "commit", "device:s", do_commit, NULL,
       "device|all", "commit changes to the disk images (if -snapshot is used) or backing files" },
 STEXI
 @item commit
 Commit changes to the disk images (if -snapshot is used) or backing files.
 ETEXI
 
-    { "info", "item:s?", do_info,
+    { "info", "item:s?", do_info, NULL,
       "[subcommand]", "show various information about the system state" },
 STEXI
 @item info @var{subcommand}
@@ -94,21 +94,21 @@ show device tree
 @end table
 ETEXI
 
-    { "q|quit", "", do_quit,
+    { "q|quit", "", do_quit, NULL,
       "", "quit the emulator" },
 STEXI
 @item q or quit
 Quit the emulator.
 ETEXI
 
-    { "eject", "force:-f,filename:B", do_eject,
+    { "eject", "force:-f,filename:B", do_eject, NULL,
       "[-f] device", "eject a removable medium (use -f to force it)" },
 STEXI
 @item eject [-f] @var{device}
 Eject a removable medium (use -f to force it).
 ETEXI
 
-    { "change", "device:B,target:F,arg:s?", do_change,
+    { "change", "device:B,target:F,arg:s?", do_change, NULL,
       "device filename [format]", "change a removable medium, optional format" },
 STEXI
 @item change @var{device} @var{setting}
@@ -147,28 +147,28 @@ Password: ********
 @end table
 ETEXI
 
-    { "screendump", "filename:F", do_screen_dump,
+    { "screendump", "filename:F", do_screen_dump, NULL,
       "filename", "save screen into PPM image 'filename'" },
 STEXI
 @item screendump @var{filename}
 Save screen into PPM image @var{filename}.
 ETEXI
 
-    { "logfile", "filename:F", do_logfile,
+    { "logfile", "filename:F", do_logfile, NULL,
       "filename", "output logs to 'filename'" },
 STEXI
 @item logfile @var{filename}
 Output logs to @var{filename}.
 ETEXI
 
-    { "log", "items:s", do_log,
+    { "log", "items:s", do_log, NULL,
       "item1[,...]", "activate logging of the specified items to '/tmp/qemu.log'" },
 STEXI
 @item log @var{item1}[,...]
 Activate logging of the specified items to @file{/tmp/qemu.log}.
 ETEXI
 
-    { "savevm", "name:s?", do_savevm,
+    { "savevm", "name:s?", do_savevm, NULL,
       "[tag|id]", "save a VM snapshot. If no tag or id are provided, a new snapshot is created" },
 STEXI
 @item savevm [@var{tag}|@var{id}]
@@ -178,7 +178,7 @@ a snapshot with the same tag or ID, it is replaced. More info at
 @ref{vm_snapshots}.
 ETEXI
 
-    { "loadvm", "name:s", do_loadvm,
+    { "loadvm", "name:s", do_loadvm, NULL,
       "tag|id", "restore a VM snapshot from its tag or id" },
 STEXI
 @item loadvm @var{tag}|@var{id}
@@ -186,14 +186,14 @@ Set the whole virtual machine to the snapshot identified by the tag
 @var{tag} or the unique snapshot ID @var{id}.
 ETEXI
 
-    { "delvm", "name:s", do_delvm,
+    { "delvm", "name:s", do_delvm, NULL,
       "tag|id", "delete a VM snapshot from its tag or id" },
 STEXI
 @item delvm @var{tag}|@var{id}
 Delete the snapshot identified by @var{tag} or @var{id}.
 ETEXI
 
-    { "singlestep", "option:s?", do_singlestep,
+    { "singlestep", "option:s?", do_singlestep, NULL,
       "[on|off]", "run emulation in singlestep mode or switch to normal mode", },
 STEXI
 @item singlestep [off]
@@ -201,35 +201,35 @@ Run the emulation in single step mode.
 If called with option off, the emulation returns to normal mode.
 ETEXI
 
-    { "stop", "", do_stop,
+    { "stop", "", do_stop, NULL,
       "", "stop emulation", },
 STEXI
 @item stop
 Stop emulation.
 ETEXI
 
-    { "c|cont", "", do_cont,
+    { "c|cont", "", do_cont, NULL,
       "", "resume emulation", },
 STEXI
 @item c or cont
 Resume emulation.
 ETEXI
 
-    { "gdbserver", "device:s?", do_gdbserver,
+    { "gdbserver", "device:s?", do_gdbserver, NULL,
       "[device]", "start gdbserver on given device (default 'tcp::1234'), stop with 'none'", },
 STEXI
 @item gdbserver [@var{port}]
 Start gdbserver session (default @var{port}=1234)
 ETEXI
 
-    { "x", "fmt:/,addr:l", do_memory_dump,
+    { "x", "fmt:/,addr:l", do_memory_dump, NULL,
       "/fmt addr", "virtual memory dump starting at 'addr'", },
 STEXI
 @item x/fmt @var{addr}
 Virtual memory dump starting at @var{addr}.
 ETEXI
 
-    { "xp", "fmt:/,addr:l", do_physical_memory_dump,
+    { "xp", "fmt:/,addr:l", do_physical_memory_dump, NULL,
       "/fmt addr", "physical memory dump starting at 'addr'", },
 STEXI
 @item xp /@var{fmt} @var{addr}
@@ -289,7 +289,7 @@ Dump 80 16 bit values at the start of the video memory.
 @end itemize
 ETEXI
 
-    { "p|print", "fmt:/,val:l", do_print,
+    { "p|print", "fmt:/,val:l", do_print, NULL,
       "/fmt expr", "print expression value (use $reg for CPU register access)", },
 STEXI
 @item p or print/@var{fmt} @var{expr}
@@ -298,19 +298,19 @@ Print expression value. Only the @var{format} part of @var{fmt} is
 used.
 ETEXI
 
-    { "i", "fmt:/,addr:i,index:i.", do_ioport_read,
+    { "i", "fmt:/,addr:i,index:i.", do_ioport_read, NULL,
       "/fmt addr", "I/O port read" },
 STEXI
 Read I/O port.
 ETEXI
 
-    { "o", "fmt:/,addr:i,val:i", do_ioport_write,
+    { "o", "fmt:/,addr:i,val:i", do_ioport_write, NULL,
       "/fmt addr value", "I/O port write" },
 STEXI
 Write to I/O port.
 ETEXI
 
-    { "sendkey", "string:s,hold_time:i?", do_sendkey,
+    { "sendkey", "string:s,hold_time:i?", do_sendkey, NULL,
       "keys [hold_ms]", "send keys to the VM (e.g. 'sendkey ctrl-alt-f1', default hold time=100 ms)" },
 STEXI
 @item sendkey @var{keys}
@@ -326,7 +326,7 @@ This command is useful to send keys that your graphical user interface
 intercepts at low level, such as @code{ctrl-alt-f1} in X Window.
 ETEXI
 
-    { "system_reset", "", do_system_reset,
+    { "system_reset", "", do_system_reset, NULL,
       "", "reset the system" },
 STEXI
 @item system_reset
@@ -334,7 +334,7 @@ STEXI
 Reset the system.
 ETEXI
 
-    { "system_powerdown", "", do_system_powerdown,
+    { "system_powerdown", "", do_system_powerdown, NULL,
       "", "send system power down event" },
 STEXI
 @item system_powerdown
@@ -342,7 +342,7 @@ STEXI
 Power down the system (if supported).
 ETEXI
 
-    { "sum", "start:i,size:i", do_sum,
+    { "sum", "start:i,size:i", do_sum, NULL,
       "addr size", "compute the checksum of a memory region" },
 STEXI
 @item sum @var{addr} @var{size}
@@ -350,7 +350,7 @@ STEXI
 Compute the checksum of a memory region.
 ETEXI
 
-    { "usb_add", "devname:s", do_usb_add,
+    { "usb_add", "devname:s", do_usb_add, NULL,
       "device", "add USB device (e.g. 'host:bus.addr' or 'host:vendor_id:product_id')" },
 STEXI
 @item usb_add @var{devname}
@@ -359,7 +359,7 @@ Add the USB device @var{devname}.  For details of available devices see
 @ref{usb_devices}
 ETEXI
 
-    { "usb_del", "devname:s", do_usb_del,
+    { "usb_del", "devname:s", do_usb_del, NULL,
       "device", "remove USB device 'bus.addr'" },
 STEXI
 @item usb_del @var{devname}
@@ -369,13 +369,13 @@ hub. @var{devname} has the syntax @code{bus.addr}. Use the monitor
 command @code{info usb} to see the devices you can remove.
 ETEXI
 
-    { "cpu", "index:i", do_cpu_set,
+    { "cpu", "index:i", do_cpu_set, NULL,
       "index", "set the default CPU" },
 STEXI
 Set the default CPU.
 ETEXI
 
-    { "mouse_move", "dx_str:s,dy_str:s,dz_str:s?", do_mouse_move,
+    { "mouse_move", "dx_str:s,dy_str:s,dz_str:s?", do_mouse_move, NULL,
       "dx dy [dz]", "send mouse move events" },
 STEXI
 @item mouse_move @var{dx} @var{dy} [@var{dz}]
@@ -383,14 +383,14 @@ Move the active mouse to the specified coordinates @var{dx} @var{dy}
 with optional scroll axis @var{dz}.
 ETEXI
 
-    { "mouse_button", "button_state:i", do_mouse_button,
+    { "mouse_button", "button_state:i", do_mouse_button, NULL,
       "state", "change mouse button state (1=L, 2=M, 4=R)" },
 STEXI
 @item mouse_button @var{val}
 Change the active mouse button state @var{val} (1=L, 2=M, 4=R).
 ETEXI
 
-    { "mouse_set", "index:i", do_mouse_set,
+    { "mouse_set", "index:i", do_mouse_set, NULL,
       "index", "set which mouse device receives events" },
 STEXI
 @item mouse_set @var{index}
@@ -403,7 +403,7 @@ ETEXI
 
 #ifdef HAS_AUDIO
     { "wavcapture", "path:s,freq:i?,bits:i?,nchannels:i?", do_wav_capture,
-      "path [frequency [bits [channels]]]",
+      NULL, "path [frequency [bits [channels]]]",
       "capture audio to a wave file (default frequency=44100 bits=16 channels=2)" },
 #endif
 STEXI
@@ -420,7 +420,7 @@ Defaults:
 ETEXI
 
 #ifdef HAS_AUDIO
-    { "stopcapture", "n:i", do_stop_capture,
+    { "stopcapture", "n:i", do_stop_capture, NULL,
       "capture index", "stop capture" },
 #endif
 STEXI
@@ -431,21 +431,21 @@ info capture
 @end example
 ETEXI
 
-    { "memsave", "val:l,size:i,filename:s", do_memory_save,
+    { "memsave", "val:l,size:i,filename:s", do_memory_save, NULL,
       "addr size file", "save to disk virtual memory dump starting at 'addr' of size 'size'", },
 STEXI
 @item memsave @var{addr} @var{size} @var{file}
 save to disk virtual memory dump starting at @var{addr} of size @var{size}.
 ETEXI
 
-    { "pmemsave", "val:l,size:i,filename:s", do_physical_memory_save,
+    { "pmemsave", "val:l,size:i,filename:s", do_physical_memory_save, NULL,
       "addr size file", "save to disk physical memory dump starting at 'addr' of size 'size'", },
 STEXI
 @item pmemsave @var{addr} @var{size} @var{file}
 save to disk physical memory dump starting at @var{addr} of size @var{size}.
 ETEXI
 
-    { "boot_set", "bootdevice:s", do_boot_set,
+    { "boot_set", "bootdevice:s", do_boot_set, NULL,
       "bootdevice", "define new values for the boot device list" },
 STEXI
 @item boot_set @var{bootdevicelist}
@@ -458,7 +458,7 @@ the same that can be specified in the @code{-boot} command line option.
 ETEXI
 
 #if defined(TARGET_I386)
-    { "nmi", "cpu_index:i", do_inject_nmi,
+    { "nmi", "cpu_index:i", do_inject_nmi, NULL,
       "cpu", "inject an NMI on the given CPU", },
 #endif
 STEXI
@@ -466,28 +466,28 @@ STEXI
 Inject an NMI on the given CPU (x86 only).
 ETEXI
 
-    { "migrate", "detach:-d,uri:s", do_migrate,
+    { "migrate", "detach:-d,uri:s", do_migrate, NULL,
       "[-d] uri", "migrate to URI (using -d to not wait for completion)" },
 STEXI
 @item migrate [-d] @var{uri}
 Migrate to @var{uri} (using -d to not wait for completion).
 ETEXI
 
-    { "migrate_cancel", "", do_migrate_cancel,
+    { "migrate_cancel", "", do_migrate_cancel, NULL,
       "", "cancel the current VM migration" },
 STEXI
 @item migrate_cancel
 Cancel the current VM migration.
 ETEXI
 
-    { "migrate_set_speed", "value:s", do_migrate_set_speed,
+    { "migrate_set_speed", "value:s", do_migrate_set_speed, NULL,
       "value", "set maximum speed (in bytes) for migrations" },
 STEXI
 @item migrate_set_speed @var{value}
 Set maximum speed to @var{value} (in bytes) for migrations.
 ETEXI
 
-    { "migrate_set_downtime", "value:s", do_migrate_set_downtime,
+    { "migrate_set_downtime", "value:s", do_migrate_set_downtime, NULL,
       "value", "set maximum tolerated downtime (in seconds) for migrations" },
 
 STEXI
@@ -496,7 +496,7 @@ Set maximum tolerated downtime (in seconds) for migration.
 ETEXI
 
 #if defined(TARGET_I386)
-    { "drive_add", "pci_addr:s,opts:s", drive_hot_add,
+    { "drive_add", "pci_addr:s,opts:s", drive_hot_add, NULL,
                                         "[[<domain>:]<bus>:]<slot>\n"
                                         "[file=file][,if=type][,bus=n]\n"
                                         "[,unit=m][,media=d][index=i]\n"
@@ -510,7 +510,7 @@ Add drive to PCI storage controller.
 ETEXI
 
 #if defined(TARGET_I386)
-    { "pci_add", "pci_addr:s,type:s,opts:s?", pci_device_hot_add, "auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...", "hot-add PCI device" },
+    { "pci_add", "pci_addr:s,type:s,opts:s?", pci_device_hot_add, NULL, "auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...", "hot-add PCI device" },
 #endif
 STEXI
 @item pci_add
@@ -518,21 +518,21 @@ Hot-add PCI device.
 ETEXI
 
 #if defined(TARGET_I386)
-    { "pci_del", "pci_addr:s", do_pci_device_hot_remove, "[[<domain>:]<bus>:]<slot>", "hot remove PCI device" },
+    { "pci_del", "pci_addr:s", do_pci_device_hot_remove, NULL, "[[<domain>:]<bus>:]<slot>", "hot remove PCI device" },
 #endif
 STEXI
 @item pci_del
 Hot remove PCI device.
 ETEXI
 
-    { "host_net_add", "device:s,opts:s?", net_host_device_add,
+    { "host_net_add", "device:s,opts:s?", net_host_device_add, NULL,
       "tap|user|socket|vde|dump [options]", "add host VLAN client" },
 STEXI
 @item host_net_add
 Add host VLAN client.
 ETEXI
 
-    { "host_net_remove", "vlan_id:i,device:s", net_host_device_remove,
+    { "host_net_remove", "vlan_id:i,device:s", net_host_device_remove, NULL,
       "vlan_id name", "remove host VLAN client" },
 STEXI
 @item host_net_remove
@@ -540,11 +540,11 @@ Remove host VLAN client.
 ETEXI
 
 #ifdef CONFIG_SLIRP
-    { "hostfwd_add", "arg1:s,arg2:s?,arg3:s?", net_slirp_hostfwd_add,
+    { "hostfwd_add", "arg1:s,arg2:s?,arg3:s?", net_slirp_hostfwd_add, NULL,
       "[vlan_id name] [tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport",
       "redirect TCP or UDP connections from host to guest (requires -net user)" },
     { "hostfwd_remove", "arg1:s,arg2:s?,arg3:s?", net_slirp_hostfwd_remove,
-      "[vlan_id name] [tcp|udp]:[hostaddr]:hostport",
+      NULL, "[vlan_id name] [tcp|udp]:[hostaddr]:hostport",
       "remove host-to-guest TCP or UDP redirection" },
 #endif
 STEXI
@@ -552,28 +552,28 @@ STEXI
 Redirect TCP or UDP connections from host to guest (requires -net user).
 ETEXI
 
-    { "balloon", "value:i", do_balloon,
+    { "balloon", "value:i", do_balloon, NULL,
       "target", "request VM to change it's memory allocation (in MB)" },
 STEXI
 @item balloon @var{value}
 Request VM to change its memory allocation to @var{value} (in MB).
 ETEXI
 
-    { "set_link", "name:s,up_or_down:s", do_set_link,
+    { "set_link", "name:s,up_or_down:s", do_set_link, NULL,
       "name up|down", "change the link status of a network adapter" },
 STEXI
 @item set_link @var{name} [up|down]
 Set link @var{name} up or down.
 ETEXI
 
-    { "watchdog_action", "action:s", do_watchdog_action,
+    { "watchdog_action", "action:s", do_watchdog_action, NULL,
       "[reset|shutdown|poweroff|pause|debug|none]", "change watchdog action" },
 STEXI
 @item watchdog_action
 Change watchdog action.
 ETEXI
 
-    { "acl_show", "aclname:s", do_acl_show, "aclname",
+    { "acl_show", "aclname:s", do_acl_show, NULL, "aclname",
       "list rules in the access control list" },
 STEXI
 @item acl_show @var{aclname}
@@ -583,8 +583,8 @@ policy. There are currently two named access control lists,
 certificate distinguished name, and SASL username respectively.
 ETEXI
 
-    { "acl_policy", "aclname:s,policy:s", do_acl_policy, "aclname allow|deny",
-      "set default access control list policy" },
+    { "acl_policy", "aclname:s,policy:s", do_acl_policy, NULL,
+       "aclname allow|deny", "set default access control list policy" },
 STEXI
 @item acl_policy @var{aclname} @code{allow|deny}
 Set the default access control list policy, used in the event that
@@ -592,7 +592,8 @@ none of the explicit rules match. The default policy at startup is
 always @code{deny}.
 ETEXI
 
-    { "acl_add", "aclname:s,match:s,policy:s,index:i?", do_acl_add, "aclname match allow|deny [index]",
+    { "acl_add", "aclname:s,match:s,policy:s,index:i?", do_acl_add,
+      NULL, "aclname match allow|deny [index]",
       "add a match rule to the access control list" },
 STEXI
 @item acl_allow @var{aclname} @var{match} @code{allow|deny} [@var{index}]
@@ -604,14 +605,15 @@ normally be appended to the end of the ACL, but can be inserted
 earlier in the list if the optional @var{index} parameter is supplied.
 ETEXI
 
-    { "acl_remove", "aclname:s,match:s", do_acl_remove, "aclname match",
+    { "acl_remove", "aclname:s,match:s", do_acl_remove, NULL,
+      "aclname match",
       "remove a match rule from the access control list" },
 STEXI
 @item acl_remove @var{aclname} @var{match}
 Remove the specified match rule from the access control list.
 ETEXI
 
-    { "acl_reset", "aclname:s", do_acl_reset, "aclname",
+    { "acl_reset", "aclname:s", do_acl_reset, NULL, "aclname",
       "reset the access control list" },
 STEXI
 @item acl_remove @var{aclname} @var{match}
@@ -620,14 +622,16 @@ policy back to @code{deny}.
 ETEXI
 
 #if defined(TARGET_I386)
-    { "mce", "cpu_index:i,bank:i,status:l,mcg_status:l,addr:l,misc:l", do_inject_mce, "cpu bank status mcgstatus addr misc", "inject a MCE on the given CPU"},
+    { "mce", "cpu_index:i,bank:i,status:l,mcg_status:l,addr:l,misc:l",
+      do_inject_mce, NULL,
+     "cpu bank status mcgstatus addr misc", "inject a MCE on the given CPU"},
 #endif
 STEXI
 @item mce @var{cpu} @var{bank} @var{status} @var{mcgstatus} @var{addr} @var{misc}
 Inject an MCE on the given CPU (x86 only).
 ETEXI
 
-    { "getfd", "fdname:s", do_getfd, "getfd name",
+    { "getfd", "fdname:s", do_getfd, NULL, "getfd name",
       "receive a file descriptor via SCM rights and assign it a name" },
 STEXI
 @item getfd @var{fdname}
@@ -636,7 +640,7 @@ mechanism on unix sockets, it is stored using the name @var{fdname} for
 later use by other monitor commands.
 ETEXI
 
-    { "closefd", "fdname:s", do_closefd, "closefd name",
+    { "closefd", "fdname:s", do_closefd, NULL, "closefd name",
       "close a file descriptor previously passed via SCM rights" },
 STEXI
 @item closefd @var{fdname}
-- 
1.6.4.2.253.g0b1fac

[Qemu-devel] Re: [PATCH 1/5] monitor: Add user_print() to mon_cmd_t

[Juan Quintela]

Luiz Capitulino <lcapitulino@redhat.com> wrote:
> This new struct member will store a pointer to a function that
> should be used to output data in the user protocol format.
>
> Additionally, it will also serve as a flag to say if a given
> handler has already been ported to support the machine protocol.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c       |   69 +++++++++++++++---------------
>  qemu-monitor.hx |  124 ++++++++++++++++++++++++++++--------------------------
>  2 files changed, 99 insertions(+), 94 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index a242b4b..b43a287 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -71,6 +71,7 @@ typedef struct mon_cmd_t {
>      const char *name;
>      const char *args_type;
>      void *handler;
> +    void (*user_print)(Monitor *mon, const QObject *data);
>      const char *params;
>      const char *help;
>  } mon_cmd_t;
> @@ -1807,80 +1808,80 @@ static const mon_cmd_t mon_cmds[] = {
>  
>  /* Please update qemu-monitor.hx when adding or changing commands */
>  static const mon_cmd_t info_cmds[] = {

Once that your are changing all of them, could you use c99 initializers,
that way it is easier to see what the action is.

> -    { "version", "", do_info_version,
> +    { "version", "", do_info_version, NULL,
>        "", "show the version of QEMU" },

Later, Juan.

[Qemu-devel] Re: [PATCH 1/5] monitor: Add user_print() to mon_cmd_t

[Luiz Capitulino]

On Thu, 03 Sep 2009 20:55:51 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > This new struct member will store a pointer to a function that
> > should be used to output data in the user protocol format.
> >
> > Additionally, it will also serve as a flag to say if a given
> > handler has already been ported to support the machine protocol.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c       |   69 +++++++++++++++---------------
> >  qemu-monitor.hx |  124 ++++++++++++++++++++++++++++--------------------------
> >  2 files changed, 99 insertions(+), 94 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index a242b4b..b43a287 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -71,6 +71,7 @@ typedef struct mon_cmd_t {
> >      const char *name;
> >      const char *args_type;
> >      void *handler;
> > +    void (*user_print)(Monitor *mon, const QObject *data);
> >      const char *params;
> >      const char *help;
> >  } mon_cmd_t;
> > @@ -1807,80 +1808,80 @@ static const mon_cmd_t mon_cmds[] = {
> >  
> >  /* Please update qemu-monitor.hx when adding or changing commands */
> >  static const mon_cmd_t info_cmds[] = {
> 
> Once that your are changing all of them, could you use c99 initializers,
> that way it is easier to see what the action is.

 Yeah, I will do that for the real submission.

[Qemu-devel] [PATCH 2/5] monitor: Handle new and old style handlers

[Luiz Capitulino]

This commit changes monitor_handle_command() to support old style
and new style handlers.

New style handlers are protocol independent, they return their
data to the monitor, which in turn decides how to print them
(ie. user protocol vs. machine protocol).

Ported handlers will use the 'user_print' member of 'mon_cmd_t' to
define its user protocol function, which will be called to print
data in the user protocol format.

Handlers which don't have 'user_print' defined are not ported yet,
nothing changes for them.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index b43a287..9886bb0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2827,17 +2827,32 @@ static void monitor_handle_command(Monitor *mon, const char *cmdline)
     qdict = qdict_new();
 
     cmd = monitor_parse_command(mon, cmdline, qdict);
-    if (cmd) {
-        void (*handler)(Monitor *mon, const QDict *qdict);
+    if (!cmd)
+        goto out;
 
-        qemu_errors_to_mon(mon);
+    qemu_errors_to_mon(mon);
 
-        handler = cmd->handler;
-        handler(mon, qdict);
+    if (cmd->user_print) {
+        QObject *data = NULL;
+        int (*handler_new)(Monitor *mon, const QDict *params,
+                           QObject **ret_data);
 
-        qemu_errors_to_previous();
+        handler_new = cmd->handler;
+        handler_new(mon, qdict, &data);
+
+        cmd->user_print(mon, data);
+
+        if (data)
+            qobject_decref(data);
+    } else {
+        void (*handler_old)(Monitor *mon, const QDict *qdict);
+        handler_old = cmd->handler;
+        handler_old(mon, qdict);
     }
 
+   qemu_errors_to_previous();
+
+out:
     QDECREF(qdict);
 }
 
-- 
1.6.4.2.253.g0b1fac

[Qemu-devel] [PATCH 3/5] monitor: Port do_info() to QObject

[Luiz Capitulino]

do_info() is special, its job is to call 'info handlers'. This
is similar to what monitor_handle_command() does, therefore
do_info() also has to distinguish among new and old style
info handlers.

This commit ports do_info() to call info handlers according to
the new QObject style.

It's important to note that:

1. the return value of do_info() will be the same of the called
   handler
2. monitor_print_nothing() will be used by handlers to...
   print nothing :)

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   45 +++++++++++++++++++++++++++++++++------------
 qemu-monitor.hx |    2 +-
 2 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9886bb0..f9f3cbd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -208,6 +208,11 @@ static int monitor_fprintf(FILE *stream, const char *fmt, ...)
     return 0;
 }
 
+static void monitor_print_nothing(Monitor *mon, const QObject *data)
+{
+    return;
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
@@ -277,24 +282,40 @@ static void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
-static void do_info(Monitor *mon, const QDict *qdict)
+static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
+    int ret;
     const mon_cmd_t *cmd;
     const char *item = qdict_get_try_str(qdict, "item");
-    void (*handler)(Monitor *);
 
-    if (!item)
-        goto help;
-    for(cmd = info_cmds; cmd->name != NULL; cmd++) {
+    if (!item) {
+        help_cmd(mon, "info");
+        return 0;
+    }
+
+    for (cmd = info_cmds; cmd->name != NULL; cmd++) {
         if (compare_cmd(item, cmd->name))
-            goto found;
+            break;
     }
- help:
-    help_cmd(mon, "info");
-    return;
- found:
-    handler = cmd->handler;
-    handler(mon);
+
+    if (cmd->name == NULL)
+        return -1;
+
+    ret = 0;
+    if (cmd->user_print) {
+        int (*handler_new)(Monitor *mon, QObject **ret_data);
+
+        handler_new = cmd->handler;
+        ret = handler_new(mon, ret_data);
+
+        cmd->user_print(mon, *ret_data);
+    } else {
+        void (*handler_old)(Monitor *);
+        handler_old = cmd->handler;
+        handler_old(mon);
+    }
+
+    return ret;
 }
 
 static void do_info_version(Monitor *mon)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index a6d0f2e..44402c5 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -22,7 +22,7 @@ STEXI
 Commit changes to the disk images (if -snapshot is used) or backing files.
 ETEXI
 
-    { "info", "item:s?", do_info, NULL,
+    { "info", "item:s?", do_info, monitor_print_nothing,
       "[subcommand]", "show various information about the system state" },
 STEXI
 @item info @var{subcommand}
-- 
1.6.4.2.253.g0b1fac

[Qemu-devel] [PATCH 4/5] monitor: Port do_info_balloon() to QObject

[Luiz Capitulino]

do_info_balloon() returns 0 on success or -1 when the 'balloon'
value could not be queried.

The returned data is always a QString.

This also introduces monitor_print_qobject(), which can be
used as a standard way to print QObjects in the user protocol
format.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   41 +++++++++++++++++++++++++++++++++--------
 1 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/monitor.c b/monitor.c
index f9f3cbd..545833d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -213,6 +213,22 @@ static void monitor_print_nothing(Monitor *mon, const QObject *data)
     return;
 }
 
+static void monitor_print_qobject(Monitor *mon, const QObject *data)
+{
+    switch (qobject_type(data)) {
+        case QTYPE_QSTRING:
+            monitor_printf(mon, "%s",qstring_get_str(qobject_to_qstring(data)));
+            break;
+        case QTYPE_QINT:
+            monitor_printf(mon, "%lu", qint_get_int(qobject_to_qint(data)));
+            break;
+        default:
+            monitor_printf(mon, "ERROR: unsupported type: %d\n",
+                                                        qobject_type(data));
+            break;
+    }
+}
+
 static int compare_cmd(const char *name, const char *list)
 {
     const char *p, *pstart;
@@ -1586,18 +1602,27 @@ static void do_balloon(Monitor *mon, const QDict *qdict)
     qemu_balloon(target << 20);
 }
 
-static void do_info_balloon(Monitor *mon)
+static int do_info_balloon(Monitor *mon, QObject **ret_data)
 {
+    QString *qs;
     ram_addr_t actual;
+    int ret = -1;
 
     actual = qemu_balloon_status();
-    if (kvm_enabled() && !kvm_has_sync_mmu())
-        monitor_printf(mon, "Using KVM without synchronous MMU, "
+    if (kvm_enabled() && !kvm_has_sync_mmu()) {
+        qs = qstring_from_str("Using KVM without synchronous MMU, "
                        "ballooning disabled\n");
-    else if (actual == 0)
-        monitor_printf(mon, "Ballooning not activated in VM\n");
-    else
-        monitor_printf(mon, "balloon: actual=%d\n", (int)(actual >> 20));
+    } else if (actual == 0) {
+        qs = qstring_from_str("Ballooning not activated in VM\n");
+    } else {
+        char buf[128];
+        sprintf(buf, "balloon: actual=%d\n", (int)(actual >> 20));
+        qs = qstring_from_str(buf);
+        ret = 0;
+    }
+
+    *ret_data = QOBJECT(qs);
+    return ret;
 }
 
 static qemu_acl *find_acl(Monitor *mon, const char *name)
@@ -1898,7 +1923,7 @@ static const mon_cmd_t info_cmds[] = {
       "", "show user network stack connection states", },
 #endif
     { "migrate", "", do_info_migrate, NULL, "", "show migration status" },
-    { "balloon", "", do_info_balloon, NULL,
+    { "balloon", "", do_info_balloon, monitor_print_qobject,
       "", "show balloon information" },
     { "qtree", "", do_info_qtree, NULL,
       "", "show device tree" },
-- 
1.6.4.2.253.g0b1fac

Re: [Qemu-devel] [PATCH 4/5] monitor: Port do_info_balloon() to QObject

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> do_info_balloon() returns 0 on success or -1 when the 'balloon'
> value could not be queried.
>
> The returned data is always a QString.
>
> This also introduces monitor_print_qobject(), which can be
> used as a standard way to print QObjects in the user protocol
> format.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |   41 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index f9f3cbd..545833d 100644
> --- a/monitor.c
> +++ b/monitor.c
[...]
> @@ -1586,18 +1602,27 @@ static void do_balloon(Monitor *mon, const QDict *qdict)
>      qemu_balloon(target << 20);
>  }
>  
> -static void do_info_balloon(Monitor *mon)
> +static int do_info_balloon(Monitor *mon, QObject **ret_data)
>  {
> +    QString *qs;
>      ram_addr_t actual;
> +    int ret = -1;
>  
>      actual = qemu_balloon_status();
> -    if (kvm_enabled() && !kvm_has_sync_mmu())
> -        monitor_printf(mon, "Using KVM without synchronous MMU, "
> +    if (kvm_enabled() && !kvm_has_sync_mmu()) {
> +        qs = qstring_from_str("Using KVM without synchronous MMU, "
>                         "ballooning disabled\n");
> -    else if (actual == 0)
> -        monitor_printf(mon, "Ballooning not activated in VM\n");
> -    else
> -        monitor_printf(mon, "balloon: actual=%d\n", (int)(actual >> 20));
> +    } else if (actual == 0) {
> +        qs = qstring_from_str("Ballooning not activated in VM\n");
> +    } else {
> +        char buf[128];
> +        sprintf(buf, "balloon: actual=%d\n", (int)(actual >> 20));
> +        qs = qstring_from_str(buf);
> +        ret = 0;

If this pattern turns out to be common, let's create qstring_printf() to
print to a new qstring.

> +    }
> +
> +    *ret_data = QOBJECT(qs);
> +    return ret;
>  }
>  
>  static qemu_acl *find_acl(Monitor *mon, const char *name)
> @@ -1898,7 +1923,7 @@ static const mon_cmd_t info_cmds[] = {
>        "", "show user network stack connection states", },
>  #endif
>      { "migrate", "", do_info_migrate, NULL, "", "show migration status" },
> -    { "balloon", "", do_info_balloon, NULL,
> +    { "balloon", "", do_info_balloon, monitor_print_qobject,
>        "", "show balloon information" },
>      { "qtree", "", do_info_qtree, NULL,
>        "", "show device tree" },

Re: [Qemu-devel] [PATCH 4/5] monitor: Port do_info_balloon() to QObject

[Luiz Capitulino]

On Wed, 23 Sep 2009 17:49:42 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > do_info_balloon() returns 0 on success or -1 when the 'balloon'
> > value could not be queried.
> >
> > The returned data is always a QString.
> >
> > This also introduces monitor_print_qobject(), which can be
> > used as a standard way to print QObjects in the user protocol
> > format.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c |   41 +++++++++++++++++++++++++++++++++--------
> >  1 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index f9f3cbd..545833d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> [...]
> > @@ -1586,18 +1602,27 @@ static void do_balloon(Monitor *mon, const QDict *qdict)
> >      qemu_balloon(target << 20);
> >  }
> >  
> > -static void do_info_balloon(Monitor *mon)
> > +static int do_info_balloon(Monitor *mon, QObject **ret_data)
> >  {
> > +    QString *qs;
> >      ram_addr_t actual;
> > +    int ret = -1;
> >  
> >      actual = qemu_balloon_status();
> > -    if (kvm_enabled() && !kvm_has_sync_mmu())
> > -        monitor_printf(mon, "Using KVM without synchronous MMU, "
> > +    if (kvm_enabled() && !kvm_has_sync_mmu()) {
> > +        qs = qstring_from_str("Using KVM without synchronous MMU, "
> >                         "ballooning disabled\n");
> > -    else if (actual == 0)
> > -        monitor_printf(mon, "Ballooning not activated in VM\n");
> > -    else
> > -        monitor_printf(mon, "balloon: actual=%d\n", (int)(actual >> 20));
> > +    } else if (actual == 0) {
> > +        qs = qstring_from_str("Ballooning not activated in VM\n");
> > +    } else {
> > +        char buf[128];
> > +        sprintf(buf, "balloon: actual=%d\n", (int)(actual >> 20));
> > +        qs = qstring_from_str(buf);
> > +        ret = 0;
> 
> If this pattern turns out to be common, let's create qstring_printf() to
> print to a new qstring.

 Yes, I already have it in my tree, but do_info_balloon() is a bit
different now.

 This is my RFC series, right? Things change fast on QMP buggy world. :)

[Qemu-devel] [PATCH 5/5] monitor: Port do_balloon() to QObject

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    3 ++-
 qemu-monitor.hx |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 545833d..71aef60 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1595,11 +1595,12 @@ static void do_info_status(Monitor *mon)
 }
 
 
-static void do_balloon(Monitor *mon, const QDict *qdict)
+static int do_balloon(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     int value = qdict_get_int(qdict, "value");
     ram_addr_t target = value;
     qemu_balloon(target << 20);
+    return 0;
 }
 
 static int do_info_balloon(Monitor *mon, QObject **ret_data)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 44402c5..eec6731 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -552,7 +552,7 @@ STEXI
 Redirect TCP or UDP connections from host to guest (requires -net user).
 ETEXI
 
-    { "balloon", "value:i", do_balloon, NULL,
+    { "balloon", "value:i", do_balloon, monitor_print_nothing,
       "target", "request VM to change it's memory allocation (in MB)" },
 STEXI
 @item balloon @var{value}
-- 
1.6.4.2.253.g0b1fac

Re: [Qemu-devel] [RFC 0/5] Monitor handlers convertion to QObject

[Gerd Hoffmann]

   Hi,

>   Some people have suggested us to define errors codes, but I dislike this
> because I have the impression we will end up defining errors per-handlers,
> which is a lot of work and more protocol definitions to maintain.

What about error messages?

I've recently added some infrastructure for error messages: 
qemu_error() + friends.  Intention is to direct error messages to the 
correct place, especially for code which can be called from multiple 
paths.  Device creation for example can be triggered by command line 
(errors should go to stderr) or via hotplug monitor commands (errors 
should go to the monitor).

You might want to add a third error sink which stuffs error messages 
into a Qstring, so you can pass them along as Qobject without the code 
emitting the error message knowing anything about it.

>   Another issue in this subject is that sometimes we will have to do
> a not so small refactor to get the proper error code. I mean, sometimes
> the functions which the handlers call return void, and those functions in
> turn call other functions which also return void. This seem to be the
> case of do_balloon(), for example.

There are also a few places which call exit(1) on error.  Which is ok 
when something goes wrong while creating the virtual machine, but not 
when some monitor command failed ...

>   A possible solution is to only return error when it's easy, otherwise we
> could always return 0 (which is what we have today), the problem though
> is that changing this in the future will cause trouble.

Serious code audit is more work initially, but I think we well be very 
happy we did it in the long run ...

cheers,
   Gerd

Re: [Qemu-devel] [RFC 0/5] Monitor handlers convertion to QObject

[Luiz Capitulino]

On Thu, 03 Sep 2009 22:30:08 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>    Hi,
> 
> >   Some people have suggested us to define errors codes, but I dislike this
> > because I have the impression we will end up defining errors per-handlers,
> > which is a lot of work and more protocol definitions to maintain.
> 
> What about error messages?

 Yes, I had error messages in mind.

 Handlers that handle errors today already print error messages,
this way the additional effort would be with the ones which don't
handle errors.

 However, my initial argument is not very strong. We already have
per-handlers errors today and will also have them with strings.

> I've recently added some infrastructure for error messages: 
> qemu_error() + friends.  Intention is to direct error messages to the 
> correct place, especially for code which can be called from multiple 
> paths.  Device creation for example can be triggered by command line 
> (errors should go to stderr) or via hotplug monitor commands (errors 
> should go to the monitor).
> 
> You might want to add a third error sink which stuffs error messages 
> into a Qstring, so you can pass them along as Qobject without the code 
> emitting the error message knowing anything about it.

 Ok, I will review qemu_error() and consider your suggestion.

> 
> >   Another issue in this subject is that sometimes we will have to do
> > a not so small refactor to get the proper error code. I mean, sometimes
> > the functions which the handlers call return void, and those functions in
> > turn call other functions which also return void. This seem to be the
> > case of do_balloon(), for example.
> 
> There are also a few places which call exit(1) on error.  Which is ok 
> when something goes wrong while creating the virtual machine, but not 
> when some monitor command failed ...

 Right.

> 
> >   A possible solution is to only return error when it's easy, otherwise we
> > could always return 0 (which is what we have today), the problem though
> > is that changing this in the future will cause trouble.
> 
> Serious code audit is more work initially, but I think we well be very 
> happy we did it in the long run ...

 That's right.

 I'm doing my best to have the protocol working ASAP, but it's
unfortunate that sometimes the Right Thing takes more time.