Re: [Qemu-devel] [PATCH v6 4/4] guest agent: add guest agent RPCs/commands

[Luiz Capitulino]

On Mon, 11 Jul 2011 15:11:26 -0500
Michael Roth <address@hidden> wrote:

> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> > On Tue,  5 Jul 2011 08:21:40 -0500
> > Michael Roth<address@hidden>  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<address@hidden>
> >> ---
> >>   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<address@hidden>
> >> + *
> >> + * 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<glib.h>
> >> +#include<mntent.h>
> >> +#include<sys/types.h>
> >> +#include<sys/ioctl.h>
> >> +#include<linux/fs.h>
> >> +#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));
> >
>