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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> On Tue,  5 Jul 2011 08:21:40 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  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<mdroth@linux.vnet.ibm.com>
>> ---
>>   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<mdroth@linux.vnet.ibm.com>
>> + *
>> + * 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.

>> +
>> +    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.

>> +    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.

> 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.
>
>> +
>> +    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().

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));
>