* [Qemu-devel] [PATCH 0/3] HMP: enable info sub command taking parameter
@ 2012-12-19 10:17 Wenchao Xia
2012-12-19 10:17 ` [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler Wenchao Xia
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Wenchao Xia @ 2012-12-19 10:17 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, aliguori, stefanha, armbru, lcapitulino, pbonzini,
Wenchao Xia
This serial of patch will enhance HMP's info command, make all command
possible to take additional parameter. Please review to see if it have
problem.
Wenchao Xia (3):
HMP: add QDict to info callback handler
HMP: pass in parameter for info sub command
HMP: show internal snapshots on a single device
hmp-commands.hx | 2 +-
hmp.c | 36 ++++++++--------
hmp.h | 36 ++++++++--------
hw/i8259.c | 4 +-
hw/lm32_pic.c | 4 +-
hw/lm32_pic.h | 4 +-
hw/loader.c | 2 +-
hw/loader.h | 3 +-
hw/pc.h | 4 +-
hw/pcmcia.h | 2 +-
hw/qdev-monitor.c | 4 +-
hw/qdev-monitor.h | 4 +-
hw/sun4m.c | 4 +-
hw/sun4m.h | 4 +-
hw/usb.h | 2 +-
hw/usb/bus.c | 2 +-
hw/usb/host-bsd.c | 2 +-
hw/usb/host-linux.c | 2 +-
hw/usb/host-stub.c | 2 +-
monitor.c | 115 +++++++++++++++++++++++++++++++++------------------
net.c | 2 +-
net.h | 2 +-
net/slirp.c | 2 +-
net/slirp.h | 2 +-
savevm.c | 55 ++++++++++++++++++++++++-
sysemu.h | 4 +-
vl.c | 2 +-
27 files changed, 197 insertions(+), 110 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler 2012-12-19 10:17 [Qemu-devel] [PATCH 0/3] HMP: enable info sub command taking parameter Wenchao Xia @ 2012-12-19 10:17 ` Wenchao Xia 2012-12-21 14:01 ` Markus Armbruster 2012-12-19 10:17 ` [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device Wenchao Xia 2 siblings, 1 reply; 14+ messages in thread From: Wenchao Xia @ 2012-12-19 10:17 UTC (permalink / raw) To: qemu-devel Cc: kwolf, aliguori, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia This patch change all info call back function to take additional QDict * parameter, which allow those command take parameter. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- hmp.c | 36 ++++++++++++++++++------------------ hmp.h | 36 ++++++++++++++++++------------------ hw/i8259.c | 4 ++-- hw/lm32_pic.c | 4 ++-- hw/lm32_pic.h | 4 ++-- hw/loader.c | 2 +- hw/loader.h | 3 ++- hw/pc.h | 4 ++-- hw/pcmcia.h | 2 +- hw/qdev-monitor.c | 4 ++-- hw/qdev-monitor.h | 4 ++-- hw/sun4m.c | 4 ++-- hw/sun4m.h | 4 ++-- hw/usb.h | 2 +- hw/usb/bus.c | 2 +- hw/usb/host-bsd.c | 2 +- hw/usb/host-linux.c | 2 +- hw/usb/host-stub.c | 2 +- monitor.c | 32 ++++++++++++++++---------------- net.c | 2 +- net.h | 2 +- net/slirp.c | 2 +- net/slirp.h | 2 +- savevm.c | 2 +- sysemu.h | 4 ++-- vl.c | 2 +- 26 files changed, 85 insertions(+), 84 deletions(-) diff --git a/hmp.c b/hmp.c index 180ba2b..0aab1d8 100644 --- a/hmp.c +++ b/hmp.c @@ -30,7 +30,7 @@ static void hmp_handle_error(Monitor *mon, Error **errp) } } -void hmp_info_name(Monitor *mon) +void hmp_info_name(Monitor *mon, const QDict *qdict) { NameInfo *info; @@ -41,7 +41,7 @@ void hmp_info_name(Monitor *mon) qapi_free_NameInfo(info); } -void hmp_info_version(Monitor *mon) +void hmp_info_version(Monitor *mon, const QDict *qdict) { VersionInfo *info; @@ -54,7 +54,7 @@ void hmp_info_version(Monitor *mon) qapi_free_VersionInfo(info); } -void hmp_info_kvm(Monitor *mon) +void hmp_info_kvm(Monitor *mon, const QDict *qdict) { KvmInfo *info; @@ -69,7 +69,7 @@ void hmp_info_kvm(Monitor *mon) qapi_free_KvmInfo(info); } -void hmp_info_status(Monitor *mon) +void hmp_info_status(Monitor *mon, const QDict *qdict) { StatusInfo *info; @@ -88,7 +88,7 @@ void hmp_info_status(Monitor *mon) qapi_free_StatusInfo(info); } -void hmp_info_uuid(Monitor *mon) +void hmp_info_uuid(Monitor *mon, const QDict *qdict) { UuidInfo *info; @@ -97,7 +97,7 @@ void hmp_info_uuid(Monitor *mon) qapi_free_UuidInfo(info); } -void hmp_info_chardev(Monitor *mon) +void hmp_info_chardev(Monitor *mon, const QDict *qdict) { ChardevInfoList *char_info, *info; @@ -110,7 +110,7 @@ void hmp_info_chardev(Monitor *mon) qapi_free_ChardevInfoList(char_info); } -void hmp_info_mice(Monitor *mon) +void hmp_info_mice(Monitor *mon, const QDict *qdict) { MouseInfoList *mice_list, *mouse; @@ -130,7 +130,7 @@ void hmp_info_mice(Monitor *mon) qapi_free_MouseInfoList(mice_list); } -void hmp_info_migrate(Monitor *mon) +void hmp_info_migrate(Monitor *mon, const QDict *qdict) { MigrationInfo *info; MigrationCapabilityStatusList *caps, *cap; @@ -208,7 +208,7 @@ void hmp_info_migrate(Monitor *mon) qapi_free_MigrationCapabilityStatusList(caps); } -void hmp_info_migrate_capabilities(Monitor *mon) +void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict) { MigrationCapabilityStatusList *caps, *cap; @@ -227,13 +227,13 @@ void hmp_info_migrate_capabilities(Monitor *mon) qapi_free_MigrationCapabilityStatusList(caps); } -void hmp_info_migrate_cache_size(Monitor *mon) +void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict) { monitor_printf(mon, "xbzrel cache size: %" PRId64 " kbytes\n", qmp_query_migrate_cache_size(NULL) >> 10); } -void hmp_info_cpus(Monitor *mon) +void hmp_info_cpus(Monitor *mon, const QDict *qdict) { CpuInfoList *cpu_list, *cpu; @@ -271,7 +271,7 @@ void hmp_info_cpus(Monitor *mon) qapi_free_CpuInfoList(cpu_list); } -void hmp_info_block(Monitor *mon) +void hmp_info_block(Monitor *mon, const QDict *qdict) { BlockInfoList *block_list, *info; @@ -325,7 +325,7 @@ void hmp_info_block(Monitor *mon) qapi_free_BlockInfoList(block_list); } -void hmp_info_blockstats(Monitor *mon) +void hmp_info_blockstats(Monitor *mon, const QDict *qdict) { BlockStatsList *stats_list, *stats; @@ -359,7 +359,7 @@ void hmp_info_blockstats(Monitor *mon) qapi_free_BlockStatsList(stats_list); } -void hmp_info_vnc(Monitor *mon) +void hmp_info_vnc(Monitor *mon, const QDict *qdict) { VncInfo *info; Error *err = NULL; @@ -405,7 +405,7 @@ out: qapi_free_VncInfo(info); } -void hmp_info_spice(Monitor *mon) +void hmp_info_spice(Monitor *mon, const QDict *qdict) { SpiceChannelList *chan; SpiceInfo *info; @@ -452,7 +452,7 @@ out: qapi_free_SpiceInfo(info); } -void hmp_info_balloon(Monitor *mon) +void hmp_info_balloon(Monitor *mon, const QDict *qdict) { BalloonInfo *info; Error *err = NULL; @@ -569,7 +569,7 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev) } } -void hmp_info_pci(Monitor *mon) +void hmp_info_pci(Monitor *mon, const QDict *qdict) { PciInfoList *info_list, *info; Error *err = NULL; @@ -592,7 +592,7 @@ void hmp_info_pci(Monitor *mon) qapi_free_PciInfoList(info_list); } -void hmp_info_block_jobs(Monitor *mon) +void hmp_info_block_jobs(Monitor *mon, const QDict *qdict) { BlockJobInfoList *list; Error *err = NULL; diff --git a/hmp.h b/hmp.h index 0ab03be..c3f3a3f 100644 --- a/hmp.h +++ b/hmp.h @@ -18,24 +18,24 @@ #include "qapi-types.h" #include "qdict.h" -void hmp_info_name(Monitor *mon); -void hmp_info_version(Monitor *mon); -void hmp_info_kvm(Monitor *mon); -void hmp_info_status(Monitor *mon); -void hmp_info_uuid(Monitor *mon); -void hmp_info_chardev(Monitor *mon); -void hmp_info_mice(Monitor *mon); -void hmp_info_migrate(Monitor *mon); -void hmp_info_migrate_capabilities(Monitor *mon); -void hmp_info_migrate_cache_size(Monitor *mon); -void hmp_info_cpus(Monitor *mon); -void hmp_info_block(Monitor *mon); -void hmp_info_blockstats(Monitor *mon); -void hmp_info_vnc(Monitor *mon); -void hmp_info_spice(Monitor *mon); -void hmp_info_balloon(Monitor *mon); -void hmp_info_pci(Monitor *mon); -void hmp_info_block_jobs(Monitor *mon); +void hmp_info_name(Monitor *mon, const QDict *qdict); +void hmp_info_version(Monitor *mon, const QDict *qdict); +void hmp_info_kvm(Monitor *mon, const QDict *qdict); +void hmp_info_status(Monitor *mon, const QDict *qdict); +void hmp_info_uuid(Monitor *mon, const QDict *qdict); +void hmp_info_chardev(Monitor *mon, const QDict *qdict); +void hmp_info_mice(Monitor *mon, const QDict *qdict); +void hmp_info_migrate(Monitor *mon, const QDict *qdict); +void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict); +void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict); +void hmp_info_cpus(Monitor *mon, const QDict *qdict); +void hmp_info_block(Monitor *mon, const QDict *qdict); +void hmp_info_blockstats(Monitor *mon, const QDict *qdict); +void hmp_info_vnc(Monitor *mon, const QDict *qdict); +void hmp_info_spice(Monitor *mon, const QDict *qdict); +void hmp_info_balloon(Monitor *mon, const QDict *qdict); +void hmp_info_pci(Monitor *mon, const QDict *qdict); +void hmp_info_block_jobs(Monitor *mon, const QDict *qdict); void hmp_quit(Monitor *mon, const QDict *qdict); void hmp_stop(Monitor *mon, const QDict *qdict); void hmp_system_reset(Monitor *mon, const QDict *qdict); diff --git a/hw/i8259.c b/hw/i8259.c index af0ba4d..a143039 100644 --- a/hw/i8259.c +++ b/hw/i8259.c @@ -407,7 +407,7 @@ static void pic_init(PICCommonState *s) qdev_init_gpio_in(&s->dev.qdev, pic_set_irq, 8); } -void pic_info(Monitor *mon) +void pic_info(Monitor *mon, const QDict *qdict) { int i; PICCommonState *s; @@ -425,7 +425,7 @@ void pic_info(Monitor *mon) } } -void irq_info(Monitor *mon) +void irq_info(Monitor *mon, const QDict *qdict) { #ifndef DEBUG_IRQ_COUNT monitor_printf(mon, "irq statistic code not compiled.\n"); diff --git a/hw/lm32_pic.c b/hw/lm32_pic.c index 32f65db..bec1fee 100644 --- a/hw/lm32_pic.c +++ b/hw/lm32_pic.c @@ -39,7 +39,7 @@ struct LM32PicState { typedef struct LM32PicState LM32PicState; static LM32PicState *pic; -void lm32_do_pic_info(Monitor *mon) +void lm32_do_pic_info(Monitor *mon, const QDict *qdict) { if (pic == NULL) { return; @@ -49,7 +49,7 @@ void lm32_do_pic_info(Monitor *mon) pic->im, pic->ip, pic->irq_state); } -void lm32_irq_info(Monitor *mon) +void lm32_irq_info(Monitor *mon, const QDict *qdict) { int i; uint32_t count; diff --git a/hw/lm32_pic.h b/hw/lm32_pic.h index 14456f3..5556803 100644 --- a/hw/lm32_pic.h +++ b/hw/lm32_pic.h @@ -8,7 +8,7 @@ uint32_t lm32_pic_get_im(DeviceState *d); void lm32_pic_set_ip(DeviceState *d, uint32_t ip); void lm32_pic_set_im(DeviceState *d, uint32_t im); -void lm32_do_pic_info(Monitor *mon); -void lm32_irq_info(Monitor *mon); +void lm32_do_pic_info(Monitor *mon, const QDict *qdict); +void lm32_irq_info(Monitor *mon, const QDict *qdict); #endif /* QEMU_HW_LM32_PIC_H */ diff --git a/hw/loader.c b/hw/loader.c index ba01ca6..d1e7bf2 100644 --- a/hw/loader.c +++ b/hw/loader.c @@ -778,7 +778,7 @@ void *rom_ptr(hwaddr addr) return rom->data + (addr - rom->addr); } -void do_info_roms(Monitor *mon) +void do_info_roms(Monitor *mon, const QDict *qdict) { Rom *rom; diff --git a/hw/loader.h b/hw/loader.h index 26480ad..d2b63f6 100644 --- a/hw/loader.h +++ b/hw/loader.h @@ -1,5 +1,6 @@ #ifndef LOADER_H #define LOADER_H +#include "qdict.h" /* loader.c */ int get_image_size(const char *filename); @@ -30,7 +31,7 @@ int rom_load_all(void); void rom_set_fw(void *f); int rom_copy(uint8_t *dest, hwaddr addr, size_t size); void *rom_ptr(hwaddr addr); -void do_info_roms(Monitor *mon); +void do_info_roms(Monitor *mon, const QDict *qdict); #define rom_add_file_fixed(_f, _a, _i) \ rom_add_file(_f, NULL, _a, _i) diff --git a/hw/pc.h b/hw/pc.h index 2237e86..e284892 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -40,8 +40,8 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq); qemu_irq *kvm_i8259_init(ISABus *bus); int pic_read_irq(DeviceState *d); int pic_get_output(DeviceState *d); -void pic_info(Monitor *mon); -void irq_info(Monitor *mon); +void pic_info(Monitor *mon, const QDict *qdict); +void irq_info(Monitor *mon, const QDict *qdict); /* Global System Interrupts */ diff --git a/hw/pcmcia.h b/hw/pcmcia.h index 50648c9..3db98be 100644 --- a/hw/pcmcia.h +++ b/hw/pcmcia.h @@ -11,7 +11,7 @@ typedef struct { void pcmcia_socket_register(PCMCIASocket *socket); void pcmcia_socket_unregister(PCMCIASocket *socket); -void pcmcia_info(Monitor *mon); +void pcmcia_info(Monitor *mon, const QDict *qdict); struct PCMCIACardState { void *state; diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index a1b4d6a..9a473a2 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -563,13 +563,13 @@ static void qbus_print(Monitor *mon, BusState *bus, int indent) } #undef qdev_printf -void do_info_qtree(Monitor *mon) +void do_info_qtree(Monitor *mon, const QDict *qdict) { if (sysbus_get_default()) qbus_print(mon, sysbus_get_default(), 0); } -void do_info_qdm(Monitor *mon) +void do_info_qdm(Monitor *mon, const QDict *qdict) { object_class_foreach(qdev_print_devinfo, TYPE_DEVICE, false, NULL); } diff --git a/hw/qdev-monitor.h b/hw/qdev-monitor.h index 220ceba..ada9e15 100644 --- a/hw/qdev-monitor.h +++ b/hw/qdev-monitor.h @@ -6,8 +6,8 @@ /*** monitor commands ***/ -void do_info_qtree(Monitor *mon); -void do_info_qdm(Monitor *mon); +void do_info_qtree(Monitor *mon, const QDict *qdict); +void do_info_qdm(Monitor *mon, const QDict *qdict); int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data); int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data); int qdev_device_help(QemuOpts *opts); diff --git a/hw/sun4m.c b/hw/sun4m.c index 52cf82b..de52b5e 100644 --- a/hw/sun4m.c +++ b/hw/sun4m.c @@ -216,13 +216,13 @@ static void nvram_init(M48t59State *nvram, uint8_t *macaddr, static DeviceState *slavio_intctl; -void sun4m_pic_info(Monitor *mon) +void sun4m_pic_info(Monitor *mon, const QDict *qdict) { if (slavio_intctl) slavio_pic_info(mon, slavio_intctl); } -void sun4m_irq_info(Monitor *mon) +void sun4m_irq_info(Monitor *mon, const QDict *qdict) { if (slavio_intctl) slavio_irq_info(mon, slavio_intctl); diff --git a/hw/sun4m.h b/hw/sun4m.h index 47eb945..0361eee 100644 --- a/hw/sun4m.h +++ b/hw/sun4m.h @@ -27,8 +27,8 @@ void slavio_pic_info(Monitor *mon, DeviceState *dev); void slavio_irq_info(Monitor *mon, DeviceState *dev); /* sun4m.c */ -void sun4m_pic_info(Monitor *mon); -void sun4m_irq_info(Monitor *mon); +void sun4m_pic_info(Monitor *mon, const QDict *qdict); +void sun4m_irq_info(Monitor *mon, const QDict *qdict); /* sparc32_dma.c */ #include "sparc32_dma.h" diff --git a/hw/usb.h b/hw/usb.h index efae65d..5116ee9 100644 --- a/hw/usb.h +++ b/hw/usb.h @@ -429,7 +429,7 @@ int set_usb_string(uint8_t *buf, const char *str); /* usb-linux.c */ USBDevice *usb_host_device_open(USBBus *bus, const char *devname); int usb_host_device_close(const char *devname); -void usb_host_info(Monitor *mon); +void usb_host_info(Monitor *mon, const QDict *qdict); /* usb-bt.c */ USBDevice *usb_bt_init(USBBus *bus, HCIInfo *hci); diff --git a/hw/usb/bus.c b/hw/usb/bus.c index 8264c24..13b813b 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -534,7 +534,7 @@ static char *usb_get_fw_dev_path(DeviceState *qdev) return fw_path; } -void usb_info(Monitor *mon) +void usb_info(Monitor *mon, const QDict *qdict) { USBBus *bus; USBDevice *dev; diff --git a/hw/usb/host-bsd.c b/hw/usb/host-bsd.c index dae0009..791c905 100644 --- a/hw/usb/host-bsd.c +++ b/hw/usb/host-bsd.c @@ -633,7 +633,7 @@ static int usb_host_info_device(void *opaque, return 0; } -void usb_host_info(Monitor *mon) +void usb_host_info(Monitor *mon, const QDict *qdict) { usb_host_scan(mon, usb_host_info_device); } diff --git a/hw/usb/host-linux.c b/hw/usb/host-linux.c index bdafb6b..62de0d6 100644 --- a/hw/usb/host-linux.c +++ b/hw/usb/host-linux.c @@ -1998,7 +1998,7 @@ static void hex2str(int val, char *str, size_t size) } } -void usb_host_info(Monitor *mon) +void usb_host_info(Monitor *mon, const QDict *qdict) { struct USBAutoFilter *f; struct USBHostDevice *s; diff --git a/hw/usb/host-stub.c b/hw/usb/host-stub.c index b4e10c1..7907841 100644 --- a/hw/usb/host-stub.c +++ b/hw/usb/host-stub.c @@ -35,7 +35,7 @@ #include "hw/usb.h" #include "monitor.h" -void usb_host_info(Monitor *mon) +void usb_host_info(Monitor *mon, const QDict *qdict) { monitor_printf(mon, "USB host devices not supported\n"); } diff --git a/monitor.c b/monitor.c index c0e32d6..797680f 100644 --- a/monitor.c +++ b/monitor.c @@ -123,7 +123,7 @@ typedef struct mon_cmd_t { const char *help; void (*user_print)(Monitor *mon, const QObject *data); union { - void (*info)(Monitor *mon); + void (*info)(Monitor *mon, const QDict *qdict); void (*cmd)(Monitor *mon, const QDict *qdict); int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data); int (*cmd_async)(Monitor *mon, const QDict *params, @@ -824,7 +824,7 @@ static void do_info(Monitor *mon, const QDict *qdict) goto help; } - cmd->mhandler.info(mon); + cmd->mhandler.info(mon, NULL); return; help: @@ -895,19 +895,19 @@ int monitor_get_cpu_index(void) return mon_get_cpu()->cpu_index; } -static void do_info_registers(Monitor *mon) +static void do_info_registers(Monitor *mon, const QDict *qdict) { CPUArchState *env; env = mon_get_cpu(); cpu_dump_state(env, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU); } -static void do_info_jit(Monitor *mon) +static void do_info_jit(Monitor *mon, const QDict *qdict) { dump_exec_info((FILE *)mon, monitor_fprintf); } -static void do_info_history(Monitor *mon) +static void do_info_history(Monitor *mon, const QDict *qdict) { int i; const char *str; @@ -926,7 +926,7 @@ static void do_info_history(Monitor *mon) #if defined(TARGET_PPC) /* XXX: not implemented in other targets */ -static void do_info_cpu_stats(Monitor *mon) +static void do_info_cpu_stats(Monitor *mon, const QDict *qdict) { CPUArchState *env; @@ -935,7 +935,7 @@ static void do_info_cpu_stats(Monitor *mon) } #endif -static void do_trace_print_events(Monitor *mon) +static void do_trace_print_events(Monitor *mon, const QDict *qdict) { trace_print_events((FILE *)mon, &monitor_fprintf); } @@ -1487,7 +1487,7 @@ static void tlb_info_64(Monitor *mon, CPUArchState *env) } #endif -static void tlb_info(Monitor *mon) +static void tlb_info(Monitor *mon, const QDict *qdict) { CPUArchState *env; @@ -1710,7 +1710,7 @@ static void mem_info_64(Monitor *mon, CPUArchState *env) } #endif -static void mem_info(Monitor *mon) +static void mem_info(Monitor *mon, const QDict *qdict) { CPUArchState *env; @@ -1749,7 +1749,7 @@ static void print_tlb(Monitor *mon, int idx, tlb_t *tlb) tlb->d, tlb->wt); } -static void tlb_info(Monitor *mon) +static void tlb_info(Monitor *mon, const QDict *qdict) { CPUArchState *env = mon_get_cpu(); int i; @@ -1765,7 +1765,7 @@ static void tlb_info(Monitor *mon) #endif #if defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_XTENSA) -static void tlb_info(Monitor *mon) +static void tlb_info(Monitor *mon, const QDict *qdict) { CPUArchState *env1 = mon_get_cpu(); @@ -1773,12 +1773,12 @@ static void tlb_info(Monitor *mon) } #endif -static void do_info_mtree(Monitor *mon) +static void do_info_mtree(Monitor *mon, const QDict *qdict) { mtree_info((fprintf_function)monitor_printf, mon); } -static void do_info_numa(Monitor *mon) +static void do_info_numa(Monitor *mon, const QDict *qdict) { int i; CPUArchState *env; @@ -1802,7 +1802,7 @@ static void do_info_numa(Monitor *mon) int64_t qemu_time; int64_t dev_time; -static void do_info_profile(Monitor *mon) +static void do_info_profile(Monitor *mon, const QDict *qdict) { int64_t total; total = qemu_time; @@ -1816,7 +1816,7 @@ static void do_info_profile(Monitor *mon) dev_time = 0; } #else -static void do_info_profile(Monitor *mon) +static void do_info_profile(Monitor *mon, const QDict *qdict) { monitor_printf(mon, "Internal profiler not compiled\n"); } @@ -1825,7 +1825,7 @@ static void do_info_profile(Monitor *mon) /* Capture support */ static QLIST_HEAD (capture_list_head, CaptureState) capture_head; -static void do_info_capture(Monitor *mon) +static void do_info_capture(Monitor *mon, const QDict *qdict) { int i; CaptureState *s; diff --git a/net.c b/net.c index e8ae13e..49735d1 100644 --- a/net.c +++ b/net.c @@ -851,7 +851,7 @@ void print_net_client(Monitor *mon, NetClientState *nc) NetClientOptionsKind_lookup[nc->info->type], nc->info_str); } -void do_info_network(Monitor *mon) +void do_info_network(Monitor *mon, const QDict *qdict) { NetClientState *nc, *peer; NetClientOptionsKind type; diff --git a/net.h b/net.h index 04fda1d..83f38ef 100644 --- a/net.h +++ b/net.h @@ -112,7 +112,7 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, void *opaque); void print_net_client(Monitor *mon, NetClientState *nc); -void do_info_network(Monitor *mon); +void do_info_network(Monitor *mon, const QDict *qdict); /* NIC info */ diff --git a/net/slirp.c b/net/slirp.c index afb52c3..8e3ec77 100644 --- a/net/slirp.c +++ b/net/slirp.c @@ -669,7 +669,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str, return -1; } -void do_info_usernet(Monitor *mon) +void do_info_usernet(Monitor *mon, const QDict *qdict) { SlirpState *s; diff --git a/net/slirp.h b/net/slirp.h index 2ca09b6..051b7c7 100644 --- a/net/slirp.h +++ b/net/slirp.h @@ -40,7 +40,7 @@ int net_slirp_parse_legacy(QemuOptsList *opts_list, const char *optarg, int *ret int net_slirp_smb(const char *exported_dir); -void do_info_usernet(Monitor *mon); +void do_info_usernet(Monitor *mon, const QDict *qdict); #endif diff --git a/savevm.c b/savevm.c index 5d04d59..fa32171 100644 --- a/savevm.c +++ b/savevm.c @@ -2358,7 +2358,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) } } -void do_info_snapshots(Monitor *mon) +void do_info_snapshots(Monitor *mon, const QDict *qdict) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; diff --git a/sysemu.h b/sysemu.h index 1b6add2..93147e1 100644 --- a/sysemu.h +++ b/sysemu.h @@ -68,7 +68,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify); void do_savevm(Monitor *mon, const QDict *qdict); int load_vmstate(const char *name); void do_delvm(Monitor *mon, const QDict *qdict); -void do_info_snapshots(Monitor *mon); +void do_info_snapshots(Monitor *mon, const QDict *qdict); void qemu_announce_self(void); @@ -170,7 +170,7 @@ extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS]; void do_usb_add(Monitor *mon, const QDict *qdict); void do_usb_del(Monitor *mon, const QDict *qdict); -void usb_info(Monitor *mon); +void usb_info(Monitor *mon, const QDict *qdict); void rtc_change_mon_event(struct tm *tm); diff --git a/vl.c b/vl.c index 3ebf01f..7263a02 100644 --- a/vl.c +++ b/vl.c @@ -1263,7 +1263,7 @@ void pcmcia_socket_unregister(PCMCIASocket *socket) } } -void pcmcia_info(Monitor *mon) +void pcmcia_info(Monitor *mon, const QDict *qdict) { struct pcmcia_socket_entry_s *iter; -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler 2012-12-19 10:17 ` [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler Wenchao Xia @ 2012-12-21 14:01 ` Markus Armbruster 2012-12-25 2:52 ` Wenchao Xia 0 siblings, 1 reply; 14+ messages in thread From: Markus Armbruster @ 2012-12-21 14:01 UTC (permalink / raw) To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, lcapitulino, pbonzini Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > This patch change all info call back function to take > additional QDict * parameter, which allow those command > take parameter. [...] > diff --git a/monitor.c b/monitor.c > index c0e32d6..797680f 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -123,7 +123,7 @@ typedef struct mon_cmd_t { > const char *help; > void (*user_print)(Monitor *mon, const QObject *data); > union { > - void (*info)(Monitor *mon); > + void (*info)(Monitor *mon, const QDict *qdict); > void (*cmd)(Monitor *mon, const QDict *qdict); > int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data); > int (*cmd_async)(Monitor *mon, const QDict *params, > @@ -824,7 +824,7 @@ static void do_info(Monitor *mon, const QDict *qdict) > goto help; > } > > - cmd->mhandler.info(mon); > + cmd->mhandler.info(mon, NULL); > return; > For now, the new argument is NULL, not an empty dictionary. Could be mentioned in the commit message. Not worth a respin. [...] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler 2012-12-21 14:01 ` Markus Armbruster @ 2012-12-25 2:52 ` Wenchao Xia 0 siblings, 0 replies; 14+ messages in thread From: Wenchao Xia @ 2012-12-25 2:52 UTC (permalink / raw) To: Markus Armbruster Cc: kwolf, aliguori, stefanha, qemu-devel, lcapitulino, pbonzini OK, will add that in commit message. > > For now, the new argument is NULL, not an empty dictionary. Could be > mentioned in the commit message. Not worth a respin. > > [...] > -- Best Regards Wenchao Xia ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-19 10:17 [Qemu-devel] [PATCH 0/3] HMP: enable info sub command taking parameter Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler Wenchao Xia @ 2012-12-19 10:17 ` Wenchao Xia 2012-12-19 18:07 ` Luiz Capitulino 2012-12-19 10:17 ` [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device Wenchao Xia 2 siblings, 1 reply; 14+ messages in thread From: Wenchao Xia @ 2012-12-19 10:17 UTC (permalink / raw) To: qemu-devel Cc: kwolf, aliguori, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia This patch enable sub info command handler getting meaningful parameter. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- hmp-commands.hx | 2 +- monitor.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- 2 files changed, 57 insertions(+), 24 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index 010b8c9..667fab8 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1486,7 +1486,7 @@ ETEXI { .name = "info", - .args_type = "item:s?", + .args_type = "item:S?", .params = "[subcommand]", .help = "show various information about the system state", .mhandler.cmd = do_info, diff --git a/monitor.c b/monitor.c index 797680f..ce0e74d 100644 --- a/monitor.c +++ b/monitor.c @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) MonitorEventState monitor_event_state[QEVENT_MAX]; QemuMutex monitor_event_state_lock; +static const mon_cmd_t *monitor_parse_command(Monitor *mon, + const char *cmdline, + const mon_cmd_t *table, + QDict *qdict); + /* * Emits the event to every monitor instance */ @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, static void do_info(Monitor *mon, const QDict *qdict) { const mon_cmd_t *cmd; + QDict *qdict_info; const char *item = qdict_get_try_str(qdict, "item"); if (!item) { goto help; } - for (cmd = info_cmds; cmd->name != NULL; cmd++) { - if (compare_cmd(item, cmd->name)) - break; - } + qdict_info = qdict_new(); - if (cmd->name == NULL) { - goto help; + cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); + if (!cmd) { + QDECREF(qdict_info); + /* don't help here, to avoid error message got ignored */ + return; } - cmd->mhandler.info(mon, NULL); + cmd->mhandler.info(mon, qdict_info); + QDECREF(qdict_info); return; help: help_cmd(mon, "info"); + return; } CommandInfoList *qmp_query_commands(Error **errp) @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, return NULL; } -static const mon_cmd_t *monitor_find_command(const char *cmdname) +static const mon_cmd_t *monitor_find_command(const char *cmdname, + const mon_cmd_t *table) { - return search_dispatch_table(mon_cmds, cmdname); -} - -static const mon_cmd_t *qmp_find_cmd(const char *cmdname) -{ - return search_dispatch_table(qmp_cmds, cmdname); + return search_dispatch_table(table, cmdname); } static const mon_cmd_t *monitor_parse_command(Monitor *mon, const char *cmdline, + const mon_cmd_t *table, QDict *qdict) { const char *p, *typestr; @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, if (!p) return NULL; - cmd = monitor_find_command(cmdname); + cmd = monitor_find_command(cmdname, table); if (!cmd) { monitor_printf(mon, "unknown command: '%s'\n", cmdname); return NULL; @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, } } break; + case 'S': + { + /* package all remaining string */ + int len; + + while (qemu_isspace(*p)) { + p++; + } + if (*typestr == '?') { + typestr++; + if (*p == '\0') { + /* no remaining string: NULL argument */ + break; + } + } + len = strlen(p); + if (len <= 0) { + monitor_printf(mon, "%s: string expected\n", + cmdname); + break; + } + qdict_put(qdict, key, qstring_from_str(p)); + p += len; + } + break; default: bad_type: monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) qdict = qdict_new(); - cmd = monitor_parse_command(mon, cmdline, qdict); + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); if (!cmd) goto out; @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline) break; case 's': /* XXX: more generic ? */ - if (!strcmp(cmd->name, "info")) { - readline_set_completion_index(cur_mon->rs, strlen(str)); - for(cmd = info_cmds; cmd->name != NULL; cmd++) { - cmd_completion(str, cmd->name); - } - } else if (!strcmp(cmd->name, "sendkey")) { + if (!strcmp(cmd->name, "sendkey")) { char *sep = strrchr(str, '-'); if (sep) str = sep + 1; @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline) cmd_completion(str, cmd->name); } } + case 'S': + /* generic string packaged */ + if (!strcmp(cmd->name, "info")) { + readline_set_completion_index(cur_mon->rs, strlen(str)); + for (cmd = info_cmds; cmd->name != NULL; cmd++) { + cmd_completion(str, cmd->name); + } + } break; default: break; @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; } - cmd = qmp_find_cmd(cmd_name); + cmd = monitor_find_command(cmd_name, qmp_cmds); if (!cmd) { qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); goto err_out; -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-19 10:17 ` [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command Wenchao Xia @ 2012-12-19 18:07 ` Luiz Capitulino 2012-12-20 3:02 ` Wenchao Xia 2012-12-21 14:49 ` Markus Armbruster 0 siblings, 2 replies; 14+ messages in thread From: Luiz Capitulino @ 2012-12-19 18:07 UTC (permalink / raw) To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini On Wed, 19 Dec 2012 18:17:09 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > This patch enable sub info command handler getting meaningful > parameter. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > hmp-commands.hx | 2 +- > monitor.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- > 2 files changed, 57 insertions(+), 24 deletions(-) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 010b8c9..667fab8 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1486,7 +1486,7 @@ ETEXI > > { > .name = "info", > - .args_type = "item:s?", > + .args_type = "item:S?", > .params = "[subcommand]", > .help = "show various information about the system state", > .mhandler.cmd = do_info, > diff --git a/monitor.c b/monitor.c > index 797680f..ce0e74d 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) > MonitorEventState monitor_event_state[QEVENT_MAX]; > QemuMutex monitor_event_state_lock; > > +static const mon_cmd_t *monitor_parse_command(Monitor *mon, > + const char *cmdline, > + const mon_cmd_t *table, > + QDict *qdict); Please, move the function instead. > + > /* > * Emits the event to every monitor instance > */ > @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, > static void do_info(Monitor *mon, const QDict *qdict) > { > const mon_cmd_t *cmd; > + QDict *qdict_info; > const char *item = qdict_get_try_str(qdict, "item"); > > if (!item) { > goto help; > } > > - for (cmd = info_cmds; cmd->name != NULL; cmd++) { > - if (compare_cmd(item, cmd->name)) > - break; > - } > + qdict_info = qdict_new(); > > - if (cmd->name == NULL) { > - goto help; > + cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); > + if (!cmd) { > + QDECREF(qdict_info); > + /* don't help here, to avoid error message got ignored */ > + return; > } > > - cmd->mhandler.info(mon, NULL); > + cmd->mhandler.info(mon, qdict_info); > + QDECREF(qdict_info); > return; > > help: > help_cmd(mon, "info"); > + return; > } > > CommandInfoList *qmp_query_commands(Error **errp) > @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, > return NULL; > } > > -static const mon_cmd_t *monitor_find_command(const char *cmdname) > +static const mon_cmd_t *monitor_find_command(const char *cmdname, > + const mon_cmd_t *table) > { > - return search_dispatch_table(mon_cmds, cmdname); > -} > - > -static const mon_cmd_t *qmp_find_cmd(const char *cmdname) > -{ > - return search_dispatch_table(qmp_cmds, cmdname); > + return search_dispatch_table(table, cmdname); Please, keep only search_dispatch_table(). Actually, maybe you could change handle_qmp_command() to just loop through command names and compare them with memcpy() (in a different patch, please) then you keep monitor_find_command() for handle_hmp_command(). > } > > static const mon_cmd_t *monitor_parse_command(Monitor *mon, > const char *cmdline, > + const mon_cmd_t *table, > QDict *qdict) > { > const char *p, *typestr; > @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > if (!p) > return NULL; > > - cmd = monitor_find_command(cmdname); > + cmd = monitor_find_command(cmdname, table); > if (!cmd) { > monitor_printf(mon, "unknown command: '%s'\n", cmdname); > return NULL; > @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > } > } > break; > + case 'S': > + { > + /* package all remaining string */ > + int len; > + > + while (qemu_isspace(*p)) { > + p++; > + } > + if (*typestr == '?') { > + typestr++; > + if (*p == '\0') { > + /* no remaining string: NULL argument */ > + break; > + } > + } > + len = strlen(p); > + if (len <= 0) { > + monitor_printf(mon, "%s: string expected\n", > + cmdname); > + break; > + } > + qdict_put(qdict, key, qstring_from_str(p)); > + p += len; > + } This is a true HMP-style hack :) I don't know how to make this better though (at least not without asking you to re-write the whole thing). Maybe Markus has a better idea? > + break; > default: > bad_type: > monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); > @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) > > qdict = qdict_new(); > > - cmd = monitor_parse_command(mon, cmdline, qdict); > + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); > if (!cmd) > goto out; > > @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline) > break; > case 's': > /* XXX: more generic ? */ > - if (!strcmp(cmd->name, "info")) { > - readline_set_completion_index(cur_mon->rs, strlen(str)); > - for(cmd = info_cmds; cmd->name != NULL; cmd++) { > - cmd_completion(str, cmd->name); > - } > - } else if (!strcmp(cmd->name, "sendkey")) { > + if (!strcmp(cmd->name, "sendkey")) { > char *sep = strrchr(str, '-'); > if (sep) > str = sep + 1; > @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline) > cmd_completion(str, cmd->name); > } > } > + case 'S': > + /* generic string packaged */ > + if (!strcmp(cmd->name, "info")) { > + readline_set_completion_index(cur_mon->rs, strlen(str)); > + for (cmd = info_cmds; cmd->name != NULL; cmd++) { > + cmd_completion(str, cmd->name); > + } > + } > break; > default: > break; > @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) > goto err_out; > } > > - cmd = qmp_find_cmd(cmd_name); > + cmd = monitor_find_command(cmd_name, qmp_cmds); > if (!cmd) { > qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); > goto err_out; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-19 18:07 ` Luiz Capitulino @ 2012-12-20 3:02 ` Wenchao Xia 2012-12-22 21:36 ` Luiz Capitulino 2012-12-21 14:49 ` Markus Armbruster 1 sibling, 1 reply; 14+ messages in thread From: Wenchao Xia @ 2012-12-20 3:02 UTC (permalink / raw) To: Luiz Capitulino; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini > On Wed, 19 Dec 2012 18:17:09 +0800 > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > >> This patch enable sub info command handler getting meaningful >> parameter. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> hmp-commands.hx | 2 +- >> monitor.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- >> 2 files changed, 57 insertions(+), 24 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index 010b8c9..667fab8 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1486,7 +1486,7 @@ ETEXI >> >> { >> .name = "info", >> - .args_type = "item:s?", >> + .args_type = "item:S?", >> .params = "[subcommand]", >> .help = "show various information about the system state", >> .mhandler.cmd = do_info, >> diff --git a/monitor.c b/monitor.c >> index 797680f..ce0e74d 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) >> MonitorEventState monitor_event_state[QEVENT_MAX]; >> QemuMutex monitor_event_state_lock; >> >> +static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> + const char *cmdline, >> + const mon_cmd_t *table, >> + QDict *qdict); > > Please, move the function instead. > There will be a bit big of code moving then, hope you would be OK with it. >> + >> /* >> * Emits the event to every monitor instance >> */ >> @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, >> static void do_info(Monitor *mon, const QDict *qdict) >> { >> const mon_cmd_t *cmd; >> + QDict *qdict_info; >> const char *item = qdict_get_try_str(qdict, "item"); >> >> if (!item) { >> goto help; >> } >> >> - for (cmd = info_cmds; cmd->name != NULL; cmd++) { >> - if (compare_cmd(item, cmd->name)) >> - break; >> - } >> + qdict_info = qdict_new(); >> >> - if (cmd->name == NULL) { >> - goto help; >> + cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); >> + if (!cmd) { >> + QDECREF(qdict_info); >> + /* don't help here, to avoid error message got ignored */ >> + return; >> } >> >> - cmd->mhandler.info(mon, NULL); >> + cmd->mhandler.info(mon, qdict_info); >> + QDECREF(qdict_info); >> return; >> >> help: >> help_cmd(mon, "info"); >> + return; >> } >> >> CommandInfoList *qmp_query_commands(Error **errp) >> @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, >> return NULL; >> } >> >> -static const mon_cmd_t *monitor_find_command(const char *cmdname) >> +static const mon_cmd_t *monitor_find_command(const char *cmdname, >> + const mon_cmd_t *table) >> { >> - return search_dispatch_table(mon_cmds, cmdname); >> -} >> - >> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname) >> -{ >> - return search_dispatch_table(qmp_cmds, cmdname); >> + return search_dispatch_table(table, cmdname); > > Please, keep only search_dispatch_table(). > OK. > Actually, maybe you could change handle_qmp_command() to just loop through > command names and compare them with memcpy() (in a different patch, please) > then you keep monitor_find_command() for handle_hmp_command(). > ah, then monitor_find_command() serve HMP only. How about rename it to hmp_find_command(table, name)? This can have a more clear meaning. Or just remove this funtion but use search_dispatch_table() every where? >> } >> >> static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> const char *cmdline, >> + const mon_cmd_t *table, >> QDict *qdict) >> { >> const char *p, *typestr; >> @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> if (!p) >> return NULL; >> >> - cmd = monitor_find_command(cmdname); >> + cmd = monitor_find_command(cmdname, table); >> if (!cmd) { >> monitor_printf(mon, "unknown command: '%s'\n", cmdname); >> return NULL; >> @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> } >> } >> break; >> + case 'S': >> + { >> + /* package all remaining string */ >> + int len; >> + >> + while (qemu_isspace(*p)) { >> + p++; >> + } >> + if (*typestr == '?') { >> + typestr++; >> + if (*p == '\0') { >> + /* no remaining string: NULL argument */ >> + break; >> + } >> + } >> + len = strlen(p); >> + if (len <= 0) { >> + monitor_printf(mon, "%s: string expected\n", >> + cmdname); >> + break; >> + } >> + qdict_put(qdict, key, qstring_from_str(p)); >> + p += len; >> + } > > This is a true HMP-style hack :) > > I don't know how to make this better though (at least not without asking you > to re-write the whole thing). Maybe Markus has a better idea? > My idea was adding a new tag meaning "package all remaining string into the key value", what was I can found best before. Hope you have a better solution. >> + break; >> default: >> bad_type: >> monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); >> @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) >> >> qdict = qdict_new(); >> >> - cmd = monitor_parse_command(mon, cmdline, qdict); >> + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); >> if (!cmd) >> goto out; >> >> @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline) >> break; >> case 's': >> /* XXX: more generic ? */ >> - if (!strcmp(cmd->name, "info")) { >> - readline_set_completion_index(cur_mon->rs, strlen(str)); >> - for(cmd = info_cmds; cmd->name != NULL; cmd++) { >> - cmd_completion(str, cmd->name); >> - } >> - } else if (!strcmp(cmd->name, "sendkey")) { >> + if (!strcmp(cmd->name, "sendkey")) { >> char *sep = strrchr(str, '-'); >> if (sep) >> str = sep + 1; >> @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline) >> cmd_completion(str, cmd->name); >> } >> } >> + case 'S': >> + /* generic string packaged */ >> + if (!strcmp(cmd->name, "info")) { >> + readline_set_completion_index(cur_mon->rs, strlen(str)); >> + for (cmd = info_cmds; cmd->name != NULL; cmd++) { >> + cmd_completion(str, cmd->name); >> + } >> + } >> break; >> default: >> break; >> @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) >> goto err_out; >> } >> >> - cmd = qmp_find_cmd(cmd_name); >> + cmd = monitor_find_command(cmd_name, qmp_cmds); >> if (!cmd) { >> qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); >> goto err_out; > > -- Best Regards Wenchao Xia ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-20 3:02 ` Wenchao Xia @ 2012-12-22 21:36 ` Luiz Capitulino 0 siblings, 0 replies; 14+ messages in thread From: Luiz Capitulino @ 2012-12-22 21:36 UTC (permalink / raw) To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini On Thu, 20 Dec 2012 11:02:16 +0800 Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > > On Wed, 19 Dec 2012 18:17:09 +0800 > > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > > > >> This patch enable sub info command handler getting meaningful > >> parameter. > >> > >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > >> --- > >> hmp-commands.hx | 2 +- > >> monitor.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- > >> 2 files changed, 57 insertions(+), 24 deletions(-) > >> > >> diff --git a/hmp-commands.hx b/hmp-commands.hx > >> index 010b8c9..667fab8 100644 > >> --- a/hmp-commands.hx > >> +++ b/hmp-commands.hx > >> @@ -1486,7 +1486,7 @@ ETEXI > >> > >> { > >> .name = "info", > >> - .args_type = "item:s?", > >> + .args_type = "item:S?", > >> .params = "[subcommand]", > >> .help = "show various information about the system state", > >> .mhandler.cmd = do_info, > >> diff --git a/monitor.c b/monitor.c > >> index 797680f..ce0e74d 100644 > >> --- a/monitor.c > >> +++ b/monitor.c > >> @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) > >> MonitorEventState monitor_event_state[QEVENT_MAX]; > >> QemuMutex monitor_event_state_lock; > >> > >> +static const mon_cmd_t *monitor_parse_command(Monitor *mon, > >> + const char *cmdline, > >> + const mon_cmd_t *table, > >> + QDict *qdict); > > > > Please, move the function instead. > > > There will be a bit big of code moving then, hope you would be OK with > it. That's fine, but please do it in a different patch(es). > >> + > >> /* > >> * Emits the event to every monitor instance > >> */ > >> @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, > >> static void do_info(Monitor *mon, const QDict *qdict) > >> { > >> const mon_cmd_t *cmd; > >> + QDict *qdict_info; > >> const char *item = qdict_get_try_str(qdict, "item"); > >> > >> if (!item) { > >> goto help; > >> } > >> > >> - for (cmd = info_cmds; cmd->name != NULL; cmd++) { > >> - if (compare_cmd(item, cmd->name)) > >> - break; > >> - } > >> + qdict_info = qdict_new(); > >> > >> - if (cmd->name == NULL) { > >> - goto help; > >> + cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); > >> + if (!cmd) { > >> + QDECREF(qdict_info); > >> + /* don't help here, to avoid error message got ignored */ > >> + return; > >> } > >> > >> - cmd->mhandler.info(mon, NULL); > >> + cmd->mhandler.info(mon, qdict_info); > >> + QDECREF(qdict_info); > >> return; > >> > >> help: > >> help_cmd(mon, "info"); > >> + return; > >> } > >> > >> CommandInfoList *qmp_query_commands(Error **errp) > >> @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, > >> return NULL; > >> } > >> > >> -static const mon_cmd_t *monitor_find_command(const char *cmdname) > >> +static const mon_cmd_t *monitor_find_command(const char *cmdname, > >> + const mon_cmd_t *table) > >> { > >> - return search_dispatch_table(mon_cmds, cmdname); > >> -} > >> - > >> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname) > >> -{ > >> - return search_dispatch_table(qmp_cmds, cmdname); > >> + return search_dispatch_table(table, cmdname); > > > > Please, keep only search_dispatch_table(). > > > OK. > > > Actually, maybe you could change handle_qmp_command() to just loop through > > command names and compare them with memcpy() (in a different patch, please) > > then you keep monitor_find_command() for handle_hmp_command(). > > > ah, then monitor_find_command() serve HMP only. How about rename it to > hmp_find_command(table, name)? This can have a more clear meaning. Or That would be fine. > just remove this funtion but use search_dispatch_table() every where? > > >> } > >> > >> static const mon_cmd_t *monitor_parse_command(Monitor *mon, > >> const char *cmdline, > >> + const mon_cmd_t *table, > >> QDict *qdict) > >> { > >> const char *p, *typestr; > >> @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > >> if (!p) > >> return NULL; > >> > >> - cmd = monitor_find_command(cmdname); > >> + cmd = monitor_find_command(cmdname, table); > >> if (!cmd) { > >> monitor_printf(mon, "unknown command: '%s'\n", cmdname); > >> return NULL; > >> @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > >> } > >> } > >> break; > >> + case 'S': > >> + { > >> + /* package all remaining string */ > >> + int len; > >> + > >> + while (qemu_isspace(*p)) { > >> + p++; > >> + } > >> + if (*typestr == '?') { > >> + typestr++; > >> + if (*p == '\0') { > >> + /* no remaining string: NULL argument */ > >> + break; > >> + } > >> + } > >> + len = strlen(p); > >> + if (len <= 0) { > >> + monitor_printf(mon, "%s: string expected\n", > >> + cmdname); > >> + break; > >> + } > >> + qdict_put(qdict, key, qstring_from_str(p)); > >> + p += len; > >> + } > > > > This is a true HMP-style hack :) > > > > I don't know how to make this better though (at least not without asking you > > to re-write the whole thing). Maybe Markus has a better idea? > > > My idea was adding a new tag meaning "package all remaining string > into the key value", what was I can found best before. Hope you have > a better solution. > > >> + break; > >> default: > >> bad_type: > >> monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); > >> @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) > >> > >> qdict = qdict_new(); > >> > >> - cmd = monitor_parse_command(mon, cmdline, qdict); > >> + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); > >> if (!cmd) > >> goto out; > >> > >> @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline) > >> break; > >> case 's': > >> /* XXX: more generic ? */ > >> - if (!strcmp(cmd->name, "info")) { > >> - readline_set_completion_index(cur_mon->rs, strlen(str)); > >> - for(cmd = info_cmds; cmd->name != NULL; cmd++) { > >> - cmd_completion(str, cmd->name); > >> - } > >> - } else if (!strcmp(cmd->name, "sendkey")) { > >> + if (!strcmp(cmd->name, "sendkey")) { > >> char *sep = strrchr(str, '-'); > >> if (sep) > >> str = sep + 1; > >> @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline) > >> cmd_completion(str, cmd->name); > >> } > >> } > >> + case 'S': > >> + /* generic string packaged */ > >> + if (!strcmp(cmd->name, "info")) { > >> + readline_set_completion_index(cur_mon->rs, strlen(str)); > >> + for (cmd = info_cmds; cmd->name != NULL; cmd++) { > >> + cmd_completion(str, cmd->name); > >> + } > >> + } > >> break; > >> default: > >> break; > >> @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) > >> goto err_out; > >> } > >> > >> - cmd = qmp_find_cmd(cmd_name); > >> + cmd = monitor_find_command(cmd_name, qmp_cmds); > >> if (!cmd) { > >> qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); > >> goto err_out; > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-19 18:07 ` Luiz Capitulino 2012-12-20 3:02 ` Wenchao Xia @ 2012-12-21 14:49 ` Markus Armbruster 2012-12-21 18:17 ` Eric Blake 2012-12-25 4:29 ` Wenchao Xia 1 sibling, 2 replies; 14+ messages in thread From: Markus Armbruster @ 2012-12-21 14:49 UTC (permalink / raw) To: Luiz Capitulino Cc: kwolf, aliguori, stefanha, qemu-devel, pbonzini, Wenchao Xia Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 19 Dec 2012 18:17:09 +0800 > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: > >> This patch enable sub info command handler getting meaningful >> parameter. >> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >> --- >> hmp-commands.hx | 2 +- >> monitor.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- >> 2 files changed, 57 insertions(+), 24 deletions(-) >> >> diff --git a/hmp-commands.hx b/hmp-commands.hx >> index 010b8c9..667fab8 100644 >> --- a/hmp-commands.hx >> +++ b/hmp-commands.hx >> @@ -1486,7 +1486,7 @@ ETEXI >> >> { >> .name = "info", >> - .args_type = "item:s?", >> + .args_type = "item:S?", >> .params = "[subcommand]", >> .help = "show various information about the system state", >> .mhandler.cmd = do_info, >> diff --git a/monitor.c b/monitor.c >> index 797680f..ce0e74d 100644 >> --- a/monitor.c >> +++ b/monitor.c Missing: documentation for the new arg type S in the comment that starts with "Supported types". >> @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) >> MonitorEventState monitor_event_state[QEVENT_MAX]; >> QemuMutex monitor_event_state_lock; >> >> +static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> + const char *cmdline, >> + const mon_cmd_t *table, >> + QDict *qdict); > > Please, move the function instead. Move will make review harder. I don't mind forward declarations. >> + >> /* >> * Emits the event to every monitor instance >> */ >> @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, >> static void do_info(Monitor *mon, const QDict *qdict) >> { >> const mon_cmd_t *cmd; >> + QDict *qdict_info; >> const char *item = qdict_get_try_str(qdict, "item"); >> >> if (!item) { >> goto help; >> } >> >> - for (cmd = info_cmds; cmd->name != NULL; cmd++) { >> - if (compare_cmd(item, cmd->name)) >> - break; >> - } >> + qdict_info = qdict_new(); >> >> - if (cmd->name == NULL) { >> - goto help; >> + cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); >> + if (!cmd) { >> + QDECREF(qdict_info); >> + /* don't help here, to avoid error message got ignored */ I'm afraid I don't understand your comment. Oh, you mean you don't want to call help_cmd() here! >> + return; >> } >> >> - cmd->mhandler.info(mon, NULL); >> + cmd->mhandler.info(mon, qdict_info); >> + QDECREF(qdict_info); >> return; >> >> help: >> help_cmd(mon, "info"); >> + return; >> } The function now looks like this: static void do_info(Monitor *mon, const QDict *qdict) { const mon_cmd_t *cmd; QDict *qdict_info; const char *item = qdict_get_try_str(qdict, "item"); if (!item) { goto help; } qdict_info = qdict_new(); cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); if (!cmd) { QDECREF(qdict_info); /* don't help here, to avoid error message got ignored */ return; } cmd->mhandler.info(mon, qdict_info); QDECREF(qdict_info); return; help: help_cmd(mon, "info"); return; } Control flow isn't nice. What about: static void do_info(Monitor *mon, const QDict *qdict) { const mon_cmd_t *cmd; QDict *qdict_info; const char *item = qdict_get_try_str(qdict, "item"); if (!item) { help_cmd(mon, "info"); return; } qdict_info = qdict_new(); cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); if (!cmd) { goto out; } cmd->mhandler.info(mon, qdict_info); out: QDECREF(qdict_info); return; } >> >> CommandInfoList *qmp_query_commands(Error **errp) >> @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, >> return NULL; >> } >> >> -static const mon_cmd_t *monitor_find_command(const char *cmdname) >> +static const mon_cmd_t *monitor_find_command(const char *cmdname, >> + const mon_cmd_t *table) >> { >> - return search_dispatch_table(mon_cmds, cmdname); >> -} >> - >> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname) >> -{ >> - return search_dispatch_table(qmp_cmds, cmdname); >> + return search_dispatch_table(table, cmdname); > > Please, keep only search_dispatch_table(). Yes, because the resulting function is silly :) static const mon_cmd_t *monitor_find_command(const char *cmdname, const mon_cmd_t *table) { return search_dispatch_table(table, cmdname); } > Actually, maybe you could change handle_qmp_command() to just loop through > command names and compare them with memcpy() (in a different patch, please) > then you keep monitor_find_command() for handle_hmp_command(). > >> } >> >> static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> const char *cmdline, >> + const mon_cmd_t *table, >> QDict *qdict) >> { >> const char *p, *typestr; >> @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> if (!p) >> return NULL; >> >> - cmd = monitor_find_command(cmdname); >> + cmd = monitor_find_command(cmdname, table); >> if (!cmd) { >> monitor_printf(mon, "unknown command: '%s'\n", cmdname); >> return NULL; >> @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >> } >> } >> break; >> + case 'S': >> + { >> + /* package all remaining string */ >> + int len; >> + >> + while (qemu_isspace(*p)) { >> + p++; >> + } >> + if (*typestr == '?') { >> + typestr++; >> + if (*p == '\0') { >> + /* no remaining string: NULL argument */ >> + break; >> + } >> + } >> + len = strlen(p); >> + if (len <= 0) { >> + monitor_printf(mon, "%s: string expected\n", >> + cmdname); A "string" in this context is an argument for arg type 's', i.e. a sequence of characters delimited by unescaped '"', or a sequence of non-whitespace characters not starting with '"'. That's not what we expect here. We expect arbitrary arguments. Suggest "arguments expected". >> + break; >> + } >> + qdict_put(qdict, key, qstring_from_str(p)); >> + p += len; >> + } > > This is a true HMP-style hack :) > > I don't know how to make this better though (at least not without asking you > to re-write the whole thing). Maybe Markus has a better idea? Let me try :) The new arg type 'S' consumes the rest of the line unparsed, and puts it into the argument qdict. Has to come last, obviously. do_info() extracts it, then parses it like a full command. The info command effectively adds like a prefix that switches command parsing to an alternate table of commands. Works, quite flexible, but pretty arcane. Try #1: Implement the command prefix concept the direct way. Mark the command as prefix. Instead of a handler, it gets a pointer to the alternate table of commands. When monitor_parse_command() recognizes a prefix command, it drops the first word, switches to the alternate table, and starts over. Try #2: We already have commands that take key=value,... arguments (arg type 'O'): device_add and netdev_add. Perhaps info could use args_type "item:s?,opts:O". First argument is unchanged. The new second argument accepts key=value,... options. 'O' argument is always optional. One key can be declared optional, so that value (no '=') is shorthand for that key=value. >> + break; >> default: >> bad_type: >> monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); >> @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) >> >> qdict = qdict_new(); >> >> - cmd = monitor_parse_command(mon, cmdline, qdict); >> + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); >> if (!cmd) >> goto out; >> >> @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline) >> break; >> case 's': >> /* XXX: more generic ? */ >> - if (!strcmp(cmd->name, "info")) { >> - readline_set_completion_index(cur_mon->rs, strlen(str)); >> - for(cmd = info_cmds; cmd->name != NULL; cmd++) { >> - cmd_completion(str, cmd->name); >> - } >> - } else if (!strcmp(cmd->name, "sendkey")) { >> + if (!strcmp(cmd->name, "sendkey")) { >> char *sep = strrchr(str, '-'); >> if (sep) >> str = sep + 1; You move the special case hack for info argument completion to case 'S' (next hunk), but leave behind the /* XXX: more generic ? */ that marks it as hack! Please move it along with the hack. >> @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline) >> cmd_completion(str, cmd->name); >> } >> } >> + case 'S': >> + /* generic string packaged */ >> + if (!strcmp(cmd->name, "info")) { >> + readline_set_completion_index(cur_mon->rs, strlen(str)); >> + for (cmd = info_cmds; cmd->name != NULL; cmd++) { >> + cmd_completion(str, cmd->name); >> + } >> + } >> break; >> default: >> break; >> @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) >> goto err_out; >> } >> >> - cmd = qmp_find_cmd(cmd_name); >> + cmd = monitor_find_command(cmd_name, qmp_cmds); >> if (!cmd) { >> qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); >> goto err_out; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-21 14:49 ` Markus Armbruster @ 2012-12-21 18:17 ` Eric Blake 2012-12-25 4:29 ` Wenchao Xia 1 sibling, 0 replies; 14+ messages in thread From: Eric Blake @ 2012-12-21 18:17 UTC (permalink / raw) To: Markus Armbruster Cc: kwolf, aliguori, stefanha, qemu-devel, Luiz Capitulino, pbonzini, Wenchao Xia [-- Attachment #1: Type: text/plain, Size: 720 bytes --] On 12/21/2012 07:49 AM, Markus Armbruster wrote: >>> +static const mon_cmd_t *monitor_parse_command(Monitor *mon, >>> + const char *cmdline, >>> + const mon_cmd_t *table, >>> + QDict *qdict); >> >> Please, move the function instead. > > Move will make review harder. I don't mind forward declarations. If you do move the function, then split this into two patches - one for JUST the code motion, and another that adds the 'table' argument to the function. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 619 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command 2012-12-21 14:49 ` Markus Armbruster 2012-12-21 18:17 ` Eric Blake @ 2012-12-25 4:29 ` Wenchao Xia 1 sibling, 0 replies; 14+ messages in thread From: Wenchao Xia @ 2012-12-25 4:29 UTC (permalink / raw) To: Markus Armbruster Cc: kwolf, aliguori, stefanha, qemu-devel, Luiz Capitulino, pbonzini 于 2012-12-21 22:49, Markus Armbruster 写道: > Luiz Capitulino <lcapitulino@redhat.com> writes: > >> On Wed, 19 Dec 2012 18:17:09 +0800 >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote: >> >>> This patch enable sub info command handler getting meaningful >>> parameter. >>> >>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> >>> --- >>> hmp-commands.hx | 2 +- >>> monitor.c | 79 +++++++++++++++++++++++++++++++++++++++---------------- >>> 2 files changed, 57 insertions(+), 24 deletions(-) >>> >>> diff --git a/hmp-commands.hx b/hmp-commands.hx >>> index 010b8c9..667fab8 100644 >>> --- a/hmp-commands.hx >>> +++ b/hmp-commands.hx >>> @@ -1486,7 +1486,7 @@ ETEXI >>> >>> { >>> .name = "info", >>> - .args_type = "item:s?", >>> + .args_type = "item:S?", >>> .params = "[subcommand]", >>> .help = "show various information about the system state", >>> .mhandler.cmd = do_info, >>> diff --git a/monitor.c b/monitor.c >>> index 797680f..ce0e74d 100644 >>> --- a/monitor.c >>> +++ b/monitor.c > > Missing: documentation for the new arg type S in the comment that starts > with "Supported types". > OK, will add it. >>> @@ -464,6 +464,11 @@ QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX) >>> MonitorEventState monitor_event_state[QEVENT_MAX]; >>> QemuMutex monitor_event_state_lock; >>> >>> +static const mon_cmd_t *monitor_parse_command(Monitor *mon, >>> + const char *cmdline, >>> + const mon_cmd_t *table, >>> + QDict *qdict); >> >> Please, move the function instead. > > Move will make review harder. I don't mind forward declarations. > I will move the function in a separate patch. >>> + >>> /* >>> * Emits the event to every monitor instance >>> */ >>> @@ -809,26 +814,29 @@ static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, >>> static void do_info(Monitor *mon, const QDict *qdict) >>> { >>> const mon_cmd_t *cmd; >>> + QDict *qdict_info; >>> const char *item = qdict_get_try_str(qdict, "item"); >>> >>> if (!item) { >>> goto help; >>> } >>> >>> - for (cmd = info_cmds; cmd->name != NULL; cmd++) { >>> - if (compare_cmd(item, cmd->name)) >>> - break; >>> - } >>> + qdict_info = qdict_new(); >>> >>> - if (cmd->name == NULL) { >>> - goto help; >>> + cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); >>> + if (!cmd) { >>> + QDECREF(qdict_info); >>> + /* don't help here, to avoid error message got ignored */ > > I'm afraid I don't understand your comment. > > Oh, you mean you don't want to call help_cmd() here! > Yes, not to show the help contents, same as not call help_cmd(). >>> + return; >>> } >>> >>> - cmd->mhandler.info(mon, NULL); >>> + cmd->mhandler.info(mon, qdict_info); >>> + QDECREF(qdict_info); >>> return; >>> >>> help: >>> help_cmd(mon, "info"); >>> + return; >>> } > > The function now looks like this: > > static void do_info(Monitor *mon, const QDict *qdict) > { > const mon_cmd_t *cmd; > QDict *qdict_info; > const char *item = qdict_get_try_str(qdict, "item"); > > if (!item) { > goto help; > } > > qdict_info = qdict_new(); > > cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); > if (!cmd) { > QDECREF(qdict_info); > /* don't help here, to avoid error message got ignored */ > return; > } > > cmd->mhandler.info(mon, qdict_info); > QDECREF(qdict_info); > return; > > help: > help_cmd(mon, "info"); > return; > } > > Control flow isn't nice. What about: > > static void do_info(Monitor *mon, const QDict *qdict) > { > const mon_cmd_t *cmd; > QDict *qdict_info; > const char *item = qdict_get_try_str(qdict, "item"); > > if (!item) { > help_cmd(mon, "info"); > return; > } > > qdict_info = qdict_new(); > > cmd = monitor_parse_command(mon, item, info_cmds, qdict_info); > if (!cmd) { > goto out; > } > > cmd->mhandler.info(mon, qdict_info); > out: > QDECREF(qdict_info); > return; > } > OK, I'll following this way. >>> >>> CommandInfoList *qmp_query_commands(Error **errp) >>> @@ -3534,18 +3542,15 @@ static const mon_cmd_t *search_dispatch_table(const mon_cmd_t *disp_table, >>> return NULL; >>> } >>> >>> -static const mon_cmd_t *monitor_find_command(const char *cmdname) >>> +static const mon_cmd_t *monitor_find_command(const char *cmdname, >>> + const mon_cmd_t *table) >>> { >>> - return search_dispatch_table(mon_cmds, cmdname); >>> -} >>> - >>> -static const mon_cmd_t *qmp_find_cmd(const char *cmdname) >>> -{ >>> - return search_dispatch_table(qmp_cmds, cmdname); >>> + return search_dispatch_table(table, cmdname); >> >> Please, keep only search_dispatch_table(). > > Yes, because the resulting function is silly :) > > static const mon_cmd_t *monitor_find_command(const char *cmdname, > const mon_cmd_t *table) > { > return search_dispatch_table(table, cmdname); > } > I'll remove simple encapsulate function monitor_find_command(), which's naming make me confused. >> Actually, maybe you could change handle_qmp_command() to just loop through >> command names and compare them with memcpy() (in a different patch, please) >> then you keep monitor_find_command() for handle_hmp_command(). >> >>> } >>> >>> static const mon_cmd_t *monitor_parse_command(Monitor *mon, >>> const char *cmdline, >>> + const mon_cmd_t *table, >>> QDict *qdict) >>> { >>> const char *p, *typestr; >>> @@ -3564,7 +3569,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >>> if (!p) >>> return NULL; >>> >>> - cmd = monitor_find_command(cmdname); >>> + cmd = monitor_find_command(cmdname, table); >>> if (!cmd) { >>> monitor_printf(mon, "unknown command: '%s'\n", cmdname); >>> return NULL; >>> @@ -3872,6 +3877,31 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, >>> } >>> } >>> break; >>> + case 'S': >>> + { >>> + /* package all remaining string */ >>> + int len; >>> + >>> + while (qemu_isspace(*p)) { >>> + p++; >>> + } >>> + if (*typestr == '?') { >>> + typestr++; >>> + if (*p == '\0') { >>> + /* no remaining string: NULL argument */ >>> + break; >>> + } >>> + } >>> + len = strlen(p); >>> + if (len <= 0) { >>> + monitor_printf(mon, "%s: string expected\n", >>> + cmdname); > > A "string" in this context is an argument for arg type 's', i.e. a > sequence of characters delimited by unescaped '"', or a sequence of > non-whitespace characters not starting with '"'. That's not what we > expect here. We expect arbitrary arguments. > > Suggest "arguments expected". > OK. >>> + break; >>> + } >>> + qdict_put(qdict, key, qstring_from_str(p)); >>> + p += len; >>> + } >> >> This is a true HMP-style hack :) >> >> I don't know how to make this better though (at least not without asking you >> to re-write the whole thing). Maybe Markus has a better idea? > > Let me try :) > > The new arg type 'S' consumes the rest of the line unparsed, and puts it > into the argument qdict. Has to come last, obviously. > > do_info() extracts it, then parses it like a full command. The info > command effectively adds like a prefix that switches command parsing to > an alternate table of commands. Works, quite flexible, but pretty > arcane. > > Try #1: > > Implement the command prefix concept the direct way. Mark the command > as prefix. Instead of a handler, it gets a pointer to the alternate > table of commands. When monitor_parse_command() recognizes a prefix > command, it drops the first word, switches to the alternate table, and > starts over. > It seems "info" command need to be marked and searched in monitor_parse_command(), > Try #2: > > We already have commands that take key=value,... arguments (arg type > 'O'): device_add and netdev_add. Perhaps info could use args_type > "item:s?,opts:O". First argument is unchanged. The new second argument > accepts key=value,... options. 'O' argument is always optional. One > key can be declared optional, so that value (no '=') is shorthand for > that key=value. Personally I feel approach #1 is better, which let the command parse layer knows there would be another sub command layer, and enable embbed sub commands with deeper layers transparently. #2 and my previous patch is a hack but each layer don't know sub command exist. For #1 I guess additional tag about "sub command' need to be added, I guess that would be typedef struct mon_cmd_t { const char *name; const char *args_type; const char *params; const char *help; void (*user_print)(Monitor *mon, const QObject *data); union { void (*info)(Monitor *mon); void (*cmd)(Monitor *mon, const QDict *qdict); int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data); int (*cmd_async)(Monitor *mon, const QDict *params, MonitorCompletion *cb, void *opaque); } mhandler; int flags; struct mon_cmd_t *sub_table; } mon_cmd_t; *sub_table flag if it have a 2nd level of command table. > >>> + break; >>> default: >>> bad_type: >>> monitor_printf(mon, "%s: unknown type '%c'\n", cmdname, c); >>> @@ -3925,7 +3955,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline) >>> >>> qdict = qdict_new(); >>> >>> - cmd = monitor_parse_command(mon, cmdline, qdict); >>> + cmd = monitor_parse_command(mon, cmdline, mon_cmds, qdict); >>> if (!cmd) >>> goto out; >>> >>> @@ -4144,12 +4174,7 @@ static void monitor_find_completion(const char *cmdline) >>> break; >>> case 's': >>> /* XXX: more generic ? */ >>> - if (!strcmp(cmd->name, "info")) { >>> - readline_set_completion_index(cur_mon->rs, strlen(str)); >>> - for(cmd = info_cmds; cmd->name != NULL; cmd++) { >>> - cmd_completion(str, cmd->name); >>> - } >>> - } else if (!strcmp(cmd->name, "sendkey")) { >>> + if (!strcmp(cmd->name, "sendkey")) { >>> char *sep = strrchr(str, '-'); >>> if (sep) >>> str = sep + 1; > > You move the special case hack for info argument completion to case 'S' > (next hunk), but leave behind the /* XXX: more generic ? */ that marks > it as hack! Please move it along with the hack. > OK. >>> @@ -4163,6 +4188,14 @@ static void monitor_find_completion(const char *cmdline) >>> cmd_completion(str, cmd->name); >>> } >>> } >>> + case 'S': >>> + /* generic string packaged */ >>> + if (!strcmp(cmd->name, "info")) { >>> + readline_set_completion_index(cur_mon->rs, strlen(str)); >>> + for (cmd = info_cmds; cmd->name != NULL; cmd++) { >>> + cmd_completion(str, cmd->name); >>> + } >>> + } >>> break; >>> default: >>> break; >>> @@ -4483,7 +4516,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) >>> goto err_out; >>> } >>> >>> - cmd = qmp_find_cmd(cmd_name); >>> + cmd = monitor_find_command(cmd_name, qmp_cmds); >>> if (!cmd) { >>> qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); >>> goto err_out; > -- Best Regards Wenchao Xia ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device 2012-12-19 10:17 [Qemu-devel] [PATCH 0/3] HMP: enable info sub command taking parameter Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command Wenchao Xia @ 2012-12-19 10:17 ` Wenchao Xia 2012-12-21 14:07 ` Markus Armbruster 2 siblings, 1 reply; 14+ messages in thread From: Wenchao Xia @ 2012-12-19 10:17 UTC (permalink / raw) To: qemu-devel Cc: kwolf, aliguori, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia This patch add an option to show snapshots on a single block device, so some snapshot do not exist on other block device could be shown. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- monitor.c | 6 +++--- savevm.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/monitor.c b/monitor.c index ce0e74d..b019618 100644 --- a/monitor.c +++ b/monitor.c @@ -2613,9 +2613,9 @@ static mon_cmd_t info_cmds[] = { }, { .name = "snapshots", - .args_type = "", - .params = "", - .help = "show the currently saved VM snapshots", + .args_type = "device:B?", + .params = "[device]", + .help = "show snapshots of whole vm or a single device", .mhandler.info = do_info_snapshots, }, { diff --git a/savevm.c b/savevm.c index fa32171..438eb24 100644 --- a/savevm.c +++ b/savevm.c @@ -2358,7 +2358,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) } } -void do_info_snapshots(Monitor *mon, const QDict *qdict) +static void do_info_snapshots_vm(Monitor *mon) { BlockDriverState *bs, *bs1; QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; @@ -2422,6 +2422,59 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) } +static void do_info_snapshots_blk(Monitor *mon, const char *device) +{ + BlockDriverState *bs; + QEMUSnapshotInfo *sn_tab, *sn; + int nb_sns, i; + char buf[256]; + + /* find the target bs */ + bs = bdrv_find(device); + if (!bs) { + monitor_printf(mon, "Device '%s' not found.\n", device); + return ; + } + + if (!bdrv_can_snapshot(bs)) { + monitor_printf(mon, "Device '%s' can't have snapshot.\n", device); + return ; + } + + nb_sns = bdrv_snapshot_list(bs, &sn_tab); + if (nb_sns < 0) { + monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n", + device, nb_sns); + return; + } + + if (nb_sns == 0) { + monitor_printf(mon, "There is no snapshot available.\n"); + return; + } + + monitor_printf(mon, "Device %s:\n", device); + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); + for (i = 0; i < nb_sns; i++) { + sn = &sn_tab[i]; + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); + } + g_free(sn_tab); + return; +} + +void do_info_snapshots(Monitor *mon, const QDict *qdict) +{ + /* Todo, there should be a layer rebuild qdict before enter this func. */ + const char *device = qdict_get_try_str(qdict, "device"); + if (!device) { + do_info_snapshots_vm(mon); + } else { + do_info_snapshots_blk(mon, device); + } + return; +} + void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev) { qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK, -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device 2012-12-19 10:17 ` [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device Wenchao Xia @ 2012-12-21 14:07 ` Markus Armbruster 2012-12-25 3:11 ` Wenchao Xia 0 siblings, 1 reply; 14+ messages in thread From: Markus Armbruster @ 2012-12-21 14:07 UTC (permalink / raw) To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, lcapitulino, pbonzini Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > This patch add an option to show snapshots on a single block > device, so some snapshot do not exist on other block device > could be shown. > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > monitor.c | 6 +++--- > savevm.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 57 insertions(+), 4 deletions(-) > > diff --git a/monitor.c b/monitor.c > index ce0e74d..b019618 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -2613,9 +2613,9 @@ static mon_cmd_t info_cmds[] = { > }, > { > .name = "snapshots", > - .args_type = "", > - .params = "", > - .help = "show the currently saved VM snapshots", > + .args_type = "device:B?", > + .params = "[device]", > + .help = "show snapshots of whole vm or a single device", > .mhandler.info = do_info_snapshots, > }, > { > diff --git a/savevm.c b/savevm.c > index fa32171..438eb24 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2358,7 +2358,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) > } > } > > -void do_info_snapshots(Monitor *mon, const QDict *qdict) > +static void do_info_snapshots_vm(Monitor *mon) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s; > @@ -2422,6 +2422,59 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict) > > } > > +static void do_info_snapshots_blk(Monitor *mon, const char *device) > +{ > + BlockDriverState *bs; > + QEMUSnapshotInfo *sn_tab, *sn; > + int nb_sns, i; > + char buf[256]; > + > + /* find the target bs */ > + bs = bdrv_find(device); > + if (!bs) { > + monitor_printf(mon, "Device '%s' not found.\n", device); > + return ; > + } > + > + if (!bdrv_can_snapshot(bs)) { > + monitor_printf(mon, "Device '%s' can't have snapshot.\n", device); > + return ; > + } Rest of the function is is essentially code copied from do_info_snapshots_vm(). Could it be factored out? > + > + nb_sns = bdrv_snapshot_list(bs, &sn_tab); > + if (nb_sns < 0) { > + monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n", > + device, nb_sns); > + return; > + } > + > + if (nb_sns == 0) { > + monitor_printf(mon, "There is no snapshot available.\n"); > + return; > + } > + > + monitor_printf(mon, "Device %s:\n", device); > + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); > + for (i = 0; i < nb_sns; i++) { > + sn = &sn_tab[i]; > + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); > + } > + g_free(sn_tab); > + return; > +} > + > +void do_info_snapshots(Monitor *mon, const QDict *qdict) > +{ > + /* Todo, there should be a layer rebuild qdict before enter this func. */ I'm not sure I get this comment. We usually capitalize TODO for easy grepping. > + const char *device = qdict_get_try_str(qdict, "device"); > + if (!device) { > + do_info_snapshots_vm(mon); > + } else { > + do_info_snapshots_blk(mon, device); > + } > + return; > +} > + > void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev) > { > qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK, ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device 2012-12-21 14:07 ` Markus Armbruster @ 2012-12-25 3:11 ` Wenchao Xia 0 siblings, 0 replies; 14+ messages in thread From: Wenchao Xia @ 2012-12-25 3:11 UTC (permalink / raw) To: Markus Armbruster Cc: kwolf, aliguori, stefanha, qemu-devel, lcapitulino, pbonzini >> >> +static void do_info_snapshots_blk(Monitor *mon, const char *device) >> +{ >> + BlockDriverState *bs; >> + QEMUSnapshotInfo *sn_tab, *sn; >> + int nb_sns, i; >> + char buf[256]; >> + >> + /* find the target bs */ >> + bs = bdrv_find(device); >> + if (!bs) { >> + monitor_printf(mon, "Device '%s' not found.\n", device); >> + return ; >> + } >> + >> + if (!bdrv_can_snapshot(bs)) { >> + monitor_printf(mon, "Device '%s' can't have snapshot.\n", device); >> + return ; >> + } > > Rest of the function is is essentially code copied from > do_info_snapshots_vm(). Could it be factored out? > Sure, I forgot that before. I guess a better way is adding a QMP interface first and using common code from it. >> + >> + nb_sns = bdrv_snapshot_list(bs, &sn_tab); >> + if (nb_sns < 0) { >> + monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n", >> + device, nb_sns); >> + return; >> + } >> + >> + if (nb_sns == 0) { >> + monitor_printf(mon, "There is no snapshot available.\n"); >> + return; >> + } >> + >> + monitor_printf(mon, "Device %s:\n", device); >> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL)); >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn)); >> + } >> + g_free(sn_tab); >> + return; >> +} >> + >> +void do_info_snapshots(Monitor *mon, const QDict *qdict) >> +{ >> + /* Todo, there should be a layer rebuild qdict before enter this func. */ > > I'm not sure I get this comment. > > We usually capitalize TODO for easy grepping. > My mistake, this comments should have been removed, it is used before where qdict was not rebuilt as a new one. >> + const char *device = qdict_get_try_str(qdict, "device"); >> + if (!device) { >> + do_info_snapshots_vm(mon); >> + } else { >> + do_info_snapshots_blk(mon, device); >> + } >> + return; >> +} >> + >> void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev) >> { >> qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK, > -- Best Regards Wenchao Xia ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-12-25 4:31 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-19 10:17 [Qemu-devel] [PATCH 0/3] HMP: enable info sub command taking parameter Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 1/3] HMP: add QDict to info callback handler Wenchao Xia 2012-12-21 14:01 ` Markus Armbruster 2012-12-25 2:52 ` Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 2/3] HMP: pass in parameter for info sub command Wenchao Xia 2012-12-19 18:07 ` Luiz Capitulino 2012-12-20 3:02 ` Wenchao Xia 2012-12-22 21:36 ` Luiz Capitulino 2012-12-21 14:49 ` Markus Armbruster 2012-12-21 18:17 ` Eric Blake 2012-12-25 4:29 ` Wenchao Xia 2012-12-19 10:17 ` [Qemu-devel] [PATCH 3/3] HMP: show internal snapshots on a single device Wenchao Xia 2012-12-21 14:07 ` Markus Armbruster 2012-12-25 3:11 ` Wenchao Xia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).