From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33059) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QgNm2-0007yR-6t for qemu-devel@nongnu.org; Mon, 11 Jul 2011 17:12:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QgNlz-00044W-Jn for qemu-devel@nongnu.org; Mon, 11 Jul 2011 17:12:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32034) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QgNlz-000449-4W for qemu-devel@nongnu.org; Mon, 11 Jul 2011 17:12:11 -0400 Date: Mon, 11 Jul 2011 18:12:05 -0300 From: Luiz Capitulino Message-ID: <20110711181205.585c5d7e@doriath> In-Reply-To: <4E1B58EE.2020404@linux.vnet.ibm.com> References: <1309872100-27912-1-git-send-email-mdroth@linux.vnet.ibm.com> <1309872100-27912-5-git-send-email-mdroth@linux.vnet.ibm.com> <20110708121427.3f17ef2a@doriath> <4E1B58EE.2020404@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 4/4] guest agent: add guest agent RPCs/commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: aliguori@linux.vnet.ibm.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org, Jes.Sorensen@redhat.com On Mon, 11 Jul 2011 15:11:26 -0500 Michael Roth wrote: > On 07/08/2011 10:14 AM, Luiz Capitulino wrote: > > On Tue, 5 Jul 2011 08:21:40 -0500 > > Michael Roth wrote: > > > >> This adds the initial set of QMP/QAPI commands provided by the guest > >> agent: > >> > >> guest-sync > >> guest-ping > >> guest-info > >> guest-shutdown > >> guest-file-open > >> guest-file-read > >> guest-file-write > >> guest-file-seek > >> guest-file-close > >> guest-fsfreeze-freeze > >> guest-fsfreeze-thaw > >> guest-fsfreeze-status > >> > >> The input/output specification for these commands are documented in the > >> schema. > >> > >> Example usage: > >> > >> host: > >> qemu -device virtio-serial \ > >> -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \ > >> -device virtserialport,chardev=qga0,name=qga0 > >> ... > >> > >> echo "{'execute':'guest-info'}" | socat stdio \ > >> unix-connect:/tmp/qga0.sock > >> > >> guest: > >> qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \ > >> -p /var/run/qemu-guest-agent.pid -d > >> > >> Signed-off-by: Michael Roth > >> --- > >> Makefile | 15 ++- > >> qemu-ga.c | 4 + > >> qerror.h | 3 + > >> qga/guest-agent-commands.c | 501 ++++++++++++++++++++++++++++++++++++++++++++ > >> qga/guest-agent-core.h | 2 + > >> 5 files changed, 523 insertions(+), 2 deletions(-) > >> create mode 100644 qga/guest-agent-commands.c > >> > >> diff --git a/Makefile b/Makefile > >> index b2e8593..7e4f722 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c > >> $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py > >> $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"< $<, " GEN $@") > >> > >> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h > >> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py > >> + $(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"< $<, " GEN $@") > >> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h > >> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py > >> + $(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"< $<, " GEN $@") > >> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py > >> + $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"< $<, " GEN $@") > >> + > >> test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h) > >> test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o > >> > >> test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h) > >> test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o > >> > >> -QGALIB=qga/guest-agent-command-state.o > >> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o > >> + > >> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c > >> > >> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o > >> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o > >> > >> QEMULIBS=libhw32 libhw64 libuser libdis libdis-user > >> > >> diff --git a/qemu-ga.c b/qemu-ga.c > >> index 649c16a..04ead22 100644 > >> --- a/qemu-ga.c > >> +++ b/qemu-ga.c > >> @@ -637,6 +637,9 @@ int main(int argc, char **argv) > >> g_log_set_default_handler(ga_log, s); > >> g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR); > >> s->logging_enabled = true; > >> + s->command_state = ga_command_state_new(); > >> + ga_command_state_init(s, s->command_state); > >> + ga_command_state_init_all(s->command_state); > >> ga_state = s; > >> > >> module_call_init(MODULE_INIT_QAPI); > >> @@ -645,6 +648,7 @@ int main(int argc, char **argv) > >> > >> g_main_loop_run(ga_state->main_loop); > >> > >> + ga_command_state_cleanup_all(ga_state->command_state); > >> unlink(pidfile); > >> > >> return 0; > >> diff --git a/qerror.h b/qerror.h > >> index 9a9fa5b..0f618ac 100644 > >> --- a/qerror.h > >> +++ b/qerror.h > >> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj); > >> #define QERR_FEATURE_DISABLED \ > >> "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }" > >> > >> +#define QERR_QGA_LOGGING_FAILED \ > >> + "{ 'class': 'QgaLoggingFailed', 'data': {} }" > >> + > >> #endif /* QERROR_H */ > >> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c > >> new file mode 100644 > >> index 0000000..42390fb > >> --- /dev/null > >> +++ b/qga/guest-agent-commands.c > >> @@ -0,0 +1,501 @@ > >> +/* > >> + * QEMU Guest Agent commands > >> + * > >> + * Copyright IBM Corp. 2011 > >> + * > >> + * Authors: > >> + * Michael Roth > >> + * > >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. > >> + * See the COPYING file in the top-level directory. > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include "qga/guest-agent-core.h" > >> +#include "qga-qmp-commands.h" > >> +#include "qerror.h" > >> +#include "qemu-queue.h" > >> + > >> +static GAState *ga_state; > >> + > >> +static void disable_logging(void) > >> +{ > >> + ga_disable_logging(ga_state); > >> +} > >> + > >> +static void enable_logging(void) > >> +{ > >> + ga_enable_logging(ga_state); > >> +} > >> + > >> +/* Note: in some situations, like with the fsfreeze, logging may be > >> + * temporarilly disabled. if it is necessary that a command be able > >> + * to log for accounting purposes, check ga_logging_enabled() beforehand, > >> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error > >> + */ > >> +static void slog(const char *fmt, ...) > >> +{ > >> + va_list ap; > >> + > >> + va_start(ap, fmt); > >> + g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap); > >> + va_end(ap); > >> +} > >> + > >> +int64_t qmp_guest_sync(int64_t id, Error **errp) > >> +{ > >> + return id; > >> +} > >> + > >> +void qmp_guest_ping(Error **err) > >> +{ > >> + slog("guest-ping called"); > >> +} > >> + > >> +struct GuestAgentInfo *qmp_guest_info(Error **err) > >> +{ > >> + GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo)); > >> + > >> + info->version = g_strdup(QGA_VERSION); > >> + > >> + return info; > >> +} > >> + > >> +void qmp_guest_shutdown(const char *mode, Error **err) > >> +{ > >> + int ret; > >> + const char *shutdown_flag; > >> + > >> + slog("guest-shutdown called, mode: %s", mode); > >> + if (strcmp(mode, "halt") == 0) { > >> + shutdown_flag = "-H"; > >> + } else if (strcmp(mode, "powerdown") == 0) { > >> + shutdown_flag = "-P"; > >> + } else if (strcmp(mode, "reboot") == 0) { > >> + shutdown_flag = "-r"; > >> + } else { > >> + error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode", > >> + "halt|powerdown|reboot"); > >> + return; > >> + } > >> + > >> + ret = fork(); > >> + if (ret == 0) { > >> + /* child, start the shutdown */ > >> + setsid(); > >> + fclose(stdin); > >> + fclose(stdout); > >> + fclose(stderr); > >> + > >> + sleep(5); > > > > If we're required to return a response before the shutdown happens, this > > is a bug and I'm afraid that the right way to this is a bit complex. > > > > Otherwise we can just leave it out. > > > > Yah, I ran this by Anthony and Adam and just leaving it out seems to be > the preferred approach, for now at least. > > >> + ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0", > >> + "hypervisor initiated shutdown", (char*)NULL); > >> + if (ret) { > >> + slog("guest-shutdown failed: %s", strerror(errno)); > >> + } > >> + exit(!!ret); > >> + } else if (ret< 0) { > >> + error_set(err, QERR_UNDEFINED_ERROR); > >> + } > >> +} > >> + > >> +typedef struct GuestFileHandle { > >> + uint64_t id; > >> + FILE *fh; > >> + QTAILQ_ENTRY(GuestFileHandle) next; > >> +} GuestFileHandle; > >> + > >> +static struct { > >> + QTAILQ_HEAD(, GuestFileHandle) filehandles; > >> +} guest_file_state; > >> + > >> +static void guest_file_handle_add(FILE *fh) > >> +{ > >> + GuestFileHandle *gfh; > >> + > >> + gfh = qemu_mallocz(sizeof(GuestFileHandle)); > >> + gfh->id = fileno(fh); > >> + gfh->fh = fh; > >> + QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next); > >> +} > >> + > >> +static GuestFileHandle *guest_file_handle_find(int64_t id) > >> +{ > >> + GuestFileHandle *gfh; > >> + > >> + QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next) > >> + { > >> + if (gfh->id == id) { > >> + return gfh; > >> + } > >> + } > >> + > >> + return NULL; > >> +} > >> + > >> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err) > >> +{ > >> + FILE *fh; > >> + int fd; > >> + int64_t ret = -1; > >> + > >> + if (!has_mode) { > >> + mode = "r"; > >> + } > >> + slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode); > >> + fh = fopen(filepath, mode); > >> + if (!fh) { > >> + error_set(err, QERR_OPEN_FILE_FAILED, filepath); > >> + return -1; > >> + } > >> + > >> + /* set fd non-blocking to avoid common use cases (like reading from a > >> + * named pipe) from hanging the agent > >> + */ > >> + fd = fileno(fh); > >> + ret = fcntl(fd, F_GETFL); > >> + ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK); > >> + if (ret == -1) { > >> + error_set(err, QERR_OPEN_FILE_FAILED, filepath); > >> + fclose(fh); > >> + return -1; > >> + } > >> + > >> + guest_file_handle_add(fh); > >> + slog("guest-file-open, filehandle: %d", fd); > >> + return fd; > >> +} > >> + > >> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count, > >> + Error **err) > >> +{ > >> + GuestFileHandle *gfh = guest_file_handle_find(filehandle); > >> + GuestFileRead *read_data; > >> + guchar *buf; > >> + FILE *fh; > >> + size_t read_count; > >> + > >> + if (!gfh) { > >> + error_set(err, QERR_FD_NOT_FOUND, "filehandle"); > >> + return NULL; > >> + } > >> + > >> + if (count< 0 || count> QGA_READ_LIMIT) { > >> + error_set(err, QERR_INVALID_PARAMETER, "count"); > >> + return NULL; > >> + } > > > > Are we imposing that limit because of the malloc() call below? If that's > > the case I think it's wrong, because we don't know the VM (neither the guest) > > better than the client. > > > > The best thing we can do here is to limit it to the file size. Additionally > > to this we could have a command-line option to allow the sysadmin set his/her > > own limit. > > > > That's technically the better approach, but we're also bound by the > maximum token size limit in the JSON parser, which is 64MB. Best we can > do is set QGA_READ_LIMIT accordingly. Good point, but I think it's ok to let the parser do this check itself, as control won't get here anyway. Maybe we should only document that the server imposes a limit on the token size. > > >> + > >> + fh = gfh->fh; > >> + read_data = qemu_mallocz(sizeof(GuestFileRead) + 1); > >> + buf = qemu_mallocz(count+1); > >> + if (!buf) { > >> + error_set(err, QERR_UNDEFINED_ERROR); > >> + return NULL; > >> + } > > > > qemu_malloc() functions never fail... > > > >> + > >> + read_count = fread(buf, 1, count, fh); > > > > Isn't 'nmemb' and 'size' swapped? > > > > I'm not sure... > > size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream); > > I wrote it as $count number of 1-bytes elements. This seems logical to > me, since it's a non-blocking FD which way result in a partial of some > lesser number of bytes than count. Ok. I think either way will work. > > >> + buf[read_count] = 0; > >> + read_data->count = read_count; > >> + read_data->eof = feof(fh); > >> + if (read_count) { > >> + read_data->buf = g_base64_encode(buf, read_count); > >> + } > >> + qemu_free(buf); > >> + /* clear error and eof. error is generally due to EAGAIN from non-blocking > >> + * mode, and no real way to differenitate from a real error since we only > >> + * get boolean error flag from ferror() > >> + */ > >> + clearerr(fh); > >> + > >> + return read_data; > >> +} > >> + > >> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64, > >> + int64_t count, Error **err) > >> +{ > >> + GuestFileWrite *write_data; > >> + guchar *data; > >> + gsize data_len; > >> + int write_count; > >> + GuestFileHandle *gfh = guest_file_handle_find(filehandle); > >> + FILE *fh; > >> + > >> + if (!gfh) { > >> + error_set(err, QERR_FD_NOT_FOUND, "filehandle"); > >> + return NULL; > >> + } > >> + > >> + fh = gfh->fh; > >> + data = g_base64_decode(data_b64,&data_len); > >> + if (count> data_len) { > >> + qemu_free(data); > >> + error_set(err, QERR_INVALID_PARAMETER, "count"); > >> + return NULL; > >> + } > >> + write_data = qemu_mallocz(sizeof(GuestFileWrite)); > >> + write_count = fwrite(data, 1, count, fh); > >> + write_data->count = write_count; > >> + write_data->eof = feof(fh); > >> + qemu_free(data); > >> + clearerr(fh); > > > > Shouldn't we check for errors instead of doing this? > > > > Yah...unfortunately we only get a boolean flag with ferror() so it's not > all that useful, but I can add an error flag to the calls to capture it > at least. clearerr() is only being used here to clear the eof flag. I meant to check fwrite()'s return. > > > Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush() > > here, but that's probably bad). > > > > Yah, I'd been planning on adding a guest-file-flush() for a while now. > I'll add that for the respin. > > >> + > >> + return write_data; > >> +} > >> + > >> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset, > >> + int64_t whence, Error **err) > >> +{ > >> + GuestFileSeek *seek_data; > >> + GuestFileHandle *gfh = guest_file_handle_find(filehandle); > >> + FILE *fh; > >> + int ret; > >> + > >> + if (!gfh) { > >> + error_set(err, QERR_FD_NOT_FOUND, "filehandle"); > >> + return NULL; > >> + } > >> + > >> + fh = gfh->fh; > >> + seek_data = qemu_mallocz(sizeof(GuestFileRead)); > >> + ret = fseek(fh, offset, whence); > >> + if (ret == -1) { > >> + error_set(err, QERR_UNDEFINED_ERROR); > >> + qemu_free(seek_data); > >> + return NULL; > >> + } > >> + seek_data->position = ftell(fh); > >> + seek_data->eof = feof(fh); > >> + clearerr(fh); > > > > Again, I don't see why we should do this. This is probably ok, as we're checking fseek() above. > > > >> + > >> + return seek_data; > >> +} > >> + > >> +void qmp_guest_file_close(int64_t filehandle, Error **err) > >> +{ > >> + GuestFileHandle *gfh = guest_file_handle_find(filehandle); > >> + > >> + slog("guest-file-close called, filehandle: %ld", filehandle); > >> + if (!gfh) { > >> + error_set(err, QERR_FD_NOT_FOUND, "filehandle"); > >> + return; > >> + } > >> + > >> + fclose(gfh->fh); > >> + QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next); > >> + qemu_free(gfh); > >> +} > >> + > >> +static void guest_file_init(void) > >> +{ > >> + QTAILQ_INIT(&guest_file_state.filehandles); > >> +} > >> + > >> +typedef struct GuestFsfreezeMount { > >> + char *dirname; > >> + char *devtype; > >> + QTAILQ_ENTRY(GuestFsfreezeMount) next; > >> +} GuestFsfreezeMount; > >> + > >> +struct { > >> + GuestFsfreezeStatus status; > >> + QTAILQ_HEAD(, GuestFsfreezeMount) mount_list; > >> +} guest_fsfreeze_state; > >> + > >> +/* > >> + * Walk the mount table and build a list of local file systems > >> + */ > >> +static int guest_fsfreeze_build_mount_list(void) > >> +{ > >> + struct mntent *ment; > >> + GuestFsfreezeMount *mount, *temp; > >> + char const *mtab = MOUNTED; > >> + FILE *fp; > >> + > >> + fp = setmntent(mtab, "r"); > >> + if (!fp) { > >> + g_warning("fsfreeze: unable to read mtab"); > >> + goto fail; > >> + } > >> + > >> + while ((ment = getmntent(fp))) { > >> + /* > >> + * An entry which device name doesn't start with a '/' is > >> + * either a dummy file system or a network file system. > >> + * Add special handling for smbfs and cifs as is done by > >> + * coreutils as well. > >> + */ > >> + if ((ment->mnt_fsname[0] != '/') || > >> + (strcmp(ment->mnt_type, "smbfs") == 0) || > >> + (strcmp(ment->mnt_type, "cifs") == 0)) { > >> + continue; > >> + } > >> + > >> + mount = qemu_mallocz(sizeof(GuestFsfreezeMount)); > >> + mount->dirname = qemu_strdup(ment->mnt_dir); > >> + mount->devtype = qemu_strdup(ment->mnt_type); > >> + > >> + QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next); > >> + } > >> + > >> + endmntent(fp); > >> + > >> + return 0; > >> + > >> +fail: > >> + QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) { > >> + QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next); > >> + qemu_free(mount->dirname); > >> + qemu_free(mount->devtype); > >> + qemu_free(mount); > >> + } > > > > This doesn't seem to be used. > > > > It can get used even a 2nd invocation of this function gets called that > results in a goto fail. But looking again this should be done > unconditionally at the start of the function, since the mount list is > part of global state now. > > >> + > >> + return -1; > >> +} > >> + > >> +/* > >> + * Return status of freeze/thaw > >> + */ > >> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err) > >> +{ > >> + return guest_fsfreeze_state.status; > >> +} > >> + > >> +/* > >> + * Walk list of mounted file systems in the guest, and freeze the ones which > >> + * are real local file systems. > >> + */ > >> +int64_t qmp_guest_fsfreeze_freeze(Error **err) > >> +{ > >> + int ret = 0, i = 0; > >> + struct GuestFsfreezeMount *mount, *temp; > >> + int fd; > >> + > >> + slog("guest-fsfreeze called"); > >> + > >> + if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) { > > > > return 0; > > > >> + ret = 0; > >> + goto out; > >> + } > >> + > >> + ret = guest_fsfreeze_build_mount_list(); > >> + if (ret< 0) { > > > > return ret; > > > >> + goto out; > >> + } > >> + > >> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS; > >> + > >> + /* cannot risk guest agent blocking itself on a write in this state */ > >> + disable_logging(); > >> + > >> + QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) { > >> + fd = qemu_open(mount->dirname, O_RDONLY); > >> + if (fd == -1) { > >> + ret = errno; > >> + goto error; > >> + } > >> + > >> + /* we try to cull filesytems we know won't work in advance, but other > >> + * filesytems may not implement fsfreeze for less obvious reasons. > >> + * these will reason EOPNOTSUPP, so we simply ignore them. when > >> + * thawing, these filesystems will return an EINVAL instead, due to > >> + * not being in a frozen state. Other filesystem-specific > >> + * errors may result in EINVAL, however, so the user should check the > >> + * number * of filesystems returned here against those returned by the > >> + * thaw operation to determine whether everything completed > >> + * successfully > >> + */ > >> + ret = ioctl(fd, FIFREEZE); > >> + if (ret< 0&& errno != EOPNOTSUPP) { > >> + close(fd); > >> + goto error; > >> + } > >> + close(fd); > >> + > >> + i++; > >> + } > >> + > >> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN; > >> + ret = i; > >> +out: > >> + return ret; > >> +error: > >> + if (i> 0) { > >> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR; > >> + } > > > > Shouldn't you undo everything that has been done so far? Which is > > freeing the build list and thawing the file-systems that were frozen > > before the error? > > > > We can...the way it's done right now is you get a count of how many > filesystems were frozen, along an error status. Depending on the > error/count the user can either ignore the error, do what it is they > want to do, then call thaw(), or just immediately call thaw(). But you don't get the count on error, so it's difficult (if possible) to learn how many file-systems were frozen. > > So we can do an automatic thaw() on their behalf, but i figured the > status was good enough. > > >> + goto out; > >> +} > >> + > >> +/* > >> + * Walk list of frozen file systems in the guest, and thaw them. > >> + */ > >> +int64_t qmp_guest_fsfreeze_thaw(Error **err) > >> +{ > >> + int ret; > >> + GuestFsfreezeMount *mount, *temp; > >> + int fd, i = 0; > >> + > >> + if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&& > >> + guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) { > > > > I don't follow why we're checking against INPROGRESS here. > > > > To prevent a race I believe...but we're synchronous now so that's > probably no longer needed. I'll look it over and remove it if that's the > case. > > >> + ret = 0; > >> + goto out_enable_logging; > >> + } > >> + > >> + QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) { > >> + fd = qemu_open(mount->dirname, O_RDONLY); > >> + if (fd == -1) { > >> + ret = -errno; > >> + goto out; > >> + } > >> + ret = ioctl(fd, FITHAW); > >> + if (ret< 0&& errno != EOPNOTSUPP&& errno != EINVAL) { > >> + ret = -errno; > >> + close(fd); > >> + goto out; > > > > Shouldn't you continue and try to thaw the other file-systems in the list? > > > > That's probably better > > >> + } > >> + close(fd); > >> + > >> + QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next); > >> + qemu_free(mount->dirname); > >> + qemu_free(mount->devtype); > >> + qemu_free(mount); > >> + i++; > >> + } > >> + > >> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED; > >> + ret = i; > >> +out_enable_logging: > >> + enable_logging(); > >> +out: > >> + return ret; > >> +} > >> + > >> +static void guest_fsfreeze_init(void) > >> +{ > >> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED; > >> + QTAILQ_INIT(&guest_fsfreeze_state.mount_list); > >> +} > >> + > >> +static void guest_fsfreeze_cleanup(void) > >> +{ > >> + int64_t ret; > >> + Error *err = NULL; > >> + > >> + if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) { > >> + ret = qmp_guest_fsfreeze_thaw(&err); > >> + if (ret< 0 || err) { > >> + slog("failed to clean up frozen filesystems"); > >> + } > >> + } > >> +} > >> + > >> +/* register init/cleanup routines for stateful command groups */ > >> +void ga_command_state_init(GAState *s, GACommandState *cs) > >> +{ > >> + ga_state = s; > >> + ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup); > >> + ga_command_state_add(cs, guest_file_init, NULL); > >> +} > >> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h > >> index 66d1729..3501ff4 100644 > >> --- a/qga/guest-agent-core.h > >> +++ b/qga/guest-agent-core.h > >> @@ -14,10 +14,12 @@ > >> #include "qemu-common.h" > >> > >> #define QGA_VERSION "1.0" > >> +#define QGA_READ_LIMIT 4<< 20 /* 4MB block size max for chunked reads */ > >> > >> typedef struct GAState GAState; > >> typedef struct GACommandState GACommandState; > >> > >> +void ga_command_state_init(GAState *s, GACommandState *cs); > >> void ga_command_state_add(GACommandState *cs, > >> void (*init)(void), > >> void (*cleanup)(void)); > > >