Patchwork [v7,4/4] guest agent: add guest agent RPCs/commands

login
register
mail settings
Submitter Michael Roth
Date July 14, 2011, 8 p.m.
Message ID <1310673634-17631-5-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/104721/
State New
Headers show

Comments

Michael Roth - July 14, 2011, 8 p.m.
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-flush
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=org.qemu.quest_agent.0
         ...

    echo "{'execute':'guest-info'}" | socat stdio unix-connect:/tmp/qga0.sock

  guest:
    qemu-ga -m virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent.0 \
            -p /var/run/qemu-guest-agent.pid -d

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile                   |   15 ++-
 qapi-schema-guest.json     |  217 +++++++++++++++++++
 qemu-ga.c                  |    4 +
 qerror.c                   |    8 +
 qerror.h                   |    6 +
 qga/guest-agent-commands.c |  512 ++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h     |    2 +
 7 files changed, 762 insertions(+), 2 deletions(-)
 create mode 100644 qapi-schema-guest.json
 create mode 100644 qga/guest-agent-commands.c
Luiz Capitulino - July 15, 2011, 4:23 p.m.
On Thu, 14 Jul 2011 15:00:34 -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-flush
> 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=org.qemu.quest_agent.0
>          ...
> 
>     echo "{'execute':'guest-info'}" | socat stdio unix-connect:/tmp/qga0.sock
> 
>   guest:
>     qemu-ga -m virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent.0 \
>             -p /var/run/qemu-guest-agent.pid -d
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  Makefile                   |   15 ++-
>  qapi-schema-guest.json     |  217 +++++++++++++++++++
>  qemu-ga.c                  |    4 +
>  qerror.c                   |    8 +
>  qerror.h                   |    6 +
>  qga/guest-agent-commands.c |  512 ++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h     |    2 +
>  7 files changed, 762 insertions(+), 2 deletions(-)
>  create mode 100644 qapi-schema-guest.json
>  create mode 100644 qga/guest-agent-commands.c
> 
> diff --git a/Makefile b/Makefile
> index b2e8593..f98e127 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-qapi-types.o $(qapi-dir)/qga-qmp-marshal.o
>  
>  QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>  
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> new file mode 100644
> index 0000000..a18d6f8
> --- /dev/null
> +++ b/qapi-schema-guest.json
> @@ -0,0 +1,217 @@
> +# *-*- Mode: Python -*-*
> +
> +##
> +# @guest-sync:
> +#
> +# Echo back a unique integer value
> +#
> +# This is used by clients talking to the guest agent over the
> +# wire to ensure the stream is in sync and doesn't contain stale
> +# data from previous client. All guest agent responses should be
> +# ignored until the provided unique integer value is returned,
> +# and it is up to the client to handle stale whole or
> +# partially-delivered JSON text in such a way that this response
> +# can be obtained.
> +#
> +# Such clients should also preceed this command
> +# with a 0xFF byte to make such the guest agent flushes any
> +# partially read JSON data from a previous session.
> +#
> +# @id: randomly generated 64-bit integer
> +#
> +# Returns: The unique integer id passed in by the client
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-sync'
> +  'data':    { 'id': 'int' },
> +  'returns': 'int' }
> +
> +##
> +# @guest-ping:
> +#
> +# Ping the guest agent, a non-error return implies success
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-ping' }
> +
> +##
> +# @guest-info:
> +#
> +# Get some information about the guest agent.
> +#
> +# Since: 0.15.0
> +##
> +{ 'type': 'GuestAgentInfo', 'data': {'version': 'str'} }
> +{ 'command': 'guest-info',
> +  'returns': 'GuestAgentInfo' }
> +
> +##
> +# @guest-shutdown:
> +#
> +# Initiate guest-activated shutdown. Note: this is an asynchronous
> +# shutdown request, with no guaruntee of successful shutdown. Errors
> +# will be logged to guest's syslog.
> +#
> +# @mode: #optional "halt", "powerdown", or "reboot" (default)

I'd choose 'powerdown' as the default.

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } }
> +
> +##
> +# @guest-file-open:
> +#
> +# Open a file in the guest and retrieve a file handle for it
> +#
> +# @filepath: Full path to the file in the guest to open.
> +#
> +# @mode: #optional open mode, as per fopen(), "r" is the default.
> +#
> +# Returns: Guest file handle on success.
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-file-open',
> +  'data':    { 'path': 'str', '*mode': 'str' },
> +  'returns': 'int' }
> +
> +##
> +# @guest-file-close:
> +#
> +# Close an open file in the guest
> +#
> +# @handle: filehandle returned by guest-file-open
> +#
> +# Returns: Nothing on success.

fclose() can fail, sorry for not seeing this before.

> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-file-close',
> +  'data': { 'handle': 'int' } }
> +
> +##
> +# @guest-file-read:
> +#
> +# Read from an open file in the guest. Data will be base64-encoded
> +#
> +# @handle: filehandle returned by guest-file-open
> +#
> +# @count: #optional maximum number of bytes to read (default is 4KB)
> +#
> +# Returns: GuestFileRead on success. Note: count is number of bytes read
> +#          *before* base64 encoding
> +#          bytes read.

unneeded line break.

> +#
> +# Since: 0.15.0
> +##
> +{ 'type': 'GuestFileRead',
> +  'data': { 'count': 'int', 'buf-b64': 'str', 'eof': 'bool' } }
> +
> +{ 'command': 'guest-file-read',
> +  'data':    { 'handle': 'int', '*count': 'int' },
> +  'returns': 'GuestFileRead' }
> +
> +##
> +# @guest-file-write:
> +#
> +# Write to an open file in the guest.
> +#
> +# @handle: filehandle returned by guest-file-open
> +#
> +# @buf-b64: base64-encoded string representing data to be written
> +#
> +# @count: #optional bytes to write (actual bytes, after base64-decode),
> +#         default is all content in buf-b64 buffer after base64 decoding
> +#
> +# Returns: GuestFileWrite on success. Note: count is the number of 

number of?

> +#
> +# Since: 0.15.0
> +##
> +{ 'type': 'GuestFileWrite',
> +  'data': { 'count': 'int', 'eof': 'bool' } }
> +{ 'command': 'guest-file-write',
> +  'data':    { 'handle': 'int', 'buf-b64': 'str', '*count': 'int' },
> +  'returns': 'GuestFileWrite' }
> +
> +##
> +# @guest-file-seek:
> +#
> +# Seek to a position in the file, as with fseek(), and return the
> +# current file position afterward. Also encapsulates ftell()'s
> +# functionality, just Set offset=0, whence=SEEK_CUR.
> +#
> +# @handle: filehandle returned by guest-file-open
> +#
> +# @offset: bytes to skip over in the file stream
> +#
> +# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
> +#
> +# Returns: GuestFileSeek on success.
> +#
> +# Since: 0.15.0
> +##
> +{ 'type': 'GuestFileSeek',
> +  'data': { 'position': 'int', 'eof': 'bool' } }
> +
> +{ 'command': 'guest-file-seek',
> +  'data':    { 'handle': 'int', 'offset': 'int', 'whence': 'int' },
> +  'returns': 'GuestFileSeek' }
> +
> +##
> +# @guest-file-flush:
> +#
> +# Write file changes bufferred in userspace to disk/kernel buffers
> +#
> +# @handle: filehandle returned by guest-file-open
> +#
> +# Returns: Nothing on success.
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-file-flush',
> +  'data': { 'handle': 'int' } }
> +
> +##
> +# @guest-fsfreeze-status:
> +#
> +# Get guest fsfreeze state. error state indicates failure to thaw 1 or more
> +# previously frozen filesystems, or failure to open a previously cached
> +# filesytem (filesystem unmounted/directory changes, etc).
> +#
> +# Returns: GuestFsfreezeStatus (of the form "GUEST_FSFREEZE_STATUS_<STATUS>")
> +#
> +# Since: 0.15.0
> +##
> +{ 'enum': 'GuestFsfreezeStatus',
> +  'data': [ 'thawed', 'frozen', 'error' ] }

I'd expect the status to be just that and not the plain enum string name.

