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

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

On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
> On Mon, 11 Jul 2011 15:11:26 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> 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.
>
> 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));
>>>
>>
>