Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory

[Wen Congyang <wency@cn.fujitsu.com>]

At 03/19/2012 10:28 AM, Wen Congyang Wrote:
> At 03/15/2012 01:18 AM, Luiz Capitulino Wrote:
>> On Wed, 14 Mar 2012 10:11:35 +0800
>> Wen Congyang <wency@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 <wency@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?

Sorry for my misundestand. I forgot to resume vm before dump_init() returns.

Thanks
Wen Congyang

> 
>>
>>> +        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
> 
>