| Message ID | 1348247243-12446-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/21/2012 11:07 AM, Luiz Capitulino wrote: > 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 impact to libvirt as a QMP client, and we've already declared that HMP can change for better UI even if it is not back-compatible. Reviewed-by: Eric Blake <eblake@redhat.com>
Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com> > --- > 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)
At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: > 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. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> This patch looks fine to me. Thanks Wen Congyang > --- > 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); > + > + 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)
On Tue, 25 Sep 2012 10:48:05 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> 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. I think that the right solution would be to have an URI. > One remark inline. > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > 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) ? Good suggestion. > > > + > > + 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) >
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); + + 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)
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. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- hmp-commands.hx | 8 +++----- hmp.c | 11 ++++++++--- 2 files changed, 11 insertions(+), 8 deletions(-)