From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QgPdd-0004MS-Pt for qemu-devel@nongnu.org; Mon, 11 Jul 2011 19:11:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QgPdV-00041H-4b for qemu-devel@nongnu.org; Mon, 11 Jul 2011 19:11:41 -0400 Received: from e4.ny.us.ibm.com ([32.97.182.144]:40438) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QgPdU-0003xY-Ld for qemu-devel@nongnu.org; Mon, 11 Jul 2011 19:11:32 -0400 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by e4.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p6BMnGn0031754 for ; Mon, 11 Jul 2011 18:49:16 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p6BNBNmh179176 for ; Mon, 11 Jul 2011 19:11:24 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p6BNBMHh030493 for ; Mon, 11 Jul 2011 20:11:23 -0300 Message-ID: <4E1B8319.5040706@linux.vnet.ibm.com> Date: Mon, 11 Jul 2011 18:11:21 -0500 From: Michael Roth MIME-Version: 1.0 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> <20110711181205.585c5d7e@doriath> In-Reply-To: <20110711181205.585c5d7e@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Luiz Capitulino Cc: aliguori@linux.vnet.ibm.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org, Jes.Sorensen@redhat.com On 07/11/2011 04:12 PM, Luiz Capitulino wrote: > 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. > From a usability perspective I think the QERR_INVALID_PARAMETER, or perhaps something more descriptive, is a little nicer. Plus, they won't have to wait for 64MB to get streamed back before they get the error :) >> >>>> + >>>> + 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. > Ahh, right. Definitely. >> >>> 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. > Yah, my mistake. I think the out: label was one line higher, but if we did that we lose error information. Automatically unfreezing should be okay. >> >> 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)); >>> >> >