At 03/15/2012 01:18 AM, Luiz Capitulino Wrote:
> On Wed, 14 Mar 2012 10:11:35 +0800
> Wen Congyang <we...@cn.fujitsu.com> wrote:
>
>> The command's usage:
>> dump [-p] file
>> file should be start with "file:"(the file's path) or "fd:"(the fd's name).
>>
>> Note:
>> 1. If you want to use gdb to analyse the core, please specify -p option.
>> 2. This command doesn't support the fd that is is associated with a pipe,
>> socket, or FIFO(lseek will fail with such fd).
>>
>> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com>
<cut>
>> +
>> +static DumpState *dump_init(bool paging, Error **errp)
>> +{
>> + CPUState *env;
>> + DumpState *s = dump_get_current();
>> + int ret;
>> +
>> + if (runstate_is_running()) {
>> + vm_stop(RUN_STATE_PAUSED);
>> + s->resume = true;
>
> Hmm, you actually stop the VM. Seems obvious now, but when people talked about
> making this asynchronous I automatically assumed that what we didn't want was
> having the global mutex held for too much time (ie. while this command was
> running).
Yes, In the earlier version, I add a vm state change handler. If the vm is
resumed
by the user, qemu dump will be auto cancelled.
>
> The only disadvantage of having this as a synchronous command is that libvirt
> won't be able to cancel it and won't be able to run other commands in
> parallel.
> Doesn't seem that serious to me.
>
> Btw, RUN_STATE_PAUSED is not a good one. Doesn't matter that much, as this
> is unlikely to be visible, but you should use RUN_STATE_SAVE_VM or
> RUN_STATE_DEBUG.
OK, I will use RUN_STATE_SAVE_VM.
>
>> + } else {
<cut>
>> + ret = cpu_get_dump_info(&s->dump_info);
>> + if (ret < 0) {
>> + error_set(errp, QERR_UNSUPPORTED);
>
> This will let the VM paused.
Hmm, in which function the vm is paused?
>
>> + return NULL;
<cut>
>> + ret = write(fd, buf, size);
>> + if (ret != size) {
>> + return -1;
>> + }
>
> I think you should use send_all() instead of plain write().
OK, I will use qemu_write_full() you mentioned in anohter mail.
>
>> +
>> + return 0;
>> +}
<cut>
>> +
>> + s->f = fd_write_vmcore;
>> + s->cleanup = fd_cleanup;
>> + s->opaque = (void *)(intptr_t)fd;
>
> Do we really need all these indirections?
At 02/15/2012 01:31 AM, Jan Kiszka Wrote:
> Is writing to file descriptor generic enough? What if we want to dump
> via QMP, letting the receiver side decide about where to write it?
So I use these indirections.
>
>> +
>> + return s;
>> +}
>> +
>> +void qmp_dump(bool paging, const char *file, Error **errp)
>> +{
>> + const char *p;
>> + int fd = -1;
>> + DumpState *s;
>> +
>> +#if !defined(WIN32)
>> + if (strstart(file, "fd:", &p)) {
>> + fd = qemu_get_fd(p);
>
> qemu_get_fd() won't be merged, you should use monitor_get_fd(cur_mon, p);
OK
>
>> + if (fd == -1) {
>> + error_set(errp, QERR_FD_NOT_FOUND, p);
>> + return;
>> + }
>> + }
>> +#endif
>> +
>> + if (strstart(file, "file:", &p)) {
>> + fd = open(p, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR);
>
> This is minor, but I'd use qemu_open() here.
OK
>
>> + if (fd < 0) {
<cut>
>> +
>> + qmp_dump(!!paging, file, &errp);
>
> Why the double negation on 'paging'?
OK, I will remove double negation.
>
>> + hmp_handle_error(mon, &errp);
<cut>
>> +
>> +##
>> +# @dump
>
> 'dump' is too generic, please call this dump-guest-memory-vmcore or something
> more descriptive.
Hmm, dump-guest-memory-vmcore is too long. What about dump-guest-memory or
dump-memory?
>
>> +#
>> +# Dump guest's memory to vmcore.
>> +#
>> +# @paging: if true, do paging to get guest's memory mapping
>> +# @file: the filename or file descriptor of the vmcore.
>
> 'file' is not a good name because it can also dump to an fd, maybe 'protocol'?
OK
Thanks for you reviewing
Wen Congyang
