From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9Xy7-0008SM-QO for qemu-devel@nongnu.org; Mon, 19 Mar 2012 04:29:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9Xy2-0008D3-3s for qemu-devel@nongnu.org; Mon, 19 Mar 2012 04:29:31 -0400 Received: from [222.73.24.84] (port=52256 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9Xy1-0008CR-HR for qemu-devel@nongnu.org; Mon, 19 Mar 2012 04:29:26 -0400 Message-ID: <4F66EED7.6060403@cn.fujitsu.com> Date: Mon, 19 Mar 2012 16:31:19 +0800 From: Wen Congyang MIME-Version: 1.0 References: <4F5FFC63.3060300@cn.fujitsu.com> <4F5FFE57.8010308@cn.fujitsu.com> <20120314141847.4557b4bb@doriath.home> <4F6699C1.6070905@cn.fujitsu.com> In-Reply-To: <4F6699C1.6070905@cn.fujitsu.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [RFC][PATCH 11/14 v9] introduce a new monitor command 'dump' to dump guest's memory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Jan Kiszka , HATAYAMA Daisuke , Dave Anderson , qemu-devel , Eric Blake 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 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 > > > >>> + >>> +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 { > > > >>> + 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; > > > >>> + 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; >>> +} > > > >>> + >>> + 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) { > > > >>> + >>> + qmp_dump(!!paging, file, &errp); >> >> Why the double negation on 'paging'? > > OK, I will remove double negation. > >> >>> + hmp_handle_error(mon, &errp); > > > >>> + >>> +## >>> +# @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 > >