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