[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol a
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] hmp: dump-guest-memory: hardcode protocol argument to file: |
Date: |
Tue, 25 Sep 2012 10:48:05 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Luiz Capitulino <address@hidden> writes:
> Today, it's necessary to specify the protocol you want to use
> when dumping the guest memory, for example:
>
> (qemu) dump-guest-memory file:/tmp/guest-memory
>
> This has a few issues:
>
> 1. It's cumbersome to type
> 2. We loose file path autocompletion
> 3. Being able to specify fd:X in HMP makes little sense for humans
>
> Because of these reasons, hardcode the 'protocol' argument to
> 'file:' in HMP.
No objection.
The QMP wart remains: protocol is a string that needs to be parsed. I
feel that should be avoided in QMP. We should have expressed "either
filename or file descriptor name" in the schema. Let's avoid such
mistakes in the future.
One remark inline.
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
> hmp-commands.hx | 8 +++-----
> hmp.c | 11 ++++++++---
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ed67e99..0302458 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -914,12 +914,11 @@ ETEXI
> #if defined(CONFIG_HAVE_CORE_DUMP)
> {
> .name = "dump-guest-memory",
> - .args_type = "paging:-p,protocol:s,begin:i?,length:i?",
> - .params = "[-p] protocol [begin] [length]",
> + .args_type = "paging:-p,filename:F,begin:i?,length:i?",
> + .params = "[-p] filename [begin] [length]",
> .help = "dump guest memory to file"
> "\n\t\t\t begin(optional): the starting physical
> address"
> "\n\t\t\t length(optional): the memory size, in bytes",
> - .user_print = monitor_user_noop,
> .mhandler.cmd = hmp_dump_guest_memory,
> },
>
> @@ -929,8 +928,7 @@ STEXI
> @findex dump-guest-memory
> Dump guest memory to @var{protocol}. The file can be processed with crash or
> gdb.
> - protocol: destination file(started with "file:") or destination file
> - descriptor (started with "fd:")
> + filename: dump file name
> paging: do paging to get guest's memory mapping
> begin: the starting physical address. It's optional, and should be
> specified with length together.
> diff --git a/hmp.c b/hmp.c
> index ba6fbd3..513b40b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1042,11 +1042,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict
> *qdict)
> {
> Error *errp = NULL;
> int paging = qdict_get_try_bool(qdict, "paging", 0);
> - const char *file = qdict_get_str(qdict, "protocol");
> + const char *file = qdict_get_str(qdict, "filename");
> bool has_begin = qdict_haskey(qdict, "begin");
> bool has_length = qdict_haskey(qdict, "length");
> int64_t begin = 0;
> int64_t length = 0;
> + QString *prot;
>
> if (has_begin) {
> begin = qdict_get_int(qdict, "begin");
> @@ -1055,9 +1056,13 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict
> *qdict)
> length = qdict_get_int(qdict, "length");
> }
>
> - qmp_dump_guest_memory(paging, file, has_begin, begin, has_length, length,
> - &errp);
> + prot = qstring_from_str("file:");
> + qstring_append(prot, file);
What about prot = g_strconcat("file:", file, NULL) ... g_free(prot) ?
> +
> + qmp_dump_guest_memory(paging, qstring_get_str(prot), has_begin, begin,
> + has_length, length, &errp);
> hmp_handle_error(mon, &errp);
> + QDECREF(prot);
> }
>
> void hmp_netdev_add(Monitor *mon, const QDict *qdict)