qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command '


From: Wen Congyang
Subject: Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory
Date: Mon, 19 Mar 2012 10:28:17 +0800
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100413 Fedora/3.0.4-2.fc13 Thunderbird/3.0.4

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

<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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]