[Top][All Lists]
[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
- Re: [Qemu-devel] [RFC][PATCH 08/14 v9] target-i386: Add API to write cpu status to core file, (continued)
[Qemu-devel] [RFC][PATCH 09/14 v9] target-i386: add API to get dump info, Wen Congyang, 2012/03/13
[Qemu-devel] [RFC][PATCH 10/14 v9] make gdb_id() generally avialable, Wen Congyang, 2012/03/13
[Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Wen Congyang, 2012/03/13
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Luiz Capitulino, 2012/03/14
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Wen Congyang, 2012/03/14
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Luiz Capitulino, 2012/03/15
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Wen Congyang, 2012/03/16
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory,
Wen Congyang <=
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Wen Congyang, 2012/03/19
- Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, Luiz Capitulino, 2012/03/19
Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory, HATAYAMA Daisuke, 2012/03/15
[Qemu-devel] [RFC][PATCH 12/14 v9] support to cancel the current dumping, Wen Congyang, 2012/03/13
[Qemu-devel] [RFC][PATCH 13/14 v9] support to query dumping status, Wen Congyang, 2012/03/13
[Qemu-devel] [RFC][PATCH 14/14 v9] allow user to dump a fraction of the memory, Wen Congyang, 2012/03/13