> +{ 'command': 'guest-fsfreeze-status',
> +  'returns': 'GuestFsfreezeStatus' }
> +
> +##
> +# @guest-fsfreeze-freeze:
> +#
> +# Sync and freeze all non-network guest filesystems
> +#
> +# Returns: Number of file systems frozen on success
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-fsfreeze-freeze',
> +  'returns': 'int' }
> +
> +##
> +# @guest-fsfreeze-thaw:
> +#
> +# Unfreeze frozen guest fileystems
> +#
> +# Returns: Number of file systems thawed
> +#          If error, -1 (unknown error) or -errno
> +#
> +# Since: 0.15.0
> +##
> +{ 'command': 'guest-fsfreeze-thaw',
> +  'returns': 'int' }
> diff --git a/qemu-ga.c b/qemu-ga.c
> index eb09100..4530d3d 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -653,6 +653,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);
> @@ -661,6 +664,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.c b/qerror.c
> index c92adfc..229d0d6 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -218,6 +218,14 @@ static const QErrorStringTable qerror_table[] = {
>          .error_fmt = QERR_VNC_SERVER_FAILED,
>          .desc      = "Could not start VNC server on %(target)",
>      },
> +    {
> +        .error_fmt = QERR_QGA_LOGGING_FAILED,
> +        .desc      = "Guest agent failed to log non-optional log statement",
> +    },
> +    {
> +        .error_fmt = QERR_QGA_COMMAND_FAILED,
> +        .desc      = "Guest agent command failed, error was '%(message)'",
> +    },
>      {}
>  };
>  
> diff --git a/qerror.h b/qerror.h
> index 9a9fa5b..7ec0fc1 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -184,4 +184,10 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_FEATURE_DISABLED \
>      "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>  
> +#define QERR_QGA_LOGGING_FAILED \
> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> +
> +#define QERR_QGA_COMMAND_FAILED \
> +    "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
> +
>  #endif /* QERROR_H */
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> new file mode 100644
> index 0000000..0e885d0
> --- /dev/null
> +++ b/qga/guest-agent-commands.c
> @@ -0,0 +1,512 @@
> +/*
> + * 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(bool has_mode, const char *mode, Error **err)
> +{
> +    int ret;
> +    const char *shutdown_flag;
> +
> +    slog("guest-shutdown called, mode: %s", mode);
> +    if (!has_mode || strcmp(mode, "reboot") == 0) {
> +        shutdown_flag = "-r";
> +    } else if (strcmp(mode, "halt") == 0) {
> +        shutdown_flag = "-H";
> +    } else if (strcmp(mode, "powerdown") == 0) {
> +        shutdown_flag = "-P";
> +    } 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);
> +
> +        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 *path, 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", path, mode);
> +    fh = fopen(path, mode);

I think it's good to check if 'mode' is valid. Linux libc seems to handle
this correctly, but I don't know if other unices will do.

> +    if (!fh) {
> +        error_set(err, QERR_OPEN_FILE_FAILED, path);
> +        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, path);
> +        fclose(fh);
> +        return -1;
> +    }
> +
> +    guest_file_handle_add(fh);
> +    slog("guest-file-open, handle: %d", fd);
> +    return fd;
> +}
> +
> +void qmp_guest_file_close(int64_t handle, Error **err)
> +{
> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +
> +    slog("guest-file-close called, handle: %ld", handle);
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> +        return;
> +    }
> +
> +    fclose(gfh->fh);

As I said above, this can fail.

> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> +    qemu_free(gfh);
> +}
> +
> +struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
> +                                          int64_t count, Error **err)
> +{
> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileRead *read_data = NULL;
> +    guchar *buf;
> +    FILE *fh;
> +    size_t read_count;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> +        return NULL;
> +    }
> +
> +    if (!has_count) {
> +        count = QGA_READ_COUNT_DEFAULT;
> +    } else if (count < 0) {
> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> +        return NULL;
> +    }

As I said in my past review, I think it's a good idea to limit 'count' to the
file size.

> +
> +    fh = gfh->fh;
> +    buf = qemu_mallocz(count+1);
> +    read_count = fread(buf, 1, count, fh);
> +    if (ferror(fh)) {
> +        slog("guest-file-read failed, handle: %ld", handle);
> +        error_set(err, QERR_QGA_COMMAND_FAILED, "fread() failed");
> +    } else {
> +        buf[read_count] = 0;
> +        read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);

Why +1?

> +        read_data->count = read_count;
> +        read_data->eof = feof(fh);
> +        if (read_count) {
> +            read_data->buf_b64 = g_base64_encode(buf, read_count);
> +        }
> +    }
> +    qemu_free(buf);
> +    clearerr(fh);
> +
> +    return read_data;
> +}
> +
> +GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
> +                                     bool has_count, int64_t count, Error **err)
> +{
> +    GuestFileWrite *write_data = NULL;
> +    guchar *buf;
> +    gsize buf_len;
> +    int write_count;
> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    FILE *fh;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> +        return NULL;
> +    }
> +
> +    fh = gfh->fh;
> +    buf = g_base64_decode(buf_b64, &buf_len);
> +
> +    if (!has_count) {
> +        count = buf_len;
> +    } else if (count < 0 || count > buf_len) {
> +        qemu_free(buf);
> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> +        return NULL;
> +    }
> +
> +    write_count = fwrite(buf, 1, count, fh);
> +    if (ferror(fh)) {
> +        slog("guest-file-write failed, handle: %ld", handle);
> +        error_set(err, QERR_QGA_COMMAND_FAILED, "fwrite() error");
> +    } else {
> +        write_data = qemu_mallocz(sizeof(GuestFileWrite));
> +        write_data->count = write_count;
> +        write_data->eof = feof(fh);
> +    }
> +    qemu_free(buf);
> +    clearerr(fh);
> +
> +    return write_data;
> +}
> +
> +struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
> +                                          int64_t whence, Error **err)
> +{
> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    GuestFileSeek *seek_data = NULL;
> +    FILE *fh;
> +    int ret;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> +        return NULL;
> +    }
> +
> +    fh = gfh->fh;
> +    ret = fseek(fh, offset, whence);
> +    if (ret == -1) {
> +        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
> +    } else {
> +        seek_data = qemu_mallocz(sizeof(GuestFileRead));
> +        seek_data->position = ftell(fh);
> +        seek_data->eof = feof(fh);
> +    }
> +    clearerr(fh);
> +
> +    return seek_data;
> +}
> +
> +void qmp_guest_file_flush(int64_t handle, Error **err)
> +{
> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> +    FILE *fh;
> +    int ret;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> +        return;
> +    }
> +
> +    fh = gfh->fh;
> +    ret = fflush(fh);
> +    if (ret == EOF) {
> +        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
> +    }
> +}
> +
> +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;
> +
> +    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);
> +    }

Why is this needed? Shouldn't this be done on qmp_guest_fsfreeze_thaw()?

> +
> +    fp = setmntent(mtab, "r");
> +    if (!fp) {
> +        g_warning("fsfreeze: unable to read mtab");
> +        return -1;
> +    }
> +
> +    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;
> +}
> +
> +/*
> + * 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;
> +    char err_msg[512];
> +
> +    slog("guest-fsfreeze called");
> +
> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> +        return 0;
> +    }
> +
> +    ret = guest_fsfreeze_build_mount_list();
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* 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) {
> +            sprintf(err_msg, "failed to open %s, %s", mount->dirname, strerror(errno));
> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            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 report 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) {
> +            sprintf(err_msg, "failed to freeze %s, %s", mount->dirname, strerror(errno));
> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> +            close(fd);
> +            goto error;
> +        }
> +        close(fd);
> +
> +        i++;
> +    }
> +
> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> +    return i;
> +
> +error:
> +    if (i > 0) {
> +        qmp_guest_fsfreeze_thaw(NULL);
> +    }
> +    return 0;
> +}
> +
> +/*
> + * 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;
> +    bool has_error = false;
> +
> +    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
> +        fd = qemu_open(mount->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            has_error = true;
> +            continue;
> +        }
> +        ret = ioctl(fd, FITHAW);
> +        if (ret < 0 && errno != EOPNOTSUPP && errno != EINVAL) {
> +            has_error = true;
> +            close(fd);
> +            continue;
> +        }
> +        close(fd);
> +        i++;
> +    }
> +
> +    if (has_error) {
> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> +    } else {
> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> +    }
> +    enable_logging();
> +    return i;
> +}
> +
> +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..e42b91d 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_COUNT_DEFAULT 4 << 10
>  
>  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));
Michael Roth - July 15, 2011, 5:21 p.m.
On 07/15/2011 11:23 AM, Luiz Capitulino wrote:
> On Thu, 14 Jul 2011 15:00:34 -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-flush
>> 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=org.qemu.quest_agent.0
>>           ...
>>
>>      echo "{'execute':'guest-info'}" | socat stdio unix-connect:/tmp/qga0.sock
>>
>>    guest:
>>      qemu-ga -m virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent.0 \
>>              -p /var/run/qemu-guest-agent.pid -d
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   Makefile                   |   15 ++-
>>   qapi-schema-guest.json     |  217 +++++++++++++++++++
>>   qemu-ga.c                  |    4 +
>>   qerror.c                   |    8 +
>>   qerror.h                   |    6 +
>>   qga/guest-agent-commands.c |  512 ++++++++++++++++++++++++++++++++++++++++++++
>>   qga/guest-agent-core.h     |    2 +
>>   7 files changed, 762 insertions(+), 2 deletions(-)
>>   create mode 100644 qapi-schema-guest.json
>>   create mode 100644 qga/guest-agent-commands.c
>>
>> diff --git a/Makefile b/Makefile
>> index b2e8593..f98e127 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-qapi-types.o $(qapi-dir)/qga-qmp-marshal.o
>>
>>   QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>>
>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>> new file mode 100644
>> index 0000000..a18d6f8
>> --- /dev/null
>> +++ b/qapi-schema-guest.json
>> @@ -0,0 +1,217 @@
>> +# *-*- Mode: Python -*-*
>> +
>> +##
>> +# @guest-sync:
>> +#
>> +# Echo back a unique integer value
>> +#
>> +# This is used by clients talking to the guest agent over the
>> +# wire to ensure the stream is in sync and doesn't contain stale
>> +# data from previous client. All guest agent responses should be
>> +# ignored until the provided unique integer value is returned,
>> +# and it is up to the client to handle stale whole or
>> +# partially-delivered JSON text in such a way that this response
>> +# can be obtained.
>> +#
>> +# Such clients should also preceed this command
>> +# with a 0xFF byte to make such the guest agent flushes any
>> +# partially read JSON data from a previous session.
>> +#
>> +# @id: randomly generated 64-bit integer
>> +#
>> +# Returns: The unique integer id passed in by the client
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-sync'
>> +  'data':    { 'id': 'int' },
>> +  'returns': 'int' }
>> +
>> +##
>> +# @guest-ping:
>> +#
>> +# Ping the guest agent, a non-error return implies success
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-ping' }
>> +
>> +##
>> +# @guest-info:
>> +#
>> +# Get some information about the guest agent.
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'type': 'GuestAgentInfo', 'data': {'version': 'str'} }
>> +{ 'command': 'guest-info',
>> +  'returns': 'GuestAgentInfo' }
>> +
>> +##
>> +# @guest-shutdown:
>> +#
>> +# Initiate guest-activated shutdown. Note: this is an asynchronous
>> +# shutdown request, with no guaruntee of successful shutdown. Errors
>> +# will be logged to guest's syslog.
>> +#
>> +# @mode: #optional "halt", "powerdown", or "reboot" (default)
>
> I'd choose 'powerdown' as the default.
>

Yup, meant to change this sorry.

>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } }
>> +
>> +##
>> +# @guest-file-open:
>> +#
>> +# Open a file in the guest and retrieve a file handle for it
>> +#
>> +# @filepath: Full path to the file in the guest to open.
>> +#
>> +# @mode: #optional open mode, as per fopen(), "r" is the default.
>> +#
>> +# Returns: Guest file handle on success.
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-file-open',
>> +  'data':    { 'path': 'str', '*mode': 'str' },
>> +  'returns': 'int' }
>> +
>> +##
>> +# @guest-file-close:
>> +#
>> +# Close an open file in the guest
>> +#
>> +# @handle: filehandle returned by guest-file-open
>> +#
>> +# Returns: Nothing on success.
>
> fclose() can fail, sorry for not seeing this before.
>

I'll add checks for this.

>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-file-close',
>> +  'data': { 'handle': 'int' } }
>> +
>> +##
>> +# @guest-file-read:
>> +#
>> +# Read from an open file in the guest. Data will be base64-encoded
>> +#
>> +# @handle: filehandle returned by guest-file-open
>> +#
>> +# @count: #optional maximum number of bytes to read (default is 4KB)
>> +#
>> +# Returns: GuestFileRead on success. Note: count is number of bytes read
>> +#          *before* base64 encoding
>> +#          bytes read.
>
> unneeded line break.
>
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'type': 'GuestFileRead',
>> +  'data': { 'count': 'int', 'buf-b64': 'str', 'eof': 'bool' } }
>> +
>> +{ 'command': 'guest-file-read',
>> +  'data':    { 'handle': 'int', '*count': 'int' },
>> +  'returns': 'GuestFileRead' }
>> +
>> +##
>> +# @guest-file-write:
>> +#
>> +# Write to an open file in the guest.
>> +#
>> +# @handle: filehandle returned by guest-file-open
>> +#
>> +# @buf-b64: base64-encoded string representing data to be written
>> +#
>> +# @count: #optional bytes to write (actual bytes, after base64-decode),
>> +#         default is all content in buf-b64 buffer after base64 decoding
>> +#
>> +# Returns: GuestFileWrite on success. Note: count is the number of
>
> number of?
>

number of bytes base64-decoded bytes written. must've gotten distracted :)

>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'type': 'GuestFileWrite',
>> +  'data': { 'count': 'int', 'eof': 'bool' } }
>> +{ 'command': 'guest-file-write',
>> +  'data':    { 'handle': 'int', 'buf-b64': 'str', '*count': 'int' },
>> +  'returns': 'GuestFileWrite' }
>> +
>> +##
>> +# @guest-file-seek:
>> +#
>> +# Seek to a position in the file, as with fseek(), and return the
>> +# current file position afterward. Also encapsulates ftell()'s
>> +# functionality, just Set offset=0, whence=SEEK_CUR.
>> +#
>> +# @handle: filehandle returned by guest-file-open
>> +#
>> +# @offset: bytes to skip over in the file stream
>> +#
>> +# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
>> +#
>> +# Returns: GuestFileSeek on success.
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'type': 'GuestFileSeek',
>> +  'data': { 'position': 'int', 'eof': 'bool' } }
>> +
>> +{ 'command': 'guest-file-seek',
>> +  'data':    { 'handle': 'int', 'offset': 'int', 'whence': 'int' },
>> +  'returns': 'GuestFileSeek' }
>> +
>> +##
>> +# @guest-file-flush:
>> +#
>> +# Write file changes bufferred in userspace to disk/kernel buffers
>> +#
>> +# @handle: filehandle returned by guest-file-open
>> +#
>> +# Returns: Nothing on success.
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-file-flush',
>> +  'data': { 'handle': 'int' } }
>> +
>> +##
>> +# @guest-fsfreeze-status:
>> +#
>> +# Get guest fsfreeze state. error state indicates failure to thaw 1 or more
>> +# previously frozen filesystems, or failure to open a previously cached
>> +# filesytem (filesystem unmounted/directory changes, etc).
>> +#
>> +# Returns: GuestFsfreezeStatus (of the form "GUEST_FSFREEZE_STATUS_<STATUS>")
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'enum': 'GuestFsfreezeStatus',
>> +  'data': [ 'thawed', 'frozen', 'error' ] }
>
> I'd expect the status to be just that and not the plain enum string name.
>

I like the extra verbosity for enums but I guess the 
schema/command/field tell you that anyway so I guess it's somewhat 
redundant. Will change.

>> +{ 'command': 'guest-fsfreeze-status',
>> +  'returns': 'GuestFsfreezeStatus' }
>> +
>> +##
>> +# @guest-fsfreeze-freeze:
>> +#
>> +# Sync and freeze all non-network guest filesystems
>> +#
>> +# Returns: Number of file systems frozen on success
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-fsfreeze-freeze',
>> +  'returns': 'int' }
>> +
>> +##
>> +# @guest-fsfreeze-thaw:
>> +#
>> +# Unfreeze frozen guest fileystems
>> +#
>> +# Returns: Number of file systems thawed
>> +#          If error, -1 (unknown error) or -errno
>> +#
>> +# Since: 0.15.0
>> +##
>> +{ 'command': 'guest-fsfreeze-thaw',
>> +  'returns': 'int' }
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> index eb09100..4530d3d 100644
>> --- a/qemu-ga.c
>> +++ b/qemu-ga.c
>> @@ -653,6 +653,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);
>> @@ -661,6 +664,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.c b/qerror.c
>> index c92adfc..229d0d6 100644
>> --- a/qerror.c
>> +++ b/qerror.c
>> @@ -218,6 +218,14 @@ static const QErrorStringTable qerror_table[] = {
>>           .error_fmt = QERR_VNC_SERVER_FAILED,
>>           .desc      = "Could not start VNC server on %(target)",
>>       },
>> +    {
>> +        .error_fmt = QERR_QGA_LOGGING_FAILED,
>> +        .desc      = "Guest agent failed to log non-optional log statement",
>> +    },
>> +    {
>> +        .error_fmt = QERR_QGA_COMMAND_FAILED,
>> +        .desc      = "Guest agent command failed, error was '%(message)'",
>> +    },
>>       {}
>>   };
>>
>> diff --git a/qerror.h b/qerror.h
>> index 9a9fa5b..7ec0fc1 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -184,4 +184,10 @@ QError *qobject_to_qerror(const QObject *obj);
>>   #define QERR_FEATURE_DISABLED \
>>       "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>>
>> +#define QERR_QGA_LOGGING_FAILED \
>> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
>> +
>> +#define QERR_QGA_COMMAND_FAILED \
>> +    "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
>> +
>>   #endif /* QERROR_H */
>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>> new file mode 100644
>> index 0000000..0e885d0
>> --- /dev/null
>> +++ b/qga/guest-agent-commands.c
>> @@ -0,0 +1,512 @@
>> +/*
>> + * 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(bool has_mode, const char *mode, Error **err)
>> +{
>> +    int ret;
>> +    const char *shutdown_flag;
>> +
>> +    slog("guest-shutdown called, mode: %s", mode);
>> +    if (!has_mode || strcmp(mode, "reboot") == 0) {
>> +        shutdown_flag = "-r";
>> +    } else if (strcmp(mode, "halt") == 0) {
>> +        shutdown_flag = "-H";
>> +    } else if (strcmp(mode, "powerdown") == 0) {
>> +        shutdown_flag = "-P";
>> +    } 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);
>> +
>> +        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 *path, 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", path, mode);
>> +    fh = fopen(path, mode);
>
> I think it's good to check if 'mode' is valid. Linux libc seems to handle
> this correctly, but I don't know if other unices will do.
>

I think it's okay to apply the "let them blow their guest up" 
methodology we adopted for guest-read-file here as well, since the 
client theoretically knows the guest better.

>> +    if (!fh) {
>> +        error_set(err, QERR_OPEN_FILE_FAILED, path);
>> +        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, path);
>> +        fclose(fh);
>> +        return -1;
>> +    }
>> +
>> +    guest_file_handle_add(fh);
>> +    slog("guest-file-open, handle: %d", fd);
>> +    return fd;
>> +}
>> +
>> +void qmp_guest_file_close(int64_t handle, Error **err)
>> +{
>> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
>> +
>> +    slog("guest-file-close called, handle: %ld", handle);
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
>> +        return;
>> +    }
>> +
>> +    fclose(gfh->fh);
>
> As I said above, this can fail.
>
>> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
>> +    qemu_free(gfh);
>> +}
>> +
>> +struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>> +                                          int64_t count, Error **err)
>> +{
>> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
>> +    GuestFileRead *read_data = NULL;
>> +    guchar *buf;
>> +    FILE *fh;
>> +    size_t read_count;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
>> +        return NULL;
>> +    }
>> +
>> +    if (!has_count) {
>> +        count = QGA_READ_COUNT_DEFAULT;
>> +    } else if (count<  0) {
>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>> +        return NULL;
>> +    }
>
> As I said in my past review, I think it's a good idea to limit 'count' to the
> file size.
>

I guess I misunderstood. I suggested we drop both the idea of limiting 
to a QGA_READ_COUNT_MAX as well as limiting to filesize and thought we 
were in agreement in letting the client blow up their guest with huge 
mallocs if they were so inclined. And if they find themselves in that 
situation they're misusing the interface anyway since there's a token 
size limit of 64MB (which reminds me I need to add a note about that in 
the schema/docs) which would've bit them if their malloc didn't cause 
any problems.

If you'd still like to do it this way, I don't think fstat() would work 
since it wouldn't reflect updates from fwrite() until an 
fflush()/fclose(). And if we're gonna bother with this we should also be 
more correct and limit to amount of bytes left in the stream. I think 
something like this would do it:

cur_pos = ftell(fh);
fseek(fh, 0, SEEK_END);
last_pos = ftell(fh);
fseek(fh, cur_pos, SEEK_SET);
remaining = last_pos - cur_pos
count = (count > remaining) ? remaining : count;

And we'd need to do it on every call to guest-file-read.

>> +
>> +    fh = gfh->fh;
>> +    buf = qemu_mallocz(count+1);
>> +    read_count = fread(buf, 1, count, fh);
>> +    if (ferror(fh)) {
>> +        slog("guest-file-read failed, handle: %ld", handle);
>> +        error_set(err, QERR_QGA_COMMAND_FAILED, "fread() failed");
>> +    } else {
>> +        buf[read_count] = 0;
>> +        read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
>
> Why +1?
>

ehhh...no idea. Must've added that erroneously when I was fixing a buf 
overflow with 'buf = qemu_mallocz(count);'. Will fix.

>> +        read_data->count = read_count;
>> +        read_data->eof = feof(fh);
>> +        if (read_count) {
>> +            read_data->buf_b64 = g_base64_encode(buf, read_count);
>> +        }
>> +    }
>> +    qemu_free(buf);
>> +    clearerr(fh);
>> +
>> +    return read_data;
>> +}
>> +
>> +GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
>> +                                     bool has_count, int64_t count, Error **err)
>> +{
>> +    GuestFileWrite *write_data = NULL;
>> +    guchar *buf;
>> +    gsize buf_len;
>> +    int write_count;
>> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
>> +    FILE *fh;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
>> +        return NULL;
>> +    }
>> +
>> +    fh = gfh->fh;
>> +    buf = g_base64_decode(buf_b64,&buf_len);
>> +
>> +    if (!has_count) {
>> +        count = buf_len;
>> +    } else if (count<  0 || count>  buf_len) {
>> +        qemu_free(buf);
>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>> +        return NULL;
>> +    }
>> +
>> +    write_count = fwrite(buf, 1, count, fh);
>> +    if (ferror(fh)) {
>> +        slog("guest-file-write failed, handle: %ld", handle);
>> +        error_set(err, QERR_QGA_COMMAND_FAILED, "fwrite() error");
>> +    } else {
>> +        write_data = qemu_mallocz(sizeof(GuestFileWrite));
>> +        write_data->count = write_count;
>> +        write_data->eof = feof(fh);
>> +    }
>> +    qemu_free(buf);
>> +    clearerr(fh);
>> +
>> +    return write_data;
>> +}
>> +
>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
>> +                                          int64_t whence, Error **err)
>> +{
>> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
>> +    GuestFileSeek *seek_data = NULL;
>> +    FILE *fh;
>> +    int ret;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
>> +        return NULL;
>> +    }
>> +
>> +    fh = gfh->fh;
>> +    ret = fseek(fh, offset, whence);
>> +    if (ret == -1) {
>> +        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
>> +    } else {
>> +        seek_data = qemu_mallocz(sizeof(GuestFileRead));
>> +        seek_data->position = ftell(fh);
>> +        seek_data->eof = feof(fh);
>> +    }
>> +    clearerr(fh);
>> +
>> +    return seek_data;
>> +}
>> +
>> +void qmp_guest_file_flush(int64_t handle, Error **err)
>> +{
>> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
>> +    FILE *fh;
>> +    int ret;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
>> +        return;
>> +    }
>> +
>> +    fh = gfh->fh;
>> +    ret = fflush(fh);
>> +    if (ret == EOF) {
>> +        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
>> +    }
>> +}
>> +
>> +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;
>> +
>> +    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);
>> +    }
>
> Why is this needed? Shouldn't this be done on qmp_guest_fsfreeze_thaw()?
>

Thaw might leave them in an error state, and the only recourse might be 
to retry the thaw on the cached mount list after manual intervening on 
the guest. So the list needs to stick around else we'd have to rebuild 
it in thaw, which means it may not be in sync with the list we were 
using when doing the initial freeze.

>> +
>> +    fp = setmntent(mtab, "r");
>> +    if (!fp) {
>> +        g_warning("fsfreeze: unable to read mtab");
>> +        return -1;
>> +    }
>> +
>> +    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;
>> +}
>> +
>> +/*
>> + * 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;
>> +    char err_msg[512];
>> +
>> +    slog("guest-fsfreeze called");
>> +
>> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
>> +        return 0;
>> +    }
>> +
>> +    ret = guest_fsfreeze_build_mount_list();
>> +    if (ret<  0) {
>> +        return ret;
>> +    }
>> +
>> +    /* 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) {
>> +            sprintf(err_msg, "failed to open %s, %s", mount->dirname, strerror(errno));
>> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
>> +            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 report 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) {
>> +            sprintf(err_msg, "failed to freeze %s, %s", mount->dirname, strerror(errno));
>> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
>> +            close(fd);
>> +            goto error;
>> +        }
>> +        close(fd);
>> +
>> +        i++;
>> +    }
>> +
>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
>> +    return i;
>> +
>> +error:
>> +    if (i>  0) {
>> +        qmp_guest_fsfreeze_thaw(NULL);
>> +    }
>> +    return 0;
>> +}
>> +
>> +/*
>> + * 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;
>> +    bool has_error = false;
>> +
>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>> +        if (fd == -1) {
>> +            has_error = true;
>> +            continue;
>> +        }
>> +        ret = ioctl(fd, FITHAW);
>> +        if (ret<  0&&  errno != EOPNOTSUPP&&  errno != EINVAL) {
>> +            has_error = true;
>> +            close(fd);
>> +            continue;
>> +        }
>> +        close(fd);
>> +        i++;
>> +    }
>> +
>> +    if (has_error) {
>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
>> +    } else {
>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>> +    }
>> +    enable_logging();
>> +    return i;
>> +}
>> +
>> +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..e42b91d 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_COUNT_DEFAULT 4<<  10
>>
>>   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));
>
Luiz Capitulino - July 15, 2011, 6:01 p.m.
On Fri, 15 Jul 2011 12:21:55 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 07/15/2011 11:23 AM, Luiz Capitulino wrote:
> > On Thu, 14 Jul 2011 15:00:34 -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-flush
> >> 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=org.qemu.quest_agent.0
> >>           ...
> >>
> >>      echo "{'execute':'guest-info'}" | socat stdio unix-connect:/tmp/qga0.sock
> >>
> >>    guest:
> >>      qemu-ga -m virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent.0 \
> >>              -p /var/run/qemu-guest-agent.pid -d
> >>
> >> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >> ---
> >>   Makefile                   |   15 ++-
> >>   qapi-schema-guest.json     |  217 +++++++++++++++++++
> >>   qemu-ga.c                  |    4 +
> >>   qerror.c                   |    8 +
> >>   qerror.h                   |    6 +
> >>   qga/guest-agent-commands.c |  512 ++++++++++++++++++++++++++++++++++++++++++++
> >>   qga/guest-agent-core.h     |    2 +
> >>   7 files changed, 762 insertions(+), 2 deletions(-)
> >>   create mode 100644 qapi-schema-guest.json
> >>   create mode 100644 qga/guest-agent-commands.c
> >>
> >> diff --git a/Makefile b/Makefile
> >> index b2e8593..f98e127 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-qapi-types.o $(qapi-dir)/qga-qmp-marshal.o
> >>
> >>   QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
> >>
> >> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> >> new file mode 100644
> >> index 0000000..a18d6f8
> >> --- /dev/null
> >> +++ b/qapi-schema-guest.json
> >> @@ -0,0 +1,217 @@
> >> +# *-*- Mode: Python -*-*
> >> +
> >> +##
> >> +# @guest-sync:
> >> +#
> >> +# Echo back a unique integer value
> >> +#
> >> +# This is used by clients talking to the guest agent over the
> >> +# wire to ensure the stream is in sync and doesn't contain stale
> >> +# data from previous client. All guest agent responses should be
> >> +# ignored until the provided unique integer value is returned,
> >> +# and it is up to the client to handle stale whole or
> >> +# partially-delivered JSON text in such a way that this response
> >> +# can be obtained.
> >> +#
> >> +# Such clients should also preceed this command
> >> +# with a 0xFF byte to make such the guest agent flushes any
> >> +# partially read JSON data from a previous session.
> >> +#
> >> +# @id: randomly generated 64-bit integer
> >> +#
> >> +# Returns: The unique integer id passed in by the client
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-sync'
> >> +  'data':    { 'id': 'int' },
> >> +  'returns': 'int' }
> >> +
> >> +##
> >> +# @guest-ping:
> >> +#
> >> +# Ping the guest agent, a non-error return implies success
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-ping' }
> >> +
> >> +##
> >> +# @guest-info:
> >> +#
> >> +# Get some information about the guest agent.
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'type': 'GuestAgentInfo', 'data': {'version': 'str'} }
> >> +{ 'command': 'guest-info',
> >> +  'returns': 'GuestAgentInfo' }
> >> +
> >> +##
> >> +# @guest-shutdown:
> >> +#
> >> +# Initiate guest-activated shutdown. Note: this is an asynchronous
> >> +# shutdown request, with no guaruntee of successful shutdown. Errors
> >> +# will be logged to guest's syslog.
> >> +#
> >> +# @mode: #optional "halt", "powerdown", or "reboot" (default)
> >
> > I'd choose 'powerdown' as the default.
> >
> 
> Yup, meant to change this sorry.
> 
> >> +#
> >> +# Returns: Nothing on success
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } }
> >> +
> >> +##
> >> +# @guest-file-open:
> >> +#
> >> +# Open a file in the guest and retrieve a file handle for it
> >> +#
> >> +# @filepath: Full path to the file in the guest to open.
> >> +#
> >> +# @mode: #optional open mode, as per fopen(), "r" is the default.
> >> +#
> >> +# Returns: Guest file handle on success.
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-file-open',
> >> +  'data':    { 'path': 'str', '*mode': 'str' },
> >> +  'returns': 'int' }
> >> +
> >> +##
> >> +# @guest-file-close:
> >> +#
> >> +# Close an open file in the guest
> >> +#
> >> +# @handle: filehandle returned by guest-file-open
> >> +#
> >> +# Returns: Nothing on success.
> >
> > fclose() can fail, sorry for not seeing this before.
> >
> 
> I'll add checks for this.
> 
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-file-close',
> >> +  'data': { 'handle': 'int' } }
> >> +
> >> +##
> >> +# @guest-file-read:
> >> +#
> >> +# Read from an open file in the guest. Data will be base64-encoded
> >> +#
> >> +# @handle: filehandle returned by guest-file-open
> >> +#
> >> +# @count: #optional maximum number of bytes to read (default is 4KB)
> >> +#
> >> +# Returns: GuestFileRead on success. Note: count is number of bytes read
> >> +#          *before* base64 encoding
> >> +#          bytes read.
> >
> > unneeded line break.
> >
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'type': 'GuestFileRead',
> >> +  'data': { 'count': 'int', 'buf-b64': 'str', 'eof': 'bool' } }
> >> +
> >> +{ 'command': 'guest-file-read',
> >> +  'data':    { 'handle': 'int', '*count': 'int' },
> >> +  'returns': 'GuestFileRead' }
> >> +
> >> +##
> >> +# @guest-file-write:
> >> +#
> >> +# Write to an open file in the guest.
> >> +#
> >> +# @handle: filehandle returned by guest-file-open
> >> +#
> >> +# @buf-b64: base64-encoded string representing data to be written
> >> +#
> >> +# @count: #optional bytes to write (actual bytes, after base64-decode),
> >> +#         default is all content in buf-b64 buffer after base64 decoding
> >> +#
> >> +# Returns: GuestFileWrite on success. Note: count is the number of
> >
> > number of?
> >
> 
> number of bytes base64-decoded bytes written. must've gotten distracted :)
> 
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'type': 'GuestFileWrite',
> >> +  'data': { 'count': 'int', 'eof': 'bool' } }
> >> +{ 'command': 'guest-file-write',
> >> +  'data':    { 'handle': 'int', 'buf-b64': 'str', '*count': 'int' },
> >> +  'returns': 'GuestFileWrite' }
> >> +
> >> +##
> >> +# @guest-file-seek:
> >> +#
> >> +# Seek to a position in the file, as with fseek(), and return the
> >> +# current file position afterward. Also encapsulates ftell()'s
> >> +# functionality, just Set offset=0, whence=SEEK_CUR.
> >> +#
> >> +# @handle: filehandle returned by guest-file-open
> >> +#
> >> +# @offset: bytes to skip over in the file stream
> >> +#
> >> +# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
> >> +#
> >> +# Returns: GuestFileSeek on success.
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'type': 'GuestFileSeek',
> >> +  'data': { 'position': 'int', 'eof': 'bool' } }
> >> +
> >> +{ 'command': 'guest-file-seek',
> >> +  'data':    { 'handle': 'int', 'offset': 'int', 'whence': 'int' },
> >> +  'returns': 'GuestFileSeek' }
> >> +
> >> +##
> >> +# @guest-file-flush:
> >> +#
> >> +# Write file changes bufferred in userspace to disk/kernel buffers
> >> +#
> >> +# @handle: filehandle returned by guest-file-open
> >> +#
> >> +# Returns: Nothing on success.
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-file-flush',
> >> +  'data': { 'handle': 'int' } }
> >> +
> >> +##
> >> +# @guest-fsfreeze-status:
> >> +#
> >> +# Get guest fsfreeze state. error state indicates failure to thaw 1 or more
> >> +# previously frozen filesystems, or failure to open a previously cached
> >> +# filesytem (filesystem unmounted/directory changes, etc).
> >> +#
> >> +# Returns: GuestFsfreezeStatus (of the form "GUEST_FSFREEZE_STATUS_<STATUS>")
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'enum': 'GuestFsfreezeStatus',
> >> +  'data': [ 'thawed', 'frozen', 'error' ] }
> >
> > I'd expect the status to be just that and not the plain enum string name.
> >
> 
> I like the extra verbosity for enums but I guess the 
> schema/command/field tell you that anyway so I guess it's somewhat 
> redundant. Will change.
> 
> >> +{ 'command': 'guest-fsfreeze-status',
> >> +  'returns': 'GuestFsfreezeStatus' }
> >> +
> >> +##
> >> +# @guest-fsfreeze-freeze:
> >> +#
> >> +# Sync and freeze all non-network guest filesystems
> >> +#
> >> +# Returns: Number of file systems frozen on success
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-fsfreeze-freeze',
> >> +  'returns': 'int' }
> >> +
> >> +##
> >> +# @guest-fsfreeze-thaw:
> >> +#
> >> +# Unfreeze frozen guest fileystems
> >> +#
> >> +# Returns: Number of file systems thawed
> >> +#          If error, -1 (unknown error) or -errno
> >> +#
> >> +# Since: 0.15.0
> >> +##
> >> +{ 'command': 'guest-fsfreeze-thaw',
> >> +  'returns': 'int' }
> >> diff --git a/qemu-ga.c b/qemu-ga.c
> >> index eb09100..4530d3d 100644
> >> --- a/qemu-ga.c
> >> +++ b/qemu-ga.c
> >> @@ -653,6 +653,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);
> >> @@ -661,6 +664,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.c b/qerror.c
> >> index c92adfc..229d0d6 100644
> >> --- a/qerror.c
> >> +++ b/qerror.c
> >> @@ -218,6 +218,14 @@ static const QErrorStringTable qerror_table[] = {
> >>           .error_fmt = QERR_VNC_SERVER_FAILED,
> >>           .desc      = "Could not start VNC server on %(target)",
> >>       },
> >> +    {
> >> +        .error_fmt = QERR_QGA_LOGGING_FAILED,
> >> +        .desc      = "Guest agent failed to log non-optional log statement",
> >> +    },
> >> +    {
> >> +        .error_fmt = QERR_QGA_COMMAND_FAILED,
> >> +        .desc      = "Guest agent command failed, error was '%(message)'",
> >> +    },
> >>       {}
> >>   };
> >>
> >> diff --git a/qerror.h b/qerror.h
> >> index 9a9fa5b..7ec0fc1 100644
> >> --- a/qerror.h
> >> +++ b/qerror.h
> >> @@ -184,4 +184,10 @@ QError *qobject_to_qerror(const QObject *obj);
> >>   #define QERR_FEATURE_DISABLED \
> >>       "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
> >>
> >> +#define QERR_QGA_LOGGING_FAILED \
> >> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> >> +
> >> +#define QERR_QGA_COMMAND_FAILED \
> >> +    "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
> >> +
> >>   #endif /* QERROR_H */
> >> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >> new file mode 100644
> >> index 0000000..0e885d0
> >> --- /dev/null
> >> +++ b/qga/guest-agent-commands.c
> >> @@ -0,0 +1,512 @@
> >> +/*
> >> + * 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(bool has_mode, const char *mode, Error **err)
> >> +{
> >> +    int ret;
> >> +    const char *shutdown_flag;
> >> +
> >> +    slog("guest-shutdown called, mode: %s", mode);
> >> +    if (!has_mode || strcmp(mode, "reboot") == 0) {
> >> +        shutdown_flag = "-r";
> >> +    } else if (strcmp(mode, "halt") == 0) {
> >> +        shutdown_flag = "-H";
> >> +    } else if (strcmp(mode, "powerdown") == 0) {
> >> +        shutdown_flag = "-P";
> >> +    } 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);
> >> +
> >> +        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 *path, 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", path, mode);
> >> +    fh = fopen(path, mode);
> >
> > I think it's good to check if 'mode' is valid. Linux libc seems to handle
> > this correctly, but I don't know if other unices will do.
> >
> 
> I think it's okay to apply the "let them blow their guest up" 
> methodology we adopted for guest-read-file here as well, since the 
> client theoretically knows the guest better.
> 
> >> +    if (!fh) {
> >> +        error_set(err, QERR_OPEN_FILE_FAILED, path);
> >> +        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, path);
> >> +        fclose(fh);
> >> +        return -1;
> >> +    }
> >> +
> >> +    guest_file_handle_add(fh);
> >> +    slog("guest-file-open, handle: %d", fd);
> >> +    return fd;
> >> +}
> >> +
> >> +void qmp_guest_file_close(int64_t handle, Error **err)
> >> +{
> >> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> >> +
> >> +    slog("guest-file-close called, handle: %ld", handle);
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> >> +        return;
> >> +    }
> >> +
> >> +    fclose(gfh->fh);
> >
> > As I said above, this can fail.
> >
> >> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> >> +    qemu_free(gfh);
> >> +}
> >> +
> >> +struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
> >> +                                          int64_t count, Error **err)
> >> +{
> >> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> >> +    GuestFileRead *read_data = NULL;
> >> +    guchar *buf;
> >> +    FILE *fh;
> >> +    size_t read_count;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    if (!has_count) {
> >> +        count = QGA_READ_COUNT_DEFAULT;
> >> +    } else if (count<  0) {
> >> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >> +        return NULL;
> >> +    }
> >
> > As I said in my past review, I think it's a good idea to limit 'count' to the
> > file size.
> >
> 
> I guess I misunderstood. I suggested we drop both the idea of limiting 
> to a QGA_READ_COUNT_MAX as well as limiting to filesize and thought we 
> were in agreement in letting the client blow up their guest with huge 
> mallocs if they were so inclined. And if they find themselves in that 
> situation they're misusing the interface anyway since there's a token 
> size limit of 64MB (which reminds me I need to add a note about that in 
> the schema/docs) which would've bit them if their malloc didn't cause 
> any problems.
> 
> If you'd still like to do it this way, I don't think fstat() would work 
> since it wouldn't reflect updates from fwrite() until an 
> fflush()/fclose(). And if we're gonna bother with this we should also be 
> more correct and limit to amount of bytes left in the stream. I think 
> something like this would do it:
> 
> cur_pos = ftell(fh);
> fseek(fh, 0, SEEK_END);
> last_pos = ftell(fh);
> fseek(fh, cur_pos, SEEK_SET);
> remaining = last_pos - cur_pos
> count = (count > remaining) ? remaining : count;
> 
> And we'd need to do it on every call to guest-file-read.

I think it's up to the client to do that, that's when the 'you should know
what it's doing' saying applies. At the same time, I'm not that strong about
that for this first version. If you prefer, we can wait & see if this turns
out to be an issue and fix it if it does.

> >> +
> >> +    fh = gfh->fh;
> >> +    buf = qemu_mallocz(count+1);
> >> +    read_count = fread(buf, 1, count, fh);
> >> +    if (ferror(fh)) {
> >> +        slog("guest-file-read failed, handle: %ld", handle);
> >> +        error_set(err, QERR_QGA_COMMAND_FAILED, "fread() failed");
> >> +    } else {
> >> +        buf[read_count] = 0;
> >> +        read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
> >
> > Why +1?
> >
> 
> ehhh...no idea. Must've added that erroneously when I was fixing a buf 
> overflow with 'buf = qemu_mallocz(count);'. Will fix.
> 
> >> +        read_data->count = read_count;
> >> +        read_data->eof = feof(fh);
> >> +        if (read_count) {
> >> +            read_data->buf_b64 = g_base64_encode(buf, read_count);
> >> +        }
> >> +    }
> >> +    qemu_free(buf);
> >> +    clearerr(fh);
> >> +
> >> +    return read_data;
> >> +}
> >> +
> >> +GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
> >> +                                     bool has_count, int64_t count, Error **err)
> >> +{
> >> +    GuestFileWrite *write_data = NULL;
> >> +    guchar *buf;
> >> +    gsize buf_len;
> >> +    int write_count;
> >> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> >> +    FILE *fh;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    fh = gfh->fh;
> >> +    buf = g_base64_decode(buf_b64,&buf_len);
> >> +
> >> +    if (!has_count) {
> >> +        count = buf_len;
> >> +    } else if (count<  0 || count>  buf_len) {
> >> +        qemu_free(buf);
> >> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    write_count = fwrite(buf, 1, count, fh);
> >> +    if (ferror(fh)) {
> >> +        slog("guest-file-write failed, handle: %ld", handle);
> >> +        error_set(err, QERR_QGA_COMMAND_FAILED, "fwrite() error");
> >> +    } else {
> >> +        write_data = qemu_mallocz(sizeof(GuestFileWrite));
> >> +        write_data->count = write_count;
> >> +        write_data->eof = feof(fh);
> >> +    }
> >> +    qemu_free(buf);
> >> +    clearerr(fh);
> >> +
> >> +    return write_data;
> >> +}
> >> +
> >> +struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
> >> +                                          int64_t whence, Error **err)
> >> +{
> >> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> >> +    GuestFileSeek *seek_data = NULL;
> >> +    FILE *fh;
> >> +    int ret;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    fh = gfh->fh;
> >> +    ret = fseek(fh, offset, whence);
> >> +    if (ret == -1) {
> >> +        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
> >> +    } else {
> >> +        seek_data = qemu_mallocz(sizeof(GuestFileRead));
> >> +        seek_data->position = ftell(fh);
> >> +        seek_data->eof = feof(fh);
> >> +    }
> >> +    clearerr(fh);
> >> +
> >> +    return seek_data;
> >> +}
> >> +
> >> +void qmp_guest_file_flush(int64_t handle, Error **err)
> >> +{
> >> +    GuestFileHandle *gfh = guest_file_handle_find(handle);
> >> +    FILE *fh;
> >> +    int ret;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "handle");
> >> +        return;
> >> +    }
> >> +
> >> +    fh = gfh->fh;
> >> +    ret = fflush(fh);
> >> +    if (ret == EOF) {
> >> +        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
> >> +    }
> >> +}
> >> +
> >> +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;
> >> +
> >> +    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);
> >> +    }
> >
> > Why is this needed? Shouldn't this be done on qmp_guest_fsfreeze_thaw()?
> >
> 
> Thaw might leave them in an error state, and the only recourse might be 
> to retry the thaw on the cached mount list after manual intervening on 
> the guest. So the list needs to stick around else we'd have to rebuild 
> it in thaw, which means it may not be in sync with the list we were 
> using when doing the initial freeze.

Ok.

> 
> >> +
> >> +    fp = setmntent(mtab, "r");
> >> +    if (!fp) {
> >> +        g_warning("fsfreeze: unable to read mtab");
> >> +        return -1;
> >> +    }
> >> +
> >> +    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;
> >> +}
> >> +
> >> +/*
> >> + * 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;
> >> +    char err_msg[512];
> >> +
> >> +    slog("guest-fsfreeze called");
> >> +
> >> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    ret = guest_fsfreeze_build_mount_list();
> >> +    if (ret<  0) {
> >> +        return ret;
> >> +    }
> >> +
> >> +    /* 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) {
> >> +            sprintf(err_msg, "failed to open %s, %s", mount->dirname, strerror(errno));
> >> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> >> +            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 report 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) {
> >> +            sprintf(err_msg, "failed to freeze %s, %s", mount->dirname, strerror(errno));
> >> +            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
> >> +            close(fd);
> >> +            goto error;
> >> +        }
> >> +        close(fd);
> >> +
> >> +        i++;
> >> +    }
> >> +
> >> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> >> +    return i;
> >> +
> >> +error:
> >> +    if (i>  0) {
> >> +        qmp_guest_fsfreeze_thaw(NULL);
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +/*
> >> + * 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;
> >> +    bool has_error = false;
> >> +
> >> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >> +        if (fd == -1) {
> >> +            has_error = true;
> >> +            continue;
> >> +        }
> >> +        ret = ioctl(fd, FITHAW);
> >> +        if (ret<  0&&  errno != EOPNOTSUPP&&  errno != EINVAL) {
> >> +            has_error = true;
> >> +            close(fd);
> >> +            continue;
> >> +        }
> >> +        close(fd);
> >> +        i++;
> >> +    }
> >> +
> >> +    if (has_error) {
> >> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> >> +    } else {
> >> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >> +    }
> >> +    enable_logging();
> >> +    return i;
> >> +}
> >> +
> >> +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..e42b91d 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_COUNT_DEFAULT 4<<  10
> >>
> >>   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));
> >
>

Patch

diff --git a/Makefile b/Makefile
index b2e8593..f98e127 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-qapi-types.o $(qapi-dir)/qga-qmp-marshal.o
 
 QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
 
diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
new file mode 100644
index 0000000..a18d6f8
--- /dev/null
+++ b/qapi-schema-guest.json
@@ -0,0 +1,217 @@ 
+# *-*- Mode: Python -*-*
+
+##
+# @guest-sync:
+#
+# Echo back a unique integer value
+#
+# This is used by clients talking to the guest agent over the
+# wire to ensure the stream is in sync and doesn't contain stale
+# data from previous client. All guest agent responses should be
+# ignored until the provided unique integer value is returned,
+# and it is up to the client to handle stale whole or
+# partially-delivered JSON text in such a way that this response
+# can be obtained.
+#
+# Such clients should also preceed this command
+# with a 0xFF byte to make such the guest agent flushes any
+# partially read JSON data from a previous session.
+#
+# @id: randomly generated 64-bit integer
+#
+# Returns: The unique integer id passed in by the client
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-sync'
+  'data':    { 'id': 'int' },
+  'returns': 'int' }
+
+##
+# @guest-ping:
+#
+# Ping the guest agent, a non-error return implies success
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-ping' }
+
+##
+# @guest-info:
+#
+# Get some information about the guest agent.
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestAgentInfo', 'data': {'version': 'str'} }
+{ 'command': 'guest-info',
+  'returns': 'GuestAgentInfo' }
+
+##
+# @guest-shutdown:
+#
+# Initiate guest-activated shutdown. Note: this is an asynchronous
+# shutdown request, with no guaruntee of successful shutdown. Errors
+# will be logged to guest's syslog.
+#
+# @mode: #optional "halt", "powerdown", or "reboot" (default)
+#
+# Returns: Nothing on success
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-shutdown', 'data': { '*mode': 'str' } }
+
+##
+# @guest-file-open:
+#
+# Open a file in the guest and retrieve a file handle for it
+#
+# @filepath: Full path to the file in the guest to open.
+#
+# @mode: #optional open mode, as per fopen(), "r" is the default.
+#
+# Returns: Guest file handle on success.
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-file-open',
+  'data':    { 'path': 'str', '*mode': 'str' },
+  'returns': 'int' }
+
+##
+# @guest-file-close:
+#
+# Close an open file in the guest
+#
+# @handle: filehandle returned by guest-file-open
+#
+# Returns: Nothing on success.
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-file-close',
+  'data': { 'handle': 'int' } }
+
+##
+# @guest-file-read:
+#
+# Read from an open file in the guest. Data will be base64-encoded
+#
+# @handle: filehandle returned by guest-file-open
+#
+# @count: #optional maximum number of bytes to read (default is 4KB)
+#
+# Returns: GuestFileRead on success. Note: count is number of bytes read
+#          *before* base64 encoding
+#          bytes read.
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestFileRead',
+  'data': { 'count': 'int', 'buf-b64': 'str', 'eof': 'bool' } }
+
+{ 'command': 'guest-file-read',
+  'data':    { 'handle': 'int', '*count': 'int' },
+  'returns': 'GuestFileRead' }
+
+##
+# @guest-file-write:
+#
+# Write to an open file in the guest.
+#
+# @handle: filehandle returned by guest-file-open
+#
+# @buf-b64: base64-encoded string representing data to be written
+#
+# @count: #optional bytes to write (actual bytes, after base64-decode),
+#         default is all content in buf-b64 buffer after base64 decoding
+#
+# Returns: GuestFileWrite on success. Note: count is the number of 
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestFileWrite',
+  'data': { 'count': 'int', 'eof': 'bool' } }
+{ 'command': 'guest-file-write',
+  'data':    { 'handle': 'int', 'buf-b64': 'str', '*count': 'int' },
+  'returns': 'GuestFileWrite' }
+
+##
+# @guest-file-seek:
+#
+# Seek to a position in the file, as with fseek(), and return the
+# current file position afterward. Also encapsulates ftell()'s
+# functionality, just Set offset=0, whence=SEEK_CUR.
+#
+# @handle: filehandle returned by guest-file-open
+#
+# @offset: bytes to skip over in the file stream
+#
+# @whence: SEEK_SET, SEEK_CUR, or SEEK_END, as with fseek()
+#
+# Returns: GuestFileSeek on success.
+#
+# Since: 0.15.0
+##
+{ 'type': 'GuestFileSeek',
+  'data': { 'position': 'int', 'eof': 'bool' } }
+
+{ 'command': 'guest-file-seek',
+  'data':    { 'handle': 'int', 'offset': 'int', 'whence': 'int' },
+  'returns': 'GuestFileSeek' }
+
+##
+# @guest-file-flush:
+#
+# Write file changes bufferred in userspace to disk/kernel buffers
+#
+# @handle: filehandle returned by guest-file-open
+#
+# Returns: Nothing on success.
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-file-flush',
+  'data': { 'handle': 'int' } }
+
+##
+# @guest-fsfreeze-status:
+#
+# Get guest fsfreeze state. error state indicates failure to thaw 1 or more
+# previously frozen filesystems, or failure to open a previously cached
+# filesytem (filesystem unmounted/directory changes, etc).
+#
+# Returns: GuestFsfreezeStatus (of the form "GUEST_FSFREEZE_STATUS_<STATUS>")
+#
+# Since: 0.15.0
+##
+{ 'enum': 'GuestFsfreezeStatus',
+  'data': [ 'thawed', 'frozen', 'error' ] }
+{ 'command': 'guest-fsfreeze-status',
+  'returns': 'GuestFsfreezeStatus' }
+
+##
+# @guest-fsfreeze-freeze:
+#
+# Sync and freeze all non-network guest filesystems
+#
+# Returns: Number of file systems frozen on success
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-fsfreeze-freeze',
+  'returns': 'int' }
+
+##
+# @guest-fsfreeze-thaw:
+#
+# Unfreeze frozen guest fileystems
+#
+# Returns: Number of file systems thawed
+#          If error, -1 (unknown error) or -errno
+#
+# Since: 0.15.0
+##
+{ 'command': 'guest-fsfreeze-thaw',
+  'returns': 'int' }
diff --git a/qemu-ga.c b/qemu-ga.c
index eb09100..4530d3d 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -653,6 +653,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);
@@ -661,6 +664,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.c b/qerror.c
index c92adfc..229d0d6 100644
--- a/qerror.c
+++ b/qerror.c
@@ -218,6 +218,14 @@  static const QErrorStringTable qerror_table[] = {
         .error_fmt = QERR_VNC_SERVER_FAILED,
         .desc      = "Could not start VNC server on %(target)",
     },
+    {
+        .error_fmt = QERR_QGA_LOGGING_FAILED,
+        .desc      = "Guest agent failed to log non-optional log statement",
+    },
+    {
+        .error_fmt = QERR_QGA_COMMAND_FAILED,
+        .desc      = "Guest agent command failed, error was '%(message)'",
+    },
     {}
 };
 
diff --git a/qerror.h b/qerror.h
index 9a9fa5b..7ec0fc1 100644
--- a/qerror.h
+++ b/qerror.h
@@ -184,4 +184,10 @@  QError *qobject_to_qerror(const QObject *obj);
 #define QERR_FEATURE_DISABLED \
     "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
 
+#define QERR_QGA_LOGGING_FAILED \
+    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
+
+#define QERR_QGA_COMMAND_FAILED \
+    "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
+
 #endif /* QERROR_H */
diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
new file mode 100644
index 0000000..0e885d0
--- /dev/null
+++ b/qga/guest-agent-commands.c
@@ -0,0 +1,512 @@ 
+/*
+ * 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(bool has_mode, const char *mode, Error **err)
+{
+    int ret;
+    const char *shutdown_flag;
+
+    slog("guest-shutdown called, mode: %s", mode);
+    if (!has_mode || strcmp(mode, "reboot") == 0) {
+        shutdown_flag = "-r";
+    } else if (strcmp(mode, "halt") == 0) {
+        shutdown_flag = "-H";
+    } else if (strcmp(mode, "powerdown") == 0) {
+        shutdown_flag = "-P";
+    } 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);
+
+        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 *path, 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", path, mode);
+    fh = fopen(path, mode);
+    if (!fh) {
+        error_set(err, QERR_OPEN_FILE_FAILED, path);
+        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, path);
+        fclose(fh);
+        return -1;
+    }
+
+    guest_file_handle_add(fh);
+    slog("guest-file-open, handle: %d", fd);
+    return fd;
+}
+
+void qmp_guest_file_close(int64_t handle, Error **err)
+{
+    GuestFileHandle *gfh = guest_file_handle_find(handle);
+
+    slog("guest-file-close called, handle: %ld", handle);
+    if (!gfh) {
+        error_set(err, QERR_FD_NOT_FOUND, "handle");
+        return;
+    }
+
+    fclose(gfh->fh);
+    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
+    qemu_free(gfh);
+}
+
+struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
+                                          int64_t count, Error **err)
+{
+    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileRead *read_data = NULL;
+    guchar *buf;
+    FILE *fh;
+    size_t read_count;
+
+    if (!gfh) {
+        error_set(err, QERR_FD_NOT_FOUND, "handle");
+        return NULL;
+    }
+
+    if (!has_count) {
+        count = QGA_READ_COUNT_DEFAULT;
+    } else if (count < 0) {
+        error_set(err, QERR_INVALID_PARAMETER, "count");
+        return NULL;
+    }
+
+    fh = gfh->fh;
+    buf = qemu_mallocz(count+1);
+    read_count = fread(buf, 1, count, fh);
+    if (ferror(fh)) {
+        slog("guest-file-read failed, handle: %ld", handle);
+        error_set(err, QERR_QGA_COMMAND_FAILED, "fread() failed");
+    } else {
+        buf[read_count] = 0;
+        read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
+        read_data->count = read_count;
+        read_data->eof = feof(fh);
+        if (read_count) {
+            read_data->buf_b64 = g_base64_encode(buf, read_count);
+        }
+    }
+    qemu_free(buf);
+    clearerr(fh);
+
+    return read_data;
+}
+
+GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
+                                     bool has_count, int64_t count, Error **err)
+{
+    GuestFileWrite *write_data = NULL;
+    guchar *buf;
+    gsize buf_len;
+    int write_count;
+    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    FILE *fh;
+
+    if (!gfh) {
+        error_set(err, QERR_FD_NOT_FOUND, "handle");
+        return NULL;
+    }
+
+    fh = gfh->fh;
+    buf = g_base64_decode(buf_b64, &buf_len);
+
+    if (!has_count) {
+        count = buf_len;
+    } else if (count < 0 || count > buf_len) {
+        qemu_free(buf);
+        error_set(err, QERR_INVALID_PARAMETER, "count");
+        return NULL;
+    }
+
+    write_count = fwrite(buf, 1, count, fh);
+    if (ferror(fh)) {
+        slog("guest-file-write failed, handle: %ld", handle);
+        error_set(err, QERR_QGA_COMMAND_FAILED, "fwrite() error");
+    } else {
+        write_data = qemu_mallocz(sizeof(GuestFileWrite));
+        write_data->count = write_count;
+        write_data->eof = feof(fh);
+    }
+    qemu_free(buf);
+    clearerr(fh);
+
+    return write_data;
+}
+
+struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
+                                          int64_t whence, Error **err)
+{
+    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    GuestFileSeek *seek_data = NULL;
+    FILE *fh;
+    int ret;
+
+    if (!gfh) {
+        error_set(err, QERR_FD_NOT_FOUND, "handle");
+        return NULL;
+    }
+
+    fh = gfh->fh;
+    ret = fseek(fh, offset, whence);
+    if (ret == -1) {
+        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
+    } else {
+        seek_data = qemu_mallocz(sizeof(GuestFileRead));
+        seek_data->position = ftell(fh);
+        seek_data->eof = feof(fh);
+    }
+    clearerr(fh);
+
+    return seek_data;
+}
+
+void qmp_guest_file_flush(int64_t handle, Error **err)
+{
+    GuestFileHandle *gfh = guest_file_handle_find(handle);
+    FILE *fh;
+    int ret;
+
+    if (!gfh) {
+        error_set(err, QERR_FD_NOT_FOUND, "handle");
+        return;
+    }
+
+    fh = gfh->fh;
+    ret = fflush(fh);
+    if (ret == EOF) {
+        error_set(err, QERR_QGA_COMMAND_FAILED, strerror(errno));
+    }
+}
+
+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;
+
+    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);
+    }
+
+    fp = setmntent(mtab, "r");
+    if (!fp) {
+        g_warning("fsfreeze: unable to read mtab");
+        return -1;
+    }
+
+    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;
+}
+
+/*
+ * 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;
+    char err_msg[512];
+
+    slog("guest-fsfreeze called");
+
+    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
+        return 0;
+    }
+
+    ret = guest_fsfreeze_build_mount_list();
+    if (ret < 0) {
+        return ret;
+    }
+
+    /* 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) {
+            sprintf(err_msg, "failed to open %s, %s", mount->dirname, strerror(errno));
+            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+            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 report 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) {
+            sprintf(err_msg, "failed to freeze %s, %s", mount->dirname, strerror(errno));
+            error_set(err, QERR_QGA_COMMAND_FAILED, err_msg);
+            close(fd);
+            goto error;
+        }
+        close(fd);
+
+        i++;
+    }
+
+    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
+    return i;
+
+error:
+    if (i > 0) {
+        qmp_guest_fsfreeze_thaw(NULL);
+    }
+    return 0;
+}
+
+/*
+ * 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;
+    bool has_error = false;
+
+    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
+        fd = qemu_open(mount->dirname, O_RDONLY);
+        if (fd == -1) {
+            has_error = true;
+            continue;
+        }
+        ret = ioctl(fd, FITHAW);
+        if (ret < 0 && errno != EOPNOTSUPP && errno != EINVAL) {
+            has_error = true;
+            close(fd);
+            continue;
+        }
+        close(fd);
+        i++;
+    }
+
+    if (has_error) {
+        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
+    } else {
+        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
+    }
+    enable_logging();
+    return i;
+}
+
+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..e42b91d 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_COUNT_DEFAULT 4 << 10
 
 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));