[v6,4/4] guest agent: add guest agent RPCs/commands

This adds the initial set of QMP/QAPI commands provided by the guest
agent:

guest-sync
guest-ping
guest-info
guest-shutdown
guest-file-open
guest-file-read
guest-file-write
guest-file-seek
guest-file-close
guest-fsfreeze-freeze
guest-fsfreeze-thaw
guest-fsfreeze-status

The input/output specification for these commands are documented in the
schema.

Example usage:

  host:
    qemu -device virtio-serial \
         -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
         -device virtserialport,chardev=qga0,name=qga0
         ...

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

  guest:
    qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
            -p /var/run/qemu-guest-agent.pid -d

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile                   |   15 ++-
 qemu-ga.c                  |    4 +
 qerror.h                   |    3 +
 qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-core.h     |    2 +
 5 files changed, 523 insertions(+), 2 deletions(-)
 create mode 100644 qga/guest-agent-commands.c

On Tue,  5 Jul 2011 08:21:40 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> This adds the initial set of QMP/QAPI commands provided by the guest
> agent:
> 
> guest-sync
> guest-ping
> guest-info
> guest-shutdown
> guest-file-open
> guest-file-read
> guest-file-write
> guest-file-seek
> guest-file-close
> guest-fsfreeze-freeze
> guest-fsfreeze-thaw
> guest-fsfreeze-status
> 
> The input/output specification for these commands are documented in the
> schema.
> 
> Example usage:
> 
>   host:
>     qemu -device virtio-serial \
>          -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
>          -device virtserialport,chardev=qga0,name=qga0
>          ...
> 
>     echo "{'execute':'guest-info'}" | socat stdio \
>          unix-connect:/tmp/qga0.sock
> 
>   guest:
>     qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
>             -p /var/run/qemu-guest-agent.pid -d
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  Makefile                   |   15 ++-
>  qemu-ga.c                  |    4 +
>  qerror.h                   |    3 +
>  qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-core.h     |    2 +
>  5 files changed, 523 insertions(+), 2 deletions(-)
>  create mode 100644 qga/guest-agent-commands.c
> 
> diff --git a/Makefile b/Makefile
> index b2e8593..7e4f722 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
>  $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
>  	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-" < $<, "  GEN   $@")
>  
> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-" < $<, "  GEN   $@")
> +
>  test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
>  test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>  
>  test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
>  test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
>  
> -QGALIB=qga/guest-agent-command-state.o
> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
> +
> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
>  
> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
>  
>  QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>  
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 649c16a..04ead22 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
>      g_log_set_default_handler(ga_log, s);
>      g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>      s->logging_enabled = true;
> +    s->command_state = ga_command_state_new();
> +    ga_command_state_init(s, s->command_state);
> +    ga_command_state_init_all(s->command_state);
>      ga_state = s;
>  
>      module_call_init(MODULE_INIT_QAPI);
> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
>  
>      g_main_loop_run(ga_state->main_loop);
>  
> +    ga_command_state_cleanup_all(ga_state->command_state);
>      unlink(pidfile);
>  
>      return 0;
> diff --git a/qerror.h b/qerror.h
> index 9a9fa5b..0f618ac 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_FEATURE_DISABLED \
>      "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>  
> +#define QERR_QGA_LOGGING_FAILED \
> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> +
>  #endif /* QERROR_H */
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> new file mode 100644
> index 0000000..42390fb
> --- /dev/null
> +++ b/qga/guest-agent-commands.c
> @@ -0,0 +1,501 @@
> +/*
> + * QEMU Guest Agent commands
> + *
> + * Copyright IBM Corp. 2011
> + *
> + * Authors:
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <glib.h>
> +#include <mntent.h>
> +#include <sys/types.h>
> +#include <sys/ioctl.h>
> +#include <linux/fs.h>
> +#include "qga/guest-agent-core.h"
> +#include "qga-qmp-commands.h"
> +#include "qerror.h"
> +#include "qemu-queue.h"
> +
> +static GAState *ga_state;
> +
> +static void disable_logging(void)
> +{
> +    ga_disable_logging(ga_state);
> +}
> +
> +static void enable_logging(void)
> +{
> +    ga_enable_logging(ga_state);
> +}
> +
> +/* Note: in some situations, like with the fsfreeze, logging may be
> + * temporarilly disabled. if it is necessary that a command be able
> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
> + */
> +static void slog(const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
> +    va_end(ap);
> +}
> +
> +int64_t qmp_guest_sync(int64_t id, Error **errp)
> +{
> +    return id;
> +}
> +
> +void qmp_guest_ping(Error **err)
> +{
> +    slog("guest-ping called");
> +}
> +
> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> +{
> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
> +
> +    info->version = g_strdup(QGA_VERSION);
> +
> +    return info;
> +}
> +
> +void qmp_guest_shutdown(const char *mode, Error **err)
> +{
> +    int ret;
> +    const char *shutdown_flag;
> +
> +    slog("guest-shutdown called, mode: %s", mode);
> +    if (strcmp(mode, "halt") == 0) {
> +        shutdown_flag = "-H";
> +    } else if (strcmp(mode, "powerdown") == 0) {
> +        shutdown_flag = "-P";
> +    } else if (strcmp(mode, "reboot") == 0) {
> +        shutdown_flag = "-r";
> +    } else {
> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
> +                  "halt|powerdown|reboot");
> +        return;
> +    }
> +
> +    ret = fork();
> +    if (ret == 0) {
> +        /* child, start the shutdown */
> +        setsid();
> +        fclose(stdin);
> +        fclose(stdout);
> +        fclose(stderr);
> +
> +        sleep(5);

If we're required to return a response before the shutdown happens, this
is a bug and I'm afraid that the right way to this is a bit complex.

Otherwise we can just leave it out.

> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> +                    "hypervisor initiated shutdown", (char*)NULL);
> +        if (ret) {
> +            slog("guest-shutdown failed: %s", strerror(errno));
> +        }
> +        exit(!!ret);
> +    } else if (ret < 0) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +    }
> +}
> +
> +typedef struct GuestFileHandle {
> +    uint64_t id;
> +    FILE *fh;
> +    QTAILQ_ENTRY(GuestFileHandle) next;
> +} GuestFileHandle;
> +
> +static struct {
> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
> +} guest_file_state;
> +
> +static void guest_file_handle_add(FILE *fh)
> +{
> +    GuestFileHandle *gfh;
> +
> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
> +    gfh->id = fileno(fh);
> +    gfh->fh = fh;
> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> +}
> +
> +static GuestFileHandle *guest_file_handle_find(int64_t id)
> +{
> +    GuestFileHandle *gfh;
> +
> +    QTAILQ_FOREACH(gfh, &guest_file_state.filehandles, next)
> +    {
> +        if (gfh->id == id) {
> +            return gfh;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
> +{
> +    FILE *fh;
> +    int fd;
> +    int64_t ret = -1;
> +
> +    if (!has_mode) {
> +        mode = "r";
> +    }
> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
> +    fh = fopen(filepath, mode);
> +    if (!fh) {
> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> +        return -1;
> +    }
> +
> +    /* set fd non-blocking to avoid common use cases (like reading from a
> +     * named pipe) from hanging the agent
> +     */
> +    fd = fileno(fh);
> +    ret = fcntl(fd, F_GETFL);
> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
> +    if (ret == -1) {
> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> +        fclose(fh);
> +        return -1;
> +    }
> +
> +    guest_file_handle_add(fh);
> +    slog("guest-file-open, filehandle: %d", fd);
> +    return fd;
> +}
> +
> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
> +                                          Error **err)
> +{
> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> +    GuestFileRead *read_data;
> +    guchar *buf;
> +    FILE *fh;
> +    size_t read_count;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> +        return NULL;
> +    }
> +
> +    if (count < 0 || count > QGA_READ_LIMIT) {
> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> +        return NULL;
> +    }

Are we imposing that limit because of the malloc() call below? If that's
the case I think it's wrong, because we don't know the VM (neither the guest)
better than the client.

The best thing we can do here is to limit it to the file size. Additionally
to this we could have a command-line option to allow the sysadmin set his/her
own limit.

> +
> +    fh = gfh->fh;
> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
> +    buf = qemu_mallocz(count+1);
> +    if (!buf) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +        return NULL;
> +    }

qemu_malloc() functions never fail...

> +
> +    read_count = fread(buf, 1, count, fh);

Isn't 'nmemb' and 'size' swapped?

> +    buf[read_count] = 0;
> +    read_data->count = read_count;
> +    read_data->eof = feof(fh);
> +    if (read_count) {
> +        read_data->buf = g_base64_encode(buf, read_count);
> +    }
> +    qemu_free(buf);
> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
> +     * mode, and no real way to differenitate from a real error since we only
> +     * get boolean error flag from ferror()
> +     */
> +    clearerr(fh);
> +
> +    return read_data;
> +}
> +
> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
> +                                     int64_t count, Error **err)
> +{
> +    GuestFileWrite *write_data;
> +    guchar *data;
> +    gsize data_len;
> +    int write_count;
> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> +    FILE *fh;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> +        return NULL;
> +    }
> +
> +    fh = gfh->fh;
> +    data = g_base64_decode(data_b64, &data_len);
> +    if (count > data_len) {
> +        qemu_free(data);
> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> +        return NULL;
> +    }
> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
> +    write_count = fwrite(data, 1, count, fh);
> +    write_data->count = write_count;
> +    write_data->eof = feof(fh);
> +    qemu_free(data);
> +    clearerr(fh);

Shouldn't we check for errors instead of doing this?

Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
here, but that's probably bad).

> +
> +    return write_data;
> +}
> +
> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
> +                                          int64_t whence, Error **err)
> +{
> +    GuestFileSeek *seek_data;
> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> +    FILE *fh;
> +    int ret;
> +
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> +        return NULL;
> +    }
> +
> +    fh = gfh->fh;
> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
> +    ret = fseek(fh, offset, whence);
> +    if (ret == -1) {
> +        error_set(err, QERR_UNDEFINED_ERROR);
> +        qemu_free(seek_data);
> +        return NULL;
> +    }
> +    seek_data->position = ftell(fh);
> +    seek_data->eof = feof(fh);
> +    clearerr(fh);

Again, I don't see why we should do this.

> +
> +    return seek_data;
> +}
> +
> +void qmp_guest_file_close(int64_t filehandle, Error **err)
> +{
> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> +
> +    slog("guest-file-close called, filehandle: %ld", filehandle);
> +    if (!gfh) {
> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> +        return;
> +    }
> +
> +    fclose(gfh->fh);
> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> +    qemu_free(gfh);
> +}
> +
> +static void guest_file_init(void)
> +{
> +    QTAILQ_INIT(&guest_file_state.filehandles);
> +}
> +
> +typedef struct GuestFsfreezeMount {
> +    char *dirname;
> +    char *devtype;
> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
> +} GuestFsfreezeMount;
> +
> +struct {
> +    GuestFsfreezeStatus status;
> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
> +} guest_fsfreeze_state;
> +
> +/*
> + * Walk the mount table and build a list of local file systems
> + */
> +static int guest_fsfreeze_build_mount_list(void)
> +{
> +    struct mntent *ment;
> +    GuestFsfreezeMount *mount, *temp;
> +    char const *mtab = MOUNTED;
> +    FILE *fp;
> +
> +    fp = setmntent(mtab, "r");
> +    if (!fp) {
> +        g_warning("fsfreeze: unable to read mtab");
> +        goto fail;
> +    }
> +
> +    while ((ment = getmntent(fp))) {
> +        /*
> +         * An entry which device name doesn't start with a '/' is
> +         * either a dummy file system or a network file system.
> +         * Add special handling for smbfs and cifs as is done by
> +         * coreutils as well.
> +         */
> +        if ((ment->mnt_fsname[0] != '/') ||
> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
> +            continue;
> +        }
> +
> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
> +        mount->dirname = qemu_strdup(ment->mnt_dir);
> +        mount->devtype = qemu_strdup(ment->mnt_type);
> +
> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
> +    }
> +
> +    endmntent(fp);
> +
> +    return 0;
> +
> +fail:
> +    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> +        qemu_free(mount->dirname);
> +        qemu_free(mount->devtype);
> +        qemu_free(mount);
> +    }

This doesn't seem to be used.

> +
> +    return -1;
> +}
> +
> +/*
> + * Return status of freeze/thaw
> + */
> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> +{
> +    return guest_fsfreeze_state.status;
> +}
> +
> +/*
> + * Walk list of mounted file systems in the guest, and freeze the ones which
> + * are real local file systems.
> + */
> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
> +{
> +    int ret = 0, i = 0;
> +    struct GuestFsfreezeMount *mount, *temp;
> +    int fd;
> +
> +    slog("guest-fsfreeze called");
> +
> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {

return 0;

> +        ret = 0;
> +        goto out;
> +    }
> +
> +    ret = guest_fsfreeze_build_mount_list();
> +    if (ret < 0) {

return ret;

> +        goto out;
> +    }
> +
> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
> +
> +    /* cannot risk guest agent blocking itself on a write in this state */
> +    disable_logging();
> +
> +    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
> +        fd = qemu_open(mount->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = errno;
> +            goto error;
> +        }
> +
> +        /* we try to cull filesytems we know won't work in advance, but other
> +         * filesytems may not implement fsfreeze for less obvious reasons.
> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
> +         * thawing, these filesystems will return an EINVAL instead, due to
> +         * not being in a frozen state. Other filesystem-specific
> +         * errors may result in EINVAL, however, so the user should check the
> +         * number * of filesystems returned here against those returned by the
> +         * thaw operation to determine whether everything completed
> +         * successfully
> +         */
> +        ret = ioctl(fd, FIFREEZE);
> +        if (ret < 0 && errno != EOPNOTSUPP) {
> +            close(fd);
> +            goto error;
> +        }
> +        close(fd);
> +
> +        i++;
> +    }
> +
> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> +    ret = i;
> +out:
> +    return ret;
> +error:
> +    if (i > 0) {
> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> +    }

Shouldn't you undo everything that has been done so far? Which is
freeing the build list and thawing the file-systems that were frozen
before the error?

> +    goto out;
> +}
> +
> +/*
> + * Walk list of frozen file systems in the guest, and thaw them.
> + */
> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> +{
> +    int ret;
> +    GuestFsfreezeMount *mount, *temp;
> +    int fd, i = 0;
> +
> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN &&
> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {

I don't follow why we're checking against INPROGRESS here.

> +        ret = 0;
> +        goto out_enable_logging;
> +    }
> +
> +    QTAILQ_FOREACH_SAFE(mount, &guest_fsfreeze_state.mount_list, next, temp) {
> +        fd = qemu_open(mount->dirname, O_RDONLY);
> +        if (fd == -1) {
> +            ret = -errno;
> +            goto out;
> +        }
> +        ret = ioctl(fd, FITHAW);
> +        if (ret < 0 && errno != EOPNOTSUPP && errno != EINVAL) {
> +            ret = -errno;
> +            close(fd);
> +            goto out;

Shouldn't you continue and try to thaw the other file-systems in the list?

> +        }
> +        close(fd);
> +
> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> +        qemu_free(mount->dirname);
> +        qemu_free(mount->devtype);
> +        qemu_free(mount);
> +        i++;
> +    }
> +
> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> +    ret = i;
> +out_enable_logging:
> +    enable_logging();
> +out:
> +    return ret;
> +}
> +
> +static void guest_fsfreeze_init(void)
> +{
> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
> +}
> +
> +static void guest_fsfreeze_cleanup(void)
> +{
> +    int64_t ret;
> +    Error *err = NULL;
> +
> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> +        ret = qmp_guest_fsfreeze_thaw(&err);
> +        if (ret < 0 || err) {
> +            slog("failed to clean up frozen filesystems");
> +        }
> +    }
> +}
> +
> +/* register init/cleanup routines for stateful command groups */
> +void ga_command_state_init(GAState *s, GACommandState *cs)
> +{
> +    ga_state = s;
> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
> +    ga_command_state_add(cs, guest_file_init, NULL);
> +}
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 66d1729..3501ff4 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -14,10 +14,12 @@
>  #include "qemu-common.h"
>  
>  #define QGA_VERSION "1.0"
> +#define QGA_READ_LIMIT 4 << 20 /* 4MB block size max for chunked reads */
>  
>  typedef struct GAState GAState;
>  typedef struct GACommandState GACommandState;
>  
> +void ga_command_state_init(GAState *s, GACommandState *cs);
>  void ga_command_state_add(GACommandState *cs,
>                            void (*init)(void),
>                            void (*cleanup)(void));

On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> On Tue,  5 Jul 2011 08:21:40 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> This adds the initial set of QMP/QAPI commands provided by the guest
>> agent:
>>
>> guest-sync
>> guest-ping
>> guest-info
>> guest-shutdown
>> guest-file-open
>> guest-file-read
>> guest-file-write
>> guest-file-seek
>> guest-file-close
>> guest-fsfreeze-freeze
>> guest-fsfreeze-thaw
>> guest-fsfreeze-status
>>
>> The input/output specification for these commands are documented in the
>> schema.
>>
>> Example usage:
>>
>>    host:
>>      qemu -device virtio-serial \
>>           -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
>>           -device virtserialport,chardev=qga0,name=qga0
>>           ...
>>
>>      echo "{'execute':'guest-info'}" | socat stdio \
>>           unix-connect:/tmp/qga0.sock
>>
>>    guest:
>>      qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
>>              -p /var/run/qemu-guest-agent.pid -d
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   Makefile                   |   15 ++-
>>   qemu-ga.c                  |    4 +
>>   qerror.h                   |    3 +
>>   qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
>>   qga/guest-agent-core.h     |    2 +
>>   5 files changed, 523 insertions(+), 2 deletions(-)
>>   create mode 100644 qga/guest-agent-commands.c
>>
>> diff --git a/Makefile b/Makefile
>> index b2e8593..7e4f722 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
>>   $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
>>   	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<  $<, "  GEN   $@")
>>
>> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
>> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
>> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
>> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
>> +
>>   test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
>>   test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>>
>>   test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
>>   test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
>>
>> -QGALIB=qga/guest-agent-command-state.o
>> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
>> +
>> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
>>
>> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
>>
>>   QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>>
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> index 649c16a..04ead22 100644
>> --- a/qemu-ga.c
>> +++ b/qemu-ga.c
>> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
>>       g_log_set_default_handler(ga_log, s);
>>       g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>>       s->logging_enabled = true;
>> +    s->command_state = ga_command_state_new();
>> +    ga_command_state_init(s, s->command_state);
>> +    ga_command_state_init_all(s->command_state);
>>       ga_state = s;
>>
>>       module_call_init(MODULE_INIT_QAPI);
>> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
>>
>>       g_main_loop_run(ga_state->main_loop);
>>
>> +    ga_command_state_cleanup_all(ga_state->command_state);
>>       unlink(pidfile);
>>
>>       return 0;
>> diff --git a/qerror.h b/qerror.h
>> index 9a9fa5b..0f618ac 100644
>> --- a/qerror.h
>> +++ b/qerror.h
>> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
>>   #define QERR_FEATURE_DISABLED \
>>       "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>>
>> +#define QERR_QGA_LOGGING_FAILED \
>> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
>> +
>>   #endif /* QERROR_H */
>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>> new file mode 100644
>> index 0000000..42390fb
>> --- /dev/null
>> +++ b/qga/guest-agent-commands.c
>> @@ -0,0 +1,501 @@
>> +/*
>> + * QEMU Guest Agent commands
>> + *
>> + * Copyright IBM Corp. 2011
>> + *
>> + * Authors:
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include<glib.h>
>> +#include<mntent.h>
>> +#include<sys/types.h>
>> +#include<sys/ioctl.h>
>> +#include<linux/fs.h>
>> +#include "qga/guest-agent-core.h"
>> +#include "qga-qmp-commands.h"
>> +#include "qerror.h"
>> +#include "qemu-queue.h"
>> +
>> +static GAState *ga_state;
>> +
>> +static void disable_logging(void)
>> +{
>> +    ga_disable_logging(ga_state);
>> +}
>> +
>> +static void enable_logging(void)
>> +{
>> +    ga_enable_logging(ga_state);
>> +}
>> +
>> +/* Note: in some situations, like with the fsfreeze, logging may be
>> + * temporarilly disabled. if it is necessary that a command be able
>> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
>> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
>> + */
>> +static void slog(const char *fmt, ...)
>> +{
>> +    va_list ap;
>> +
>> +    va_start(ap, fmt);
>> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
>> +    va_end(ap);
>> +}
>> +
>> +int64_t qmp_guest_sync(int64_t id, Error **errp)
>> +{
>> +    return id;
>> +}
>> +
>> +void qmp_guest_ping(Error **err)
>> +{
>> +    slog("guest-ping called");
>> +}
>> +
>> +struct GuestAgentInfo *qmp_guest_info(Error **err)
>> +{
>> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
>> +
>> +    info->version = g_strdup(QGA_VERSION);
>> +
>> +    return info;
>> +}
>> +
>> +void qmp_guest_shutdown(const char *mode, Error **err)
>> +{
>> +    int ret;
>> +    const char *shutdown_flag;
>> +
>> +    slog("guest-shutdown called, mode: %s", mode);
>> +    if (strcmp(mode, "halt") == 0) {
>> +        shutdown_flag = "-H";
>> +    } else if (strcmp(mode, "powerdown") == 0) {
>> +        shutdown_flag = "-P";
>> +    } else if (strcmp(mode, "reboot") == 0) {
>> +        shutdown_flag = "-r";
>> +    } else {
>> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
>> +                  "halt|powerdown|reboot");
>> +        return;
>> +    }
>> +
>> +    ret = fork();
>> +    if (ret == 0) {
>> +        /* child, start the shutdown */
>> +        setsid();
>> +        fclose(stdin);
>> +        fclose(stdout);
>> +        fclose(stderr);
>> +
>> +        sleep(5);
>
> If we're required to return a response before the shutdown happens, this
> is a bug and I'm afraid that the right way to this is a bit complex.
>
> Otherwise we can just leave it out.
>

Yah, I ran this by Anthony and Adam and just leaving it out seems to be 
the preferred approach, for now at least.

>> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
>> +                    "hypervisor initiated shutdown", (char*)NULL);
>> +        if (ret) {
>> +            slog("guest-shutdown failed: %s", strerror(errno));
>> +        }
>> +        exit(!!ret);
>> +    } else if (ret<  0) {
>> +        error_set(err, QERR_UNDEFINED_ERROR);
>> +    }
>> +}
>> +
>> +typedef struct GuestFileHandle {
>> +    uint64_t id;
>> +    FILE *fh;
>> +    QTAILQ_ENTRY(GuestFileHandle) next;
>> +} GuestFileHandle;
>> +
>> +static struct {
>> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
>> +} guest_file_state;
>> +
>> +static void guest_file_handle_add(FILE *fh)
>> +{
>> +    GuestFileHandle *gfh;
>> +
>> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
>> +    gfh->id = fileno(fh);
>> +    gfh->fh = fh;
>> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
>> +}
>> +
>> +static GuestFileHandle *guest_file_handle_find(int64_t id)
>> +{
>> +    GuestFileHandle *gfh;
>> +
>> +    QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
>> +    {
>> +        if (gfh->id == id) {
>> +            return gfh;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
>> +{
>> +    FILE *fh;
>> +    int fd;
>> +    int64_t ret = -1;
>> +
>> +    if (!has_mode) {
>> +        mode = "r";
>> +    }
>> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
>> +    fh = fopen(filepath, mode);
>> +    if (!fh) {
>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
>> +        return -1;
>> +    }
>> +
>> +    /* set fd non-blocking to avoid common use cases (like reading from a
>> +     * named pipe) from hanging the agent
>> +     */
>> +    fd = fileno(fh);
>> +    ret = fcntl(fd, F_GETFL);
>> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
>> +    if (ret == -1) {
>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
>> +        fclose(fh);
>> +        return -1;
>> +    }
>> +
>> +    guest_file_handle_add(fh);
>> +    slog("guest-file-open, filehandle: %d", fd);
>> +    return fd;
>> +}
>> +
>> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
>> +                                          Error **err)
>> +{
>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>> +    GuestFileRead *read_data;
>> +    guchar *buf;
>> +    FILE *fh;
>> +    size_t read_count;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>> +        return NULL;
>> +    }
>> +
>> +    if (count<  0 || count>  QGA_READ_LIMIT) {
>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>> +        return NULL;
>> +    }
>
> Are we imposing that limit because of the malloc() call below? If that's
> the case I think it's wrong, because we don't know the VM (neither the guest)
> better than the client.
>
> The best thing we can do here is to limit it to the file size. Additionally
> to this we could have a command-line option to allow the sysadmin set his/her
> own limit.
>

That's technically the better approach, but we're also bound by the 
maximum token size limit in the JSON parser, which is 64MB. Best we can 
do is set QGA_READ_LIMIT accordingly.

>> +
>> +    fh = gfh->fh;
>> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
>> +    buf = qemu_mallocz(count+1);
>> +    if (!buf) {
>> +        error_set(err, QERR_UNDEFINED_ERROR);
>> +        return NULL;
>> +    }
>
> qemu_malloc() functions never fail...
>
>> +
>> +    read_count = fread(buf, 1, count, fh);
>
> Isn't 'nmemb' and 'size' swapped?
>

I'm not sure...

size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);

I wrote it as $count number of 1-bytes elements. This seems logical to 
me, since it's a non-blocking FD which way result in a partial of some 
lesser number of bytes than count.

>> +    buf[read_count] = 0;
>> +    read_data->count = read_count;
>> +    read_data->eof = feof(fh);
>> +    if (read_count) {
>> +        read_data->buf = g_base64_encode(buf, read_count);
>> +    }
>> +    qemu_free(buf);
>> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
>> +     * mode, and no real way to differenitate from a real error since we only
>> +     * get boolean error flag from ferror()
>> +     */
>> +    clearerr(fh);
>> +
>> +    return read_data;
>> +}
>> +
>> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
>> +                                     int64_t count, Error **err)
>> +{
>> +    GuestFileWrite *write_data;
>> +    guchar *data;
>> +    gsize data_len;
>> +    int write_count;
>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>> +    FILE *fh;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>> +        return NULL;
>> +    }
>> +
>> +    fh = gfh->fh;
>> +    data = g_base64_decode(data_b64,&data_len);
>> +    if (count>  data_len) {
>> +        qemu_free(data);
>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>> +        return NULL;
>> +    }
>> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
>> +    write_count = fwrite(data, 1, count, fh);
>> +    write_data->count = write_count;
>> +    write_data->eof = feof(fh);
>> +    qemu_free(data);
>> +    clearerr(fh);
>
> Shouldn't we check for errors instead of doing this?
>

Yah...unfortunately we only get a boolean flag with ferror() so it's not 
all that useful, but I can add an error flag to the calls to capture it 
at least. clearerr() is only being used here to clear the eof flag.

> Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
> here, but that's probably bad).
>

Yah, I'd been planning on adding a guest-file-flush() for a while now. 
I'll add that for the respin.

>> +
>> +    return write_data;
>> +}
>> +
>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
>> +                                          int64_t whence, Error **err)
>> +{
>> +    GuestFileSeek *seek_data;
>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>> +    FILE *fh;
>> +    int ret;
>> +
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>> +        return NULL;
>> +    }
>> +
>> +    fh = gfh->fh;
>> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
>> +    ret = fseek(fh, offset, whence);
>> +    if (ret == -1) {
>> +        error_set(err, QERR_UNDEFINED_ERROR);
>> +        qemu_free(seek_data);
>> +        return NULL;
>> +    }
>> +    seek_data->position = ftell(fh);
>> +    seek_data->eof = feof(fh);
>> +    clearerr(fh);
>
> Again, I don't see why we should do this.
>
>> +
>> +    return seek_data;
>> +}
>> +
>> +void qmp_guest_file_close(int64_t filehandle, Error **err)
>> +{
>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>> +
>> +    slog("guest-file-close called, filehandle: %ld", filehandle);
>> +    if (!gfh) {
>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>> +        return;
>> +    }
>> +
>> +    fclose(gfh->fh);
>> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
>> +    qemu_free(gfh);
>> +}
>> +
>> +static void guest_file_init(void)
>> +{
>> +    QTAILQ_INIT(&guest_file_state.filehandles);
>> +}
>> +
>> +typedef struct GuestFsfreezeMount {
>> +    char *dirname;
>> +    char *devtype;
>> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
>> +} GuestFsfreezeMount;
>> +
>> +struct {
>> +    GuestFsfreezeStatus status;
>> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
>> +} guest_fsfreeze_state;
>> +
>> +/*
>> + * Walk the mount table and build a list of local file systems
>> + */
>> +static int guest_fsfreeze_build_mount_list(void)
>> +{
>> +    struct mntent *ment;
>> +    GuestFsfreezeMount *mount, *temp;
>> +    char const *mtab = MOUNTED;
>> +    FILE *fp;
>> +
>> +    fp = setmntent(mtab, "r");
>> +    if (!fp) {
>> +        g_warning("fsfreeze: unable to read mtab");
>> +        goto fail;
>> +    }
>> +
>> +    while ((ment = getmntent(fp))) {
>> +        /*
>> +         * An entry which device name doesn't start with a '/' is
>> +         * either a dummy file system or a network file system.
>> +         * Add special handling for smbfs and cifs as is done by
>> +         * coreutils as well.
>> +         */
>> +        if ((ment->mnt_fsname[0] != '/') ||
>> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
>> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
>> +            continue;
>> +        }
>> +
>> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
>> +        mount->dirname = qemu_strdup(ment->mnt_dir);
>> +        mount->devtype = qemu_strdup(ment->mnt_type);
>> +
>> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
>> +    }
>> +
>> +    endmntent(fp);
>> +
>> +    return 0;
>> +
>> +fail:
>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
>> +        qemu_free(mount->dirname);
>> +        qemu_free(mount->devtype);
>> +        qemu_free(mount);
>> +    }
>
> This doesn't seem to be used.
>

It can get used even a 2nd invocation of this function gets called that 
results in a goto fail. But looking again this should be done 
unconditionally at the start of the function, since the mount list is 
part of global state now.

>> +
>> +    return -1;
>> +}
>> +
>> +/*
>> + * Return status of freeze/thaw
>> + */
>> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>> +{
>> +    return guest_fsfreeze_state.status;
>> +}
>> +
>> +/*
>> + * Walk list of mounted file systems in the guest, and freeze the ones which
>> + * are real local file systems.
>> + */
>> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
>> +{
>> +    int ret = 0, i = 0;
>> +    struct GuestFsfreezeMount *mount, *temp;
>> +    int fd;
>> +
>> +    slog("guest-fsfreeze called");
>> +
>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
>
> return 0;
>
>> +        ret = 0;
>> +        goto out;
>> +    }
>> +
>> +    ret = guest_fsfreeze_build_mount_list();
>> +    if (ret<  0) {
>
> return ret;
>
>> +        goto out;
>> +    }
>> +
>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
>> +
>> +    /* cannot risk guest agent blocking itself on a write in this state */
>> +    disable_logging();
>> +
>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>> +        if (fd == -1) {
>> +            ret = errno;
>> +            goto error;
>> +        }
>> +
>> +        /* we try to cull filesytems we know won't work in advance, but other
>> +         * filesytems may not implement fsfreeze for less obvious reasons.
>> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
>> +         * thawing, these filesystems will return an EINVAL instead, due to
>> +         * not being in a frozen state. Other filesystem-specific
>> +         * errors may result in EINVAL, however, so the user should check the
>> +         * number * of filesystems returned here against those returned by the
>> +         * thaw operation to determine whether everything completed
>> +         * successfully
>> +         */
>> +        ret = ioctl(fd, FIFREEZE);
>> +        if (ret<  0&&  errno != EOPNOTSUPP) {
>> +            close(fd);
>> +            goto error;
>> +        }
>> +        close(fd);
>> +
>> +        i++;
>> +    }
>> +
>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
>> +    ret = i;
>> +out:
>> +    return ret;
>> +error:
>> +    if (i>  0) {
>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
>> +    }
>
> Shouldn't you undo everything that has been done so far? Which is
> freeing the build list and thawing the file-systems that were frozen
> before the error?
>

We can...the way it's done right now is you get a count of how many 
filesystems were frozen, along an error status. Depending on the 
error/count the user can either ignore the error, do what it is they 
want to do, then call thaw(), or just immediately call thaw().

So we can do an automatic thaw() on their behalf, but i figured the 
status was good enough.

>> +    goto out;
>> +}
>> +
>> +/*
>> + * Walk list of frozen file systems in the guest, and thaw them.
>> + */
>> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
>> +{
>> +    int ret;
>> +    GuestFsfreezeMount *mount, *temp;
>> +    int fd, i = 0;
>> +
>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
>> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
>
> I don't follow why we're checking against INPROGRESS here.
>

To prevent a race I believe...but we're synchronous now so that's 
probably no longer needed. I'll look it over and remove it if that's the 
case.

>> +        ret = 0;
>> +        goto out_enable_logging;
>> +    }
>> +
>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>> +        if (fd == -1) {
>> +            ret = -errno;
>> +            goto out;
>> +        }
>> +        ret = ioctl(fd, FITHAW);
>> +        if (ret<  0&&  errno != EOPNOTSUPP&&  errno != EINVAL) {
>> +            ret = -errno;
>> +            close(fd);
>> +            goto out;
>
> Shouldn't you continue and try to thaw the other file-systems in the list?
>

That's probably better

>> +        }
>> +        close(fd);
>> +
>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
>> +        qemu_free(mount->dirname);
>> +        qemu_free(mount->devtype);
>> +        qemu_free(mount);
>> +        i++;
>> +    }
>> +
>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>> +    ret = i;
>> +out_enable_logging:
>> +    enable_logging();
>> +out:
>> +    return ret;
>> +}
>> +
>> +static void guest_fsfreeze_init(void)
>> +{
>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
>> +}
>> +
>> +static void guest_fsfreeze_cleanup(void)
>> +{
>> +    int64_t ret;
>> +    Error *err = NULL;
>> +
>> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
>> +        ret = qmp_guest_fsfreeze_thaw(&err);
>> +        if (ret<  0 || err) {
>> +            slog("failed to clean up frozen filesystems");
>> +        }
>> +    }
>> +}
>> +
>> +/* register init/cleanup routines for stateful command groups */
>> +void ga_command_state_init(GAState *s, GACommandState *cs)
>> +{
>> +    ga_state = s;
>> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
>> +    ga_command_state_add(cs, guest_file_init, NULL);
>> +}
>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>> index 66d1729..3501ff4 100644
>> --- a/qga/guest-agent-core.h
>> +++ b/qga/guest-agent-core.h
>> @@ -14,10 +14,12 @@
>>   #include "qemu-common.h"
>>
>>   #define QGA_VERSION "1.0"
>> +#define QGA_READ_LIMIT 4<<  20 /* 4MB block size max for chunked reads */
>>
>>   typedef struct GAState GAState;
>>   typedef struct GACommandState GACommandState;
>>
>> +void ga_command_state_init(GAState *s, GACommandState *cs);
>>   void ga_command_state_add(GACommandState *cs,
>>                             void (*init)(void),
>>                             void (*cleanup)(void));
>

On Mon, 11 Jul 2011 15:11:26 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> > On Tue,  5 Jul 2011 08:21:40 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> This adds the initial set of QMP/QAPI commands provided by the guest
> >> agent:
> >>
> >> guest-sync
> >> guest-ping
> >> guest-info
> >> guest-shutdown
> >> guest-file-open
> >> guest-file-read
> >> guest-file-write
> >> guest-file-seek
> >> guest-file-close
> >> guest-fsfreeze-freeze
> >> guest-fsfreeze-thaw
> >> guest-fsfreeze-status
> >>
> >> The input/output specification for these commands are documented in the
> >> schema.
> >>
> >> Example usage:
> >>
> >>    host:
> >>      qemu -device virtio-serial \
> >>           -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
> >>           -device virtserialport,chardev=qga0,name=qga0
> >>           ...
> >>
> >>      echo "{'execute':'guest-info'}" | socat stdio \
> >>           unix-connect:/tmp/qga0.sock
> >>
> >>    guest:
> >>      qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
> >>              -p /var/run/qemu-guest-agent.pid -d
> >>
> >> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >> ---
> >>   Makefile                   |   15 ++-
> >>   qemu-ga.c                  |    4 +
> >>   qerror.h                   |    3 +
> >>   qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
> >>   qga/guest-agent-core.h     |    2 +
> >>   5 files changed, 523 insertions(+), 2 deletions(-)
> >>   create mode 100644 qga/guest-agent-commands.c
> >>
> >> diff --git a/Makefile b/Makefile
> >> index b2e8593..7e4f722 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
> >>   $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
> >>   	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<  $<, "  GEN   $@")
> >>
> >> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
> >> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
> >> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
> >> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> >> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
> >> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
> >> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
> >> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<  $<, "  GEN   $@")
> >> +
> >>   test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
> >>   test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> >>
> >>   test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
> >>   test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
> >>
> >> -QGALIB=qga/guest-agent-command-state.o
> >> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
> >> +
> >> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
> >>
> >> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
> >> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
> >>
> >>   QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
> >>
> >> diff --git a/qemu-ga.c b/qemu-ga.c
> >> index 649c16a..04ead22 100644
> >> --- a/qemu-ga.c
> >> +++ b/qemu-ga.c
> >> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
> >>       g_log_set_default_handler(ga_log, s);
> >>       g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> >>       s->logging_enabled = true;
> >> +    s->command_state = ga_command_state_new();
> >> +    ga_command_state_init(s, s->command_state);
> >> +    ga_command_state_init_all(s->command_state);
> >>       ga_state = s;
> >>
> >>       module_call_init(MODULE_INIT_QAPI);
> >> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
> >>
> >>       g_main_loop_run(ga_state->main_loop);
> >>
> >> +    ga_command_state_cleanup_all(ga_state->command_state);
> >>       unlink(pidfile);
> >>
> >>       return 0;
> >> diff --git a/qerror.h b/qerror.h
> >> index 9a9fa5b..0f618ac 100644
> >> --- a/qerror.h
> >> +++ b/qerror.h
> >> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
> >>   #define QERR_FEATURE_DISABLED \
> >>       "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
> >>
> >> +#define QERR_QGA_LOGGING_FAILED \
> >> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> >> +
> >>   #endif /* QERROR_H */
> >> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >> new file mode 100644
> >> index 0000000..42390fb
> >> --- /dev/null
> >> +++ b/qga/guest-agent-commands.c
> >> @@ -0,0 +1,501 @@
> >> +/*
> >> + * QEMU Guest Agent commands
> >> + *
> >> + * Copyright IBM Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include<glib.h>
> >> +#include<mntent.h>
> >> +#include<sys/types.h>
> >> +#include<sys/ioctl.h>
> >> +#include<linux/fs.h>
> >> +#include "qga/guest-agent-core.h"
> >> +#include "qga-qmp-commands.h"
> >> +#include "qerror.h"
> >> +#include "qemu-queue.h"
> >> +
> >> +static GAState *ga_state;
> >> +
> >> +static void disable_logging(void)
> >> +{
> >> +    ga_disable_logging(ga_state);
> >> +}
> >> +
> >> +static void enable_logging(void)
> >> +{
> >> +    ga_enable_logging(ga_state);
> >> +}
> >> +
> >> +/* Note: in some situations, like with the fsfreeze, logging may be
> >> + * temporarilly disabled. if it is necessary that a command be able
> >> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
> >> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
> >> + */
> >> +static void slog(const char *fmt, ...)
> >> +{
> >> +    va_list ap;
> >> +
> >> +    va_start(ap, fmt);
> >> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
> >> +    va_end(ap);
> >> +}
> >> +
> >> +int64_t qmp_guest_sync(int64_t id, Error **errp)
> >> +{
> >> +    return id;
> >> +}
> >> +
> >> +void qmp_guest_ping(Error **err)
> >> +{
> >> +    slog("guest-ping called");
> >> +}
> >> +
> >> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> >> +{
> >> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
> >> +
> >> +    info->version = g_strdup(QGA_VERSION);
> >> +
> >> +    return info;
> >> +}
> >> +
> >> +void qmp_guest_shutdown(const char *mode, Error **err)
> >> +{
> >> +    int ret;
> >> +    const char *shutdown_flag;
> >> +
> >> +    slog("guest-shutdown called, mode: %s", mode);
> >> +    if (strcmp(mode, "halt") == 0) {
> >> +        shutdown_flag = "-H";
> >> +    } else if (strcmp(mode, "powerdown") == 0) {
> >> +        shutdown_flag = "-P";
> >> +    } else if (strcmp(mode, "reboot") == 0) {
> >> +        shutdown_flag = "-r";
> >> +    } else {
> >> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
> >> +                  "halt|powerdown|reboot");
> >> +        return;
> >> +    }
> >> +
> >> +    ret = fork();
> >> +    if (ret == 0) {
> >> +        /* child, start the shutdown */
> >> +        setsid();
> >> +        fclose(stdin);
> >> +        fclose(stdout);
> >> +        fclose(stderr);
> >> +
> >> +        sleep(5);
> >
> > If we're required to return a response before the shutdown happens, this
> > is a bug and I'm afraid that the right way to this is a bit complex.
> >
> > Otherwise we can just leave it out.
> >
> 
> Yah, I ran this by Anthony and Adam and just leaving it out seems to be 
> the preferred approach, for now at least.
> 
> >> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> >> +                    "hypervisor initiated shutdown", (char*)NULL);
> >> +        if (ret) {
> >> +            slog("guest-shutdown failed: %s", strerror(errno));
> >> +        }
> >> +        exit(!!ret);
> >> +    } else if (ret<  0) {
> >> +        error_set(err, QERR_UNDEFINED_ERROR);
> >> +    }
> >> +}
> >> +
> >> +typedef struct GuestFileHandle {
> >> +    uint64_t id;
> >> +    FILE *fh;
> >> +    QTAILQ_ENTRY(GuestFileHandle) next;
> >> +} GuestFileHandle;
> >> +
> >> +static struct {
> >> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
> >> +} guest_file_state;
> >> +
> >> +static void guest_file_handle_add(FILE *fh)
> >> +{
> >> +    GuestFileHandle *gfh;
> >> +
> >> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
> >> +    gfh->id = fileno(fh);
> >> +    gfh->fh = fh;
> >> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> >> +}
> >> +
> >> +static GuestFileHandle *guest_file_handle_find(int64_t id)
> >> +{
> >> +    GuestFileHandle *gfh;
> >> +
> >> +    QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
> >> +    {
> >> +        if (gfh->id == id) {
> >> +            return gfh;
> >> +        }
> >> +    }
> >> +
> >> +    return NULL;
> >> +}
> >> +
> >> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
> >> +{
> >> +    FILE *fh;
> >> +    int fd;
> >> +    int64_t ret = -1;
> >> +
> >> +    if (!has_mode) {
> >> +        mode = "r";
> >> +    }
> >> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
> >> +    fh = fopen(filepath, mode);
> >> +    if (!fh) {
> >> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >> +        return -1;
> >> +    }
> >> +
> >> +    /* set fd non-blocking to avoid common use cases (like reading from a
> >> +     * named pipe) from hanging the agent
> >> +     */
> >> +    fd = fileno(fh);
> >> +    ret = fcntl(fd, F_GETFL);
> >> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
> >> +    if (ret == -1) {
> >> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >> +        fclose(fh);
> >> +        return -1;
> >> +    }
> >> +
> >> +    guest_file_handle_add(fh);
> >> +    slog("guest-file-open, filehandle: %d", fd);
> >> +    return fd;
> >> +}
> >> +
> >> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
> >> +                                          Error **err)
> >> +{
> >> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >> +    GuestFileRead *read_data;
> >> +    guchar *buf;
> >> +    FILE *fh;
> >> +    size_t read_count;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    if (count<  0 || count>  QGA_READ_LIMIT) {
> >> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >> +        return NULL;
> >> +    }
> >
> > Are we imposing that limit because of the malloc() call below? If that's
> > the case I think it's wrong, because we don't know the VM (neither the guest)
> > better than the client.
> >
> > The best thing we can do here is to limit it to the file size. Additionally
> > to this we could have a command-line option to allow the sysadmin set his/her
> > own limit.
> >
> 
> That's technically the better approach, but we're also bound by the 
> maximum token size limit in the JSON parser, which is 64MB. Best we can 
> do is set QGA_READ_LIMIT accordingly.

Good point, but I think it's ok to let the parser do this check itself, as
control won't get here anyway. Maybe we should only document that the server
imposes a limit on the token size.

> 
> >> +
> >> +    fh = gfh->fh;
> >> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
> >> +    buf = qemu_mallocz(count+1);
> >> +    if (!buf) {
> >> +        error_set(err, QERR_UNDEFINED_ERROR);
> >> +        return NULL;
> >> +    }
> >
> > qemu_malloc() functions never fail...
> >
> >> +
> >> +    read_count = fread(buf, 1, count, fh);
> >
> > Isn't 'nmemb' and 'size' swapped?
> >
> 
> I'm not sure...
> 
> size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
> 
> I wrote it as $count number of 1-bytes elements. This seems logical to 
> me, since it's a non-blocking FD which way result in a partial of some 
> lesser number of bytes than count.

Ok. I think either way will work.

> 
> >> +    buf[read_count] = 0;
> >> +    read_data->count = read_count;
> >> +    read_data->eof = feof(fh);
> >> +    if (read_count) {
> >> +        read_data->buf = g_base64_encode(buf, read_count);
> >> +    }
> >> +    qemu_free(buf);
> >> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
> >> +     * mode, and no real way to differenitate from a real error since we only
> >> +     * get boolean error flag from ferror()
> >> +     */
> >> +    clearerr(fh);
> >> +
> >> +    return read_data;
> >> +}
> >> +
> >> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
> >> +                                     int64_t count, Error **err)
> >> +{
> >> +    GuestFileWrite *write_data;
> >> +    guchar *data;
> >> +    gsize data_len;
> >> +    int write_count;
> >> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >> +    FILE *fh;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    fh = gfh->fh;
> >> +    data = g_base64_decode(data_b64,&data_len);
> >> +    if (count>  data_len) {
> >> +        qemu_free(data);
> >> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >> +        return NULL;
> >> +    }
> >> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
> >> +    write_count = fwrite(data, 1, count, fh);
> >> +    write_data->count = write_count;
> >> +    write_data->eof = feof(fh);
> >> +    qemu_free(data);
> >> +    clearerr(fh);
> >
> > Shouldn't we check for errors instead of doing this?
> >
> 
> Yah...unfortunately we only get a boolean flag with ferror() so it's not 
> all that useful, but I can add an error flag to the calls to capture it 
> at least. clearerr() is only being used here to clear the eof flag.

I meant to check fwrite()'s return.

> 
> > Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
> > here, but that's probably bad).
> >
> 
> Yah, I'd been planning on adding a guest-file-flush() for a while now. 
> I'll add that for the respin.
> 
> >> +
> >> +    return write_data;
> >> +}
> >> +
> >> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
> >> +                                          int64_t whence, Error **err)
> >> +{
> >> +    GuestFileSeek *seek_data;
> >> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >> +    FILE *fh;
> >> +    int ret;
> >> +
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    fh = gfh->fh;
> >> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
> >> +    ret = fseek(fh, offset, whence);
> >> +    if (ret == -1) {
> >> +        error_set(err, QERR_UNDEFINED_ERROR);
> >> +        qemu_free(seek_data);
> >> +        return NULL;
> >> +    }
> >> +    seek_data->position = ftell(fh);
> >> +    seek_data->eof = feof(fh);
> >> +    clearerr(fh);
> >
> > Again, I don't see why we should do this.

This is probably ok, as we're checking fseek() above.

> >
> >> +
> >> +    return seek_data;
> >> +}
> >> +
> >> +void qmp_guest_file_close(int64_t filehandle, Error **err)
> >> +{
> >> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >> +
> >> +    slog("guest-file-close called, filehandle: %ld", filehandle);
> >> +    if (!gfh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return;
> >> +    }
> >> +
> >> +    fclose(gfh->fh);
> >> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> >> +    qemu_free(gfh);
> >> +}
> >> +
> >> +static void guest_file_init(void)
> >> +{
> >> +    QTAILQ_INIT(&guest_file_state.filehandles);
> >> +}
> >> +
> >> +typedef struct GuestFsfreezeMount {
> >> +    char *dirname;
> >> +    char *devtype;
> >> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
> >> +} GuestFsfreezeMount;
> >> +
> >> +struct {
> >> +    GuestFsfreezeStatus status;
> >> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
> >> +} guest_fsfreeze_state;
> >> +
> >> +/*
> >> + * Walk the mount table and build a list of local file systems
> >> + */
> >> +static int guest_fsfreeze_build_mount_list(void)
> >> +{
> >> +    struct mntent *ment;
> >> +    GuestFsfreezeMount *mount, *temp;
> >> +    char const *mtab = MOUNTED;
> >> +    FILE *fp;
> >> +
> >> +    fp = setmntent(mtab, "r");
> >> +    if (!fp) {
> >> +        g_warning("fsfreeze: unable to read mtab");
> >> +        goto fail;
> >> +    }
> >> +
> >> +    while ((ment = getmntent(fp))) {
> >> +        /*
> >> +         * An entry which device name doesn't start with a '/' is
> >> +         * either a dummy file system or a network file system.
> >> +         * Add special handling for smbfs and cifs as is done by
> >> +         * coreutils as well.
> >> +         */
> >> +        if ((ment->mnt_fsname[0] != '/') ||
> >> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
> >> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
> >> +            continue;
> >> +        }
> >> +
> >> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
> >> +        mount->dirname = qemu_strdup(ment->mnt_dir);
> >> +        mount->devtype = qemu_strdup(ment->mnt_type);
> >> +
> >> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
> >> +    }
> >> +
> >> +    endmntent(fp);
> >> +
> >> +    return 0;
> >> +
> >> +fail:
> >> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >> +        qemu_free(mount->dirname);
> >> +        qemu_free(mount->devtype);
> >> +        qemu_free(mount);
> >> +    }
> >
> > This doesn't seem to be used.
> >
> 
> It can get used even a 2nd invocation of this function gets called that 
> results in a goto fail. But looking again this should be done 
> unconditionally at the start of the function, since the mount list is 
> part of global state now.
> 
> >> +
> >> +    return -1;
> >> +}
> >> +
> >> +/*
> >> + * Return status of freeze/thaw
> >> + */
> >> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> >> +{
> >> +    return guest_fsfreeze_state.status;
> >> +}
> >> +
> >> +/*
> >> + * Walk list of mounted file systems in the guest, and freeze the ones which
> >> + * are real local file systems.
> >> + */
> >> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
> >> +{
> >> +    int ret = 0, i = 0;
> >> +    struct GuestFsfreezeMount *mount, *temp;
> >> +    int fd;
> >> +
> >> +    slog("guest-fsfreeze called");
> >> +
> >> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
> >
> > return 0;
> >
> >> +        ret = 0;
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = guest_fsfreeze_build_mount_list();
> >> +    if (ret<  0) {
> >
> > return ret;
> >
> >> +        goto out;
> >> +    }
> >> +
> >> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
> >> +
> >> +    /* cannot risk guest agent blocking itself on a write in this state */
> >> +    disable_logging();
> >> +
> >> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >> +        if (fd == -1) {
> >> +            ret = errno;
> >> +            goto error;
> >> +        }
> >> +
> >> +        /* we try to cull filesytems we know won't work in advance, but other
> >> +         * filesytems may not implement fsfreeze for less obvious reasons.
> >> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
> >> +         * thawing, these filesystems will return an EINVAL instead, due to
> >> +         * not being in a frozen state. Other filesystem-specific
> >> +         * errors may result in EINVAL, however, so the user should check the
> >> +         * number * of filesystems returned here against those returned by the
> >> +         * thaw operation to determine whether everything completed
> >> +         * successfully
> >> +         */
> >> +        ret = ioctl(fd, FIFREEZE);
> >> +        if (ret<  0&&  errno != EOPNOTSUPP) {
> >> +            close(fd);
> >> +            goto error;
> >> +        }
> >> +        close(fd);
> >> +
> >> +        i++;
> >> +    }
> >> +
> >> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> >> +    ret = i;
> >> +out:
> >> +    return ret;
> >> +error:
> >> +    if (i>  0) {
> >> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> >> +    }
> >
> > Shouldn't you undo everything that has been done so far? Which is
> > freeing the build list and thawing the file-systems that were frozen
> > before the error?
> >
> 
> We can...the way it's done right now is you get a count of how many 
> filesystems were frozen, along an error status. Depending on the 
> error/count the user can either ignore the error, do what it is they 
> want to do, then call thaw(), or just immediately call thaw().

But you don't get the count on error, so it's difficult (if possible) to
learn how many file-systems were frozen.

> 
> So we can do an automatic thaw() on their behalf, but i figured the 
> status was good enough.
> 
> >> +    goto out;
> >> +}
> >> +
> >> +/*
> >> + * Walk list of frozen file systems in the guest, and thaw them.
> >> + */
> >> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >> +{
> >> +    int ret;
> >> +    GuestFsfreezeMount *mount, *temp;
> >> +    int fd, i = 0;
> >> +
> >> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
> >> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
> >
> > I don't follow why we're checking against INPROGRESS here.
> >
> 
> To prevent a race I believe...but we're synchronous now so that's 
> probably no longer needed. I'll look it over and remove it if that's the 
> case.
> 
> >> +        ret = 0;
> >> +        goto out_enable_logging;
> >> +    }
> >> +
> >> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >> +        if (fd == -1) {
> >> +            ret = -errno;
> >> +            goto out;
> >> +        }
> >> +        ret = ioctl(fd, FITHAW);
> >> +        if (ret<  0&&  errno != EOPNOTSUPP&&  errno != EINVAL) {
> >> +            ret = -errno;
> >> +            close(fd);
> >> +            goto out;
> >
> > Shouldn't you continue and try to thaw the other file-systems in the list?
> >
> 
> That's probably better
> 
> >> +        }
> >> +        close(fd);
> >> +
> >> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >> +        qemu_free(mount->dirname);
> >> +        qemu_free(mount->devtype);
> >> +        qemu_free(mount);
> >> +        i++;
> >> +    }
> >> +
> >> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >> +    ret = i;
> >> +out_enable_logging:
> >> +    enable_logging();
> >> +out:
> >> +    return ret;
> >> +}
> >> +
> >> +static void guest_fsfreeze_init(void)
> >> +{
> >> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
> >> +}
> >> +
> >> +static void guest_fsfreeze_cleanup(void)
> >> +{
> >> +    int64_t ret;
> >> +    Error *err = NULL;
> >> +
> >> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> >> +        ret = qmp_guest_fsfreeze_thaw(&err);
> >> +        if (ret<  0 || err) {
> >> +            slog("failed to clean up frozen filesystems");
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +/* register init/cleanup routines for stateful command groups */
> >> +void ga_command_state_init(GAState *s, GACommandState *cs)
> >> +{
> >> +    ga_state = s;
> >> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
> >> +    ga_command_state_add(cs, guest_file_init, NULL);
> >> +}
> >> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >> index 66d1729..3501ff4 100644
> >> --- a/qga/guest-agent-core.h
> >> +++ b/qga/guest-agent-core.h
> >> @@ -14,10 +14,12 @@
> >>   #include "qemu-common.h"
> >>
> >>   #define QGA_VERSION "1.0"
> >> +#define QGA_READ_LIMIT 4<<  20 /* 4MB block size max for chunked reads */
> >>
> >>   typedef struct GAState GAState;
> >>   typedef struct GACommandState GACommandState;
> >>
> >> +void ga_command_state_init(GAState *s, GACommandState *cs);
> >>   void ga_command_state_add(GACommandState *cs,
> >>                             void (*init)(void),
> >>                             void (*cleanup)(void));
> >
>

On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
> On Mon, 11 Jul 2011 15:11:26 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
>>> On Tue,  5 Jul 2011 08:21:40 -0500
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
>>>
>>>> This adds the initial set of QMP/QAPI commands provided by the guest
>>>> agent:
>>>>
>>>> guest-sync
>>>> guest-ping
>>>> guest-info
>>>> guest-shutdown
>>>> guest-file-open
>>>> guest-file-read
>>>> guest-file-write
>>>> guest-file-seek
>>>> guest-file-close
>>>> guest-fsfreeze-freeze
>>>> guest-fsfreeze-thaw
>>>> guest-fsfreeze-status
>>>>
>>>> The input/output specification for these commands are documented in the
>>>> schema.
>>>>
>>>> Example usage:
>>>>
>>>>     host:
>>>>       qemu -device virtio-serial \
>>>>            -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
>>>>            -device virtserialport,chardev=qga0,name=qga0
>>>>            ...
>>>>
>>>>       echo "{'execute':'guest-info'}" | socat stdio \
>>>>            unix-connect:/tmp/qga0.sock
>>>>
>>>>     guest:
>>>>       qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
>>>>               -p /var/run/qemu-guest-agent.pid -d
>>>>
>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>>> ---
>>>>    Makefile                   |   15 ++-
>>>>    qemu-ga.c                  |    4 +
>>>>    qerror.h                   |    3 +
>>>>    qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    qga/guest-agent-core.h     |    2 +
>>>>    5 files changed, 523 insertions(+), 2 deletions(-)
>>>>    create mode 100644 qga/guest-agent-commands.c
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index b2e8593..7e4f722 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
>>>>    $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
>>>>    	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<   $<, "  GEN   $@")
>>>>
>>>> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
>>>> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"<   $<, "  GEN   $@")
>>>> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>>>> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<   $<, "  GEN   $@")
>>>> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<   $<, "  GEN   $@")
>>>> +
>>>>    test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
>>>>    test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>>>>
>>>>    test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
>>>>    test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
>>>>
>>>> -QGALIB=qga/guest-agent-command-state.o
>>>> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
>>>> +
>>>> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
>>>>
>>>> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
>>>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
>>>>
>>>>    QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>>>>
>>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>>> index 649c16a..04ead22 100644
>>>> --- a/qemu-ga.c
>>>> +++ b/qemu-ga.c
>>>> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
>>>>        g_log_set_default_handler(ga_log, s);
>>>>        g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>>>>        s->logging_enabled = true;
>>>> +    s->command_state = ga_command_state_new();
>>>> +    ga_command_state_init(s, s->command_state);
>>>> +    ga_command_state_init_all(s->command_state);
>>>>        ga_state = s;
>>>>
>>>>        module_call_init(MODULE_INIT_QAPI);
>>>> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
>>>>
>>>>        g_main_loop_run(ga_state->main_loop);
>>>>
>>>> +    ga_command_state_cleanup_all(ga_state->command_state);
>>>>        unlink(pidfile);
>>>>
>>>>        return 0;
>>>> diff --git a/qerror.h b/qerror.h
>>>> index 9a9fa5b..0f618ac 100644
>>>> --- a/qerror.h
>>>> +++ b/qerror.h
>>>> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
>>>>    #define QERR_FEATURE_DISABLED \
>>>>        "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>>>>
>>>> +#define QERR_QGA_LOGGING_FAILED \
>>>> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
>>>> +
>>>>    #endif /* QERROR_H */
>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>>> new file mode 100644
>>>> index 0000000..42390fb
>>>> --- /dev/null
>>>> +++ b/qga/guest-agent-commands.c
>>>> @@ -0,0 +1,501 @@
>>>> +/*
>>>> + * QEMU Guest Agent commands
>>>> + *
>>>> + * Copyright IBM Corp. 2011
>>>> + *
>>>> + * Authors:
>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include<glib.h>
>>>> +#include<mntent.h>
>>>> +#include<sys/types.h>
>>>> +#include<sys/ioctl.h>
>>>> +#include<linux/fs.h>
>>>> +#include "qga/guest-agent-core.h"
>>>> +#include "qga-qmp-commands.h"
>>>> +#include "qerror.h"
>>>> +#include "qemu-queue.h"
>>>> +
>>>> +static GAState *ga_state;
>>>> +
>>>> +static void disable_logging(void)
>>>> +{
>>>> +    ga_disable_logging(ga_state);
>>>> +}
>>>> +
>>>> +static void enable_logging(void)
>>>> +{
>>>> +    ga_enable_logging(ga_state);
>>>> +}
>>>> +
>>>> +/* Note: in some situations, like with the fsfreeze, logging may be
>>>> + * temporarilly disabled. if it is necessary that a command be able
>>>> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
>>>> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
>>>> + */
>>>> +static void slog(const char *fmt, ...)
>>>> +{
>>>> +    va_list ap;
>>>> +
>>>> +    va_start(ap, fmt);
>>>> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
>>>> +    va_end(ap);
>>>> +}
>>>> +
>>>> +int64_t qmp_guest_sync(int64_t id, Error **errp)
>>>> +{
>>>> +    return id;
>>>> +}
>>>> +
>>>> +void qmp_guest_ping(Error **err)
>>>> +{
>>>> +    slog("guest-ping called");
>>>> +}
>>>> +
>>>> +struct GuestAgentInfo *qmp_guest_info(Error **err)
>>>> +{
>>>> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
>>>> +
>>>> +    info->version = g_strdup(QGA_VERSION);
>>>> +
>>>> +    return info;
>>>> +}
>>>> +
>>>> +void qmp_guest_shutdown(const char *mode, Error **err)
>>>> +{
>>>> +    int ret;
>>>> +    const char *shutdown_flag;
>>>> +
>>>> +    slog("guest-shutdown called, mode: %s", mode);
>>>> +    if (strcmp(mode, "halt") == 0) {
>>>> +        shutdown_flag = "-H";
>>>> +    } else if (strcmp(mode, "powerdown") == 0) {
>>>> +        shutdown_flag = "-P";
>>>> +    } else if (strcmp(mode, "reboot") == 0) {
>>>> +        shutdown_flag = "-r";
>>>> +    } else {
>>>> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
>>>> +                  "halt|powerdown|reboot");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    ret = fork();
>>>> +    if (ret == 0) {
>>>> +        /* child, start the shutdown */
>>>> +        setsid();
>>>> +        fclose(stdin);
>>>> +        fclose(stdout);
>>>> +        fclose(stderr);
>>>> +
>>>> +        sleep(5);
>>>
>>> If we're required to return a response before the shutdown happens, this
>>> is a bug and I'm afraid that the right way to this is a bit complex.
>>>
>>> Otherwise we can just leave it out.
>>>
>>
>> Yah, I ran this by Anthony and Adam and just leaving it out seems to be
>> the preferred approach, for now at least.
>>
>>>> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
>>>> +                    "hypervisor initiated shutdown", (char*)NULL);
>>>> +        if (ret) {
>>>> +            slog("guest-shutdown failed: %s", strerror(errno));
>>>> +        }
>>>> +        exit(!!ret);
>>>> +    } else if (ret<   0) {
>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
>>>> +    }
>>>> +}
>>>> +
>>>> +typedef struct GuestFileHandle {
>>>> +    uint64_t id;
>>>> +    FILE *fh;
>>>> +    QTAILQ_ENTRY(GuestFileHandle) next;
>>>> +} GuestFileHandle;
>>>> +
>>>> +static struct {
>>>> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
>>>> +} guest_file_state;
>>>> +
>>>> +static void guest_file_handle_add(FILE *fh)
>>>> +{
>>>> +    GuestFileHandle *gfh;
>>>> +
>>>> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
>>>> +    gfh->id = fileno(fh);
>>>> +    gfh->fh = fh;
>>>> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
>>>> +}
>>>> +
>>>> +static GuestFileHandle *guest_file_handle_find(int64_t id)
>>>> +{
>>>> +    GuestFileHandle *gfh;
>>>> +
>>>> +    QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
>>>> +    {
>>>> +        if (gfh->id == id) {
>>>> +            return gfh;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
>>>> +{
>>>> +    FILE *fh;
>>>> +    int fd;
>>>> +    int64_t ret = -1;
>>>> +
>>>> +    if (!has_mode) {
>>>> +        mode = "r";
>>>> +    }
>>>> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
>>>> +    fh = fopen(filepath, mode);
>>>> +    if (!fh) {
>>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    /* set fd non-blocking to avoid common use cases (like reading from a
>>>> +     * named pipe) from hanging the agent
>>>> +     */
>>>> +    fd = fileno(fh);
>>>> +    ret = fcntl(fd, F_GETFL);
>>>> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
>>>> +    if (ret == -1) {
>>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
>>>> +        fclose(fh);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    guest_file_handle_add(fh);
>>>> +    slog("guest-file-open, filehandle: %d", fd);
>>>> +    return fd;
>>>> +}
>>>> +
>>>> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
>>>> +                                          Error **err)
>>>> +{
>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>> +    GuestFileRead *read_data;
>>>> +    guchar *buf;
>>>> +    FILE *fh;
>>>> +    size_t read_count;
>>>> +
>>>> +    if (!gfh) {
>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    if (count<   0 || count>   QGA_READ_LIMIT) {
>>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>>>> +        return NULL;
>>>> +    }
>>>
>>> Are we imposing that limit because of the malloc() call below? If that's
>>> the case I think it's wrong, because we don't know the VM (neither the guest)
>>> better than the client.
>>>
>>> The best thing we can do here is to limit it to the file size. Additionally
>>> to this we could have a command-line option to allow the sysadmin set his/her
>>> own limit.
>>>
>>
>> That's technically the better approach, but we're also bound by the
>> maximum token size limit in the JSON parser, which is 64MB. Best we can
>> do is set QGA_READ_LIMIT accordingly.
>
> Good point, but I think it's ok to let the parser do this check itself, as
> control won't get here anyway. Maybe we should only document that the server
> imposes a limit on the token size.
>

 From a usability perspective I think the QERR_INVALID_PARAMETER, or 
perhaps something more descriptive, is a little nicer. Plus, they won't 
have to wait for 64MB to get streamed back before they get the error :)

>>
>>>> +
>>>> +    fh = gfh->fh;
>>>> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
>>>> +    buf = qemu_mallocz(count+1);
>>>> +    if (!buf) {
>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
>>>> +        return NULL;
>>>> +    }
>>>
>>> qemu_malloc() functions never fail...
>>>
>>>> +
>>>> +    read_count = fread(buf, 1, count, fh);
>>>
>>> Isn't 'nmemb' and 'size' swapped?
>>>
>>
>> I'm not sure...
>>
>> size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
>>
>> I wrote it as $count number of 1-bytes elements. This seems logical to
>> me, since it's a non-blocking FD which way result in a partial of some
>> lesser number of bytes than count.
>
> Ok. I think either way will work.
>
>>
>>>> +    buf[read_count] = 0;
>>>> +    read_data->count = read_count;
>>>> +    read_data->eof = feof(fh);
>>>> +    if (read_count) {
>>>> +        read_data->buf = g_base64_encode(buf, read_count);
>>>> +    }
>>>> +    qemu_free(buf);
>>>> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
>>>> +     * mode, and no real way to differenitate from a real error since we only
>>>> +     * get boolean error flag from ferror()
>>>> +     */
>>>> +    clearerr(fh);
>>>> +
>>>> +    return read_data;
>>>> +}
>>>> +
>>>> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
>>>> +                                     int64_t count, Error **err)
>>>> +{
>>>> +    GuestFileWrite *write_data;
>>>> +    guchar *data;
>>>> +    gsize data_len;
>>>> +    int write_count;
>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>> +    FILE *fh;
>>>> +
>>>> +    if (!gfh) {
>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    fh = gfh->fh;
>>>> +    data = g_base64_decode(data_b64,&data_len);
>>>> +    if (count>   data_len) {
>>>> +        qemu_free(data);
>>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>>>> +        return NULL;
>>>> +    }
>>>> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
>>>> +    write_count = fwrite(data, 1, count, fh);
>>>> +    write_data->count = write_count;
>>>> +    write_data->eof = feof(fh);
>>>> +    qemu_free(data);
>>>> +    clearerr(fh);
>>>
>>> Shouldn't we check for errors instead of doing this?
>>>
>>
>> Yah...unfortunately we only get a boolean flag with ferror() so it's not
>> all that useful, but I can add an error flag to the calls to capture it
>> at least. clearerr() is only being used here to clear the eof flag.
>
> I meant to check fwrite()'s return.
>

Ahh, right. Definitely.

>>
>>> Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
>>> here, but that's probably bad).
>>>
>>
>> Yah, I'd been planning on adding a guest-file-flush() for a while now.
>> I'll add that for the respin.
>>
>>>> +
>>>> +    return write_data;
>>>> +}
>>>> +
>>>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
>>>> +                                          int64_t whence, Error **err)
>>>> +{
>>>> +    GuestFileSeek *seek_data;
>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>> +    FILE *fh;
>>>> +    int ret;
>>>> +
>>>> +    if (!gfh) {
>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    fh = gfh->fh;
>>>> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
>>>> +    ret = fseek(fh, offset, whence);
>>>> +    if (ret == -1) {
>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
>>>> +        qemu_free(seek_data);
>>>> +        return NULL;
>>>> +    }
>>>> +    seek_data->position = ftell(fh);
>>>> +    seek_data->eof = feof(fh);
>>>> +    clearerr(fh);
>>>
>>> Again, I don't see why we should do this.
>
> This is probably ok, as we're checking fseek() above.
>
>>>
>>>> +
>>>> +    return seek_data;
>>>> +}
>>>> +
>>>> +void qmp_guest_file_close(int64_t filehandle, Error **err)
>>>> +{
>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>> +
>>>> +    slog("guest-file-close called, filehandle: %ld", filehandle);
>>>> +    if (!gfh) {
>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    fclose(gfh->fh);
>>>> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
>>>> +    qemu_free(gfh);
>>>> +}
>>>> +
>>>> +static void guest_file_init(void)
>>>> +{
>>>> +    QTAILQ_INIT(&guest_file_state.filehandles);
>>>> +}
>>>> +
>>>> +typedef struct GuestFsfreezeMount {
>>>> +    char *dirname;
>>>> +    char *devtype;
>>>> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
>>>> +} GuestFsfreezeMount;
>>>> +
>>>> +struct {
>>>> +    GuestFsfreezeStatus status;
>>>> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
>>>> +} guest_fsfreeze_state;
>>>> +
>>>> +/*
>>>> + * Walk the mount table and build a list of local file systems
>>>> + */
>>>> +static int guest_fsfreeze_build_mount_list(void)
>>>> +{
>>>> +    struct mntent *ment;
>>>> +    GuestFsfreezeMount *mount, *temp;
>>>> +    char const *mtab = MOUNTED;
>>>> +    FILE *fp;
>>>> +
>>>> +    fp = setmntent(mtab, "r");
>>>> +    if (!fp) {
>>>> +        g_warning("fsfreeze: unable to read mtab");
>>>> +        goto fail;
>>>> +    }
>>>> +
>>>> +    while ((ment = getmntent(fp))) {
>>>> +        /*
>>>> +         * An entry which device name doesn't start with a '/' is
>>>> +         * either a dummy file system or a network file system.
>>>> +         * Add special handling for smbfs and cifs as is done by
>>>> +         * coreutils as well.
>>>> +         */
>>>> +        if ((ment->mnt_fsname[0] != '/') ||
>>>> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
>>>> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
>>>> +            continue;
>>>> +        }
>>>> +
>>>> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
>>>> +        mount->dirname = qemu_strdup(ment->mnt_dir);
>>>> +        mount->devtype = qemu_strdup(ment->mnt_type);
>>>> +
>>>> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
>>>> +    }
>>>> +
>>>> +    endmntent(fp);
>>>> +
>>>> +    return 0;
>>>> +
>>>> +fail:
>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
>>>> +        qemu_free(mount->dirname);
>>>> +        qemu_free(mount->devtype);
>>>> +        qemu_free(mount);
>>>> +    }
>>>
>>> This doesn't seem to be used.
>>>
>>
>> It can get used even a 2nd invocation of this function gets called that
>> results in a goto fail. But looking again this should be done
>> unconditionally at the start of the function, since the mount list is
>> part of global state now.
>>
>>>> +
>>>> +    return -1;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Return status of freeze/thaw
>>>> + */
>>>> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>>>> +{
>>>> +    return guest_fsfreeze_state.status;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Walk list of mounted file systems in the guest, and freeze the ones which
>>>> + * are real local file systems.
>>>> + */
>>>> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
>>>> +{
>>>> +    int ret = 0, i = 0;
>>>> +    struct GuestFsfreezeMount *mount, *temp;
>>>> +    int fd;
>>>> +
>>>> +    slog("guest-fsfreeze called");
>>>> +
>>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
>>>
>>> return 0;
>>>
>>>> +        ret = 0;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    ret = guest_fsfreeze_build_mount_list();
>>>> +    if (ret<   0) {
>>>
>>> return ret;
>>>
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
>>>> +
>>>> +    /* cannot risk guest agent blocking itself on a write in this state */
>>>> +    disable_logging();
>>>> +
>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>>>> +        if (fd == -1) {
>>>> +            ret = errno;
>>>> +            goto error;
>>>> +        }
>>>> +
>>>> +        /* we try to cull filesytems we know won't work in advance, but other
>>>> +         * filesytems may not implement fsfreeze for less obvious reasons.
>>>> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
>>>> +         * thawing, these filesystems will return an EINVAL instead, due to
>>>> +         * not being in a frozen state. Other filesystem-specific
>>>> +         * errors may result in EINVAL, however, so the user should check the
>>>> +         * number * of filesystems returned here against those returned by the
>>>> +         * thaw operation to determine whether everything completed
>>>> +         * successfully
>>>> +         */
>>>> +        ret = ioctl(fd, FIFREEZE);
>>>> +        if (ret<   0&&   errno != EOPNOTSUPP) {
>>>> +            close(fd);
>>>> +            goto error;
>>>> +        }
>>>> +        close(fd);
>>>> +
>>>> +        i++;
>>>> +    }
>>>> +
>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
>>>> +    ret = i;
>>>> +out:
>>>> +    return ret;
>>>> +error:
>>>> +    if (i>   0) {
>>>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
>>>> +    }
>>>
>>> Shouldn't you undo everything that has been done so far? Which is
>>> freeing the build list and thawing the file-systems that were frozen
>>> before the error?
>>>
>>
>> We can...the way it's done right now is you get a count of how many
>> filesystems were frozen, along an error status. Depending on the
>> error/count the user can either ignore the error, do what it is they
>> want to do, then call thaw(), or just immediately call thaw().
>
> But you don't get the count on error, so it's difficult (if possible) to
> learn how many file-systems were frozen.
>

Yah, my mistake. I think the out: label was one line higher, but if we 
did that we lose error information. Automatically unfreezing should be okay.

>>
>> So we can do an automatic thaw() on their behalf, but i figured the
>> status was good enough.
>>
>>>> +    goto out;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Walk list of frozen file systems in the guest, and thaw them.
>>>> + */
>>>> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>>> +{
>>>> +    int ret;
>>>> +    GuestFsfreezeMount *mount, *temp;
>>>> +    int fd, i = 0;
>>>> +
>>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
>>>> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
>>>
>>> I don't follow why we're checking against INPROGRESS here.
>>>
>>
>> To prevent a race I believe...but we're synchronous now so that's
>> probably no longer needed. I'll look it over and remove it if that's the
>> case.
>>
>>>> +        ret = 0;
>>>> +        goto out_enable_logging;
>>>> +    }
>>>> +
>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>>>> +        if (fd == -1) {
>>>> +            ret = -errno;
>>>> +            goto out;
>>>> +        }
>>>> +        ret = ioctl(fd, FITHAW);
>>>> +        if (ret<   0&&   errno != EOPNOTSUPP&&   errno != EINVAL) {
>>>> +            ret = -errno;
>>>> +            close(fd);
>>>> +            goto out;
>>>
>>> Shouldn't you continue and try to thaw the other file-systems in the list?
>>>
>>
>> That's probably better
>>
>>>> +        }
>>>> +        close(fd);
>>>> +
>>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
>>>> +        qemu_free(mount->dirname);
>>>> +        qemu_free(mount->devtype);
>>>> +        qemu_free(mount);
>>>> +        i++;
>>>> +    }
>>>> +
>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>>>> +    ret = i;
>>>> +out_enable_logging:
>>>> +    enable_logging();
>>>> +out:
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static void guest_fsfreeze_init(void)
>>>> +{
>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>>>> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
>>>> +}
>>>> +
>>>> +static void guest_fsfreeze_cleanup(void)
>>>> +{
>>>> +    int64_t ret;
>>>> +    Error *err = NULL;
>>>> +
>>>> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
>>>> +        ret = qmp_guest_fsfreeze_thaw(&err);
>>>> +        if (ret<   0 || err) {
>>>> +            slog("failed to clean up frozen filesystems");
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +/* register init/cleanup routines for stateful command groups */
>>>> +void ga_command_state_init(GAState *s, GACommandState *cs)
>>>> +{
>>>> +    ga_state = s;
>>>> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
>>>> +    ga_command_state_add(cs, guest_file_init, NULL);
>>>> +}
>>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>>>> index 66d1729..3501ff4 100644
>>>> --- a/qga/guest-agent-core.h
>>>> +++ b/qga/guest-agent-core.h
>>>> @@ -14,10 +14,12 @@
>>>>    #include "qemu-common.h"
>>>>
>>>>    #define QGA_VERSION "1.0"
>>>> +#define QGA_READ_LIMIT 4<<   20 /* 4MB block size max for chunked reads */
>>>>
>>>>    typedef struct GAState GAState;
>>>>    typedef struct GACommandState GACommandState;
>>>>
>>>> +void ga_command_state_init(GAState *s, GACommandState *cs);
>>>>    void ga_command_state_add(GACommandState *cs,
>>>>                              void (*init)(void),
>>>>                              void (*cleanup)(void));
>>>
>>
>

On Mon, 11 Jul 2011 18:11:21 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
> > On Mon, 11 Jul 2011 15:11:26 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> >>> On Tue,  5 Jul 2011 08:21:40 -0500
> >>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
> >>>
> >>>> This adds the initial set of QMP/QAPI commands provided by the guest
> >>>> agent:
> >>>>
> >>>> guest-sync
> >>>> guest-ping
> >>>> guest-info
> >>>> guest-shutdown
> >>>> guest-file-open
> >>>> guest-file-read
> >>>> guest-file-write
> >>>> guest-file-seek
> >>>> guest-file-close
> >>>> guest-fsfreeze-freeze
> >>>> guest-fsfreeze-thaw
> >>>> guest-fsfreeze-status
> >>>>
> >>>> The input/output specification for these commands are documented in the
> >>>> schema.
> >>>>
> >>>> Example usage:
> >>>>
> >>>>     host:
> >>>>       qemu -device virtio-serial \
> >>>>            -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
> >>>>            -device virtserialport,chardev=qga0,name=qga0
> >>>>            ...
> >>>>
> >>>>       echo "{'execute':'guest-info'}" | socat stdio \
> >>>>            unix-connect:/tmp/qga0.sock
> >>>>
> >>>>     guest:
> >>>>       qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
> >>>>               -p /var/run/qemu-guest-agent.pid -d
> >>>>
> >>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>> ---
> >>>>    Makefile                   |   15 ++-
> >>>>    qemu-ga.c                  |    4 +
> >>>>    qerror.h                   |    3 +
> >>>>    qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>    qga/guest-agent-core.h     |    2 +
> >>>>    5 files changed, 523 insertions(+), 2 deletions(-)
> >>>>    create mode 100644 qga/guest-agent-commands.c
> >>>>
> >>>> diff --git a/Makefile b/Makefile
> >>>> index b2e8593..7e4f722 100644
> >>>> --- a/Makefile
> >>>> +++ b/Makefile
> >>>> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
> >>>>    $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
> >>>>    	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<   $<, "  GEN   $@")
> >>>>
> >>>> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
> >>>> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
> >>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"<   $<, "  GEN   $@")
> >>>> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> >>>> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
> >>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<   $<, "  GEN   $@")
> >>>> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
> >>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<   $<, "  GEN   $@")
> >>>> +
> >>>>    test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
> >>>>    test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> >>>>
> >>>>    test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
> >>>>    test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
> >>>>
> >>>> -QGALIB=qga/guest-agent-command-state.o
> >>>> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
> >>>> +
> >>>> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
> >>>>
> >>>> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
> >>>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
> >>>>
> >>>>    QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
> >>>>
> >>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>> index 649c16a..04ead22 100644
> >>>> --- a/qemu-ga.c
> >>>> +++ b/qemu-ga.c
> >>>> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
> >>>>        g_log_set_default_handler(ga_log, s);
> >>>>        g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> >>>>        s->logging_enabled = true;
> >>>> +    s->command_state = ga_command_state_new();
> >>>> +    ga_command_state_init(s, s->command_state);
> >>>> +    ga_command_state_init_all(s->command_state);
> >>>>        ga_state = s;
> >>>>
> >>>>        module_call_init(MODULE_INIT_QAPI);
> >>>> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
> >>>>
> >>>>        g_main_loop_run(ga_state->main_loop);
> >>>>
> >>>> +    ga_command_state_cleanup_all(ga_state->command_state);
> >>>>        unlink(pidfile);
> >>>>
> >>>>        return 0;
> >>>> diff --git a/qerror.h b/qerror.h
> >>>> index 9a9fa5b..0f618ac 100644
> >>>> --- a/qerror.h
> >>>> +++ b/qerror.h
> >>>> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
> >>>>    #define QERR_FEATURE_DISABLED \
> >>>>        "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
> >>>>
> >>>> +#define QERR_QGA_LOGGING_FAILED \
> >>>> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> >>>> +
> >>>>    #endif /* QERROR_H */
> >>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >>>> new file mode 100644
> >>>> index 0000000..42390fb
> >>>> --- /dev/null
> >>>> +++ b/qga/guest-agent-commands.c
> >>>> @@ -0,0 +1,501 @@
> >>>> +/*
> >>>> + * QEMU Guest Agent commands
> >>>> + *
> >>>> + * Copyright IBM Corp. 2011
> >>>> + *
> >>>> + * Authors:
> >>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>> + *
> >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>> + * See the COPYING file in the top-level directory.
> >>>> + */
> >>>> +
> >>>> +#include<glib.h>
> >>>> +#include<mntent.h>
> >>>> +#include<sys/types.h>
> >>>> +#include<sys/ioctl.h>
> >>>> +#include<linux/fs.h>
> >>>> +#include "qga/guest-agent-core.h"
> >>>> +#include "qga-qmp-commands.h"
> >>>> +#include "qerror.h"
> >>>> +#include "qemu-queue.h"
> >>>> +
> >>>> +static GAState *ga_state;
> >>>> +
> >>>> +static void disable_logging(void)
> >>>> +{
> >>>> +    ga_disable_logging(ga_state);
> >>>> +}
> >>>> +
> >>>> +static void enable_logging(void)
> >>>> +{
> >>>> +    ga_enable_logging(ga_state);
> >>>> +}
> >>>> +
> >>>> +/* Note: in some situations, like with the fsfreeze, logging may be
> >>>> + * temporarilly disabled. if it is necessary that a command be able
> >>>> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
> >>>> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
> >>>> + */
> >>>> +static void slog(const char *fmt, ...)
> >>>> +{
> >>>> +    va_list ap;
> >>>> +
> >>>> +    va_start(ap, fmt);
> >>>> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
> >>>> +    va_end(ap);
> >>>> +}
> >>>> +
> >>>> +int64_t qmp_guest_sync(int64_t id, Error **errp)
> >>>> +{
> >>>> +    return id;
> >>>> +}
> >>>> +
> >>>> +void qmp_guest_ping(Error **err)
> >>>> +{
> >>>> +    slog("guest-ping called");
> >>>> +}
> >>>> +
> >>>> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> >>>> +{
> >>>> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
> >>>> +
> >>>> +    info->version = g_strdup(QGA_VERSION);
> >>>> +
> >>>> +    return info;
> >>>> +}
> >>>> +
> >>>> +void qmp_guest_shutdown(const char *mode, Error **err)
> >>>> +{
> >>>> +    int ret;
> >>>> +    const char *shutdown_flag;
> >>>> +
> >>>> +    slog("guest-shutdown called, mode: %s", mode);
> >>>> +    if (strcmp(mode, "halt") == 0) {
> >>>> +        shutdown_flag = "-H";
> >>>> +    } else if (strcmp(mode, "powerdown") == 0) {
> >>>> +        shutdown_flag = "-P";
> >>>> +    } else if (strcmp(mode, "reboot") == 0) {
> >>>> +        shutdown_flag = "-r";
> >>>> +    } else {
> >>>> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
> >>>> +                  "halt|powerdown|reboot");
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    ret = fork();
> >>>> +    if (ret == 0) {
> >>>> +        /* child, start the shutdown */
> >>>> +        setsid();
> >>>> +        fclose(stdin);
> >>>> +        fclose(stdout);
> >>>> +        fclose(stderr);
> >>>> +
> >>>> +        sleep(5);
> >>>
> >>> If we're required to return a response before the shutdown happens, this
> >>> is a bug and I'm afraid that the right way to this is a bit complex.
> >>>
> >>> Otherwise we can just leave it out.
> >>>
> >>
> >> Yah, I ran this by Anthony and Adam and just leaving it out seems to be
> >> the preferred approach, for now at least.
> >>
> >>>> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> >>>> +                    "hypervisor initiated shutdown", (char*)NULL);
> >>>> +        if (ret) {
> >>>> +            slog("guest-shutdown failed: %s", strerror(errno));
> >>>> +        }
> >>>> +        exit(!!ret);
> >>>> +    } else if (ret<   0) {
> >>>> +        error_set(err, QERR_UNDEFINED_ERROR);
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +typedef struct GuestFileHandle {
> >>>> +    uint64_t id;
> >>>> +    FILE *fh;
> >>>> +    QTAILQ_ENTRY(GuestFileHandle) next;
> >>>> +} GuestFileHandle;
> >>>> +
> >>>> +static struct {
> >>>> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
> >>>> +} guest_file_state;
> >>>> +
> >>>> +static void guest_file_handle_add(FILE *fh)
> >>>> +{
> >>>> +    GuestFileHandle *gfh;
> >>>> +
> >>>> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
> >>>> +    gfh->id = fileno(fh);
> >>>> +    gfh->fh = fh;
> >>>> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> >>>> +}
> >>>> +
> >>>> +static GuestFileHandle *guest_file_handle_find(int64_t id)
> >>>> +{
> >>>> +    GuestFileHandle *gfh;
> >>>> +
> >>>> +    QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
> >>>> +    {
> >>>> +        if (gfh->id == id) {
> >>>> +            return gfh;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    return NULL;
> >>>> +}
> >>>> +
> >>>> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
> >>>> +{
> >>>> +    FILE *fh;
> >>>> +    int fd;
> >>>> +    int64_t ret = -1;
> >>>> +
> >>>> +    if (!has_mode) {
> >>>> +        mode = "r";
> >>>> +    }
> >>>> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
> >>>> +    fh = fopen(filepath, mode);
> >>>> +    if (!fh) {
> >>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >>>> +        return -1;
> >>>> +    }
> >>>> +
> >>>> +    /* set fd non-blocking to avoid common use cases (like reading from a
> >>>> +     * named pipe) from hanging the agent
> >>>> +     */
> >>>> +    fd = fileno(fh);
> >>>> +    ret = fcntl(fd, F_GETFL);
> >>>> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
> >>>> +    if (ret == -1) {
> >>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >>>> +        fclose(fh);
> >>>> +        return -1;
> >>>> +    }
> >>>> +
> >>>> +    guest_file_handle_add(fh);
> >>>> +    slog("guest-file-open, filehandle: %d", fd);
> >>>> +    return fd;
> >>>> +}
> >>>> +
> >>>> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
> >>>> +                                          Error **err)
> >>>> +{
> >>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>> +    GuestFileRead *read_data;
> >>>> +    guchar *buf;
> >>>> +    FILE *fh;
> >>>> +    size_t read_count;
> >>>> +
> >>>> +    if (!gfh) {
> >>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>> +        return NULL;
> >>>> +    }
> >>>> +
> >>>> +    if (count<   0 || count>   QGA_READ_LIMIT) {
> >>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >>>> +        return NULL;
> >>>> +    }
> >>>
> >>> Are we imposing that limit because of the malloc() call below? If that's
> >>> the case I think it's wrong, because we don't know the VM (neither the guest)
> >>> better than the client.
> >>>
> >>> The best thing we can do here is to limit it to the file size. Additionally
> >>> to this we could have a command-line option to allow the sysadmin set his/her
> >>> own limit.
> >>>
> >>
> >> That's technically the better approach, but we're also bound by the
> >> maximum token size limit in the JSON parser, which is 64MB. Best we can
> >> do is set QGA_READ_LIMIT accordingly.
> >
> > Good point, but I think it's ok to let the parser do this check itself, as
> > control won't get here anyway. Maybe we should only document that the server
> > imposes a limit on the token size.
> >
> 
>  From a usability perspective I think the QERR_INVALID_PARAMETER, or 
> perhaps something more descriptive, is a little nicer. Plus, they won't 
> have to wait for 64MB to get streamed back before they get the error :)

That's why I suggested documenting it. Are we going to add this check
to all commands that make it more likely to reach the parser's limit?
(also note that theoretically all commands can do it).

> 
> >>
> >>>> +
> >>>> +    fh = gfh->fh;
> >>>> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
> >>>> +    buf = qemu_mallocz(count+1);
> >>>> +    if (!buf) {
> >>>> +        error_set(err, QERR_UNDEFINED_ERROR);
> >>>> +        return NULL;
> >>>> +    }
> >>>
> >>> qemu_malloc() functions never fail...
> >>>
> >>>> +
> >>>> +    read_count = fread(buf, 1, count, fh);
> >>>
> >>> Isn't 'nmemb' and 'size' swapped?
> >>>
> >>
> >> I'm not sure...
> >>
> >> size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
> >>
> >> I wrote it as $count number of 1-bytes elements. This seems logical to
> >> me, since it's a non-blocking FD which way result in a partial of some
> >> lesser number of bytes than count.
> >
> > Ok. I think either way will work.
> >
> >>
> >>>> +    buf[read_count] = 0;
> >>>> +    read_data->count = read_count;
> >>>> +    read_data->eof = feof(fh);
> >>>> +    if (read_count) {
> >>>> +        read_data->buf = g_base64_encode(buf, read_count);
> >>>> +    }
> >>>> +    qemu_free(buf);
> >>>> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
> >>>> +     * mode, and no real way to differenitate from a real error since we only
> >>>> +     * get boolean error flag from ferror()
> >>>> +     */
> >>>> +    clearerr(fh);
> >>>> +
> >>>> +    return read_data;
> >>>> +}
> >>>> +
> >>>> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
> >>>> +                                     int64_t count, Error **err)
> >>>> +{
> >>>> +    GuestFileWrite *write_data;
> >>>> +    guchar *data;
> >>>> +    gsize data_len;
> >>>> +    int write_count;
> >>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>> +    FILE *fh;
> >>>> +
> >>>> +    if (!gfh) {
> >>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>> +        return NULL;
> >>>> +    }
> >>>> +
> >>>> +    fh = gfh->fh;
> >>>> +    data = g_base64_decode(data_b64,&data_len);
> >>>> +    if (count>   data_len) {
> >>>> +        qemu_free(data);
> >>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >>>> +        return NULL;
> >>>> +    }
> >>>> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
> >>>> +    write_count = fwrite(data, 1, count, fh);
> >>>> +    write_data->count = write_count;
> >>>> +    write_data->eof = feof(fh);
> >>>> +    qemu_free(data);
> >>>> +    clearerr(fh);
> >>>
> >>> Shouldn't we check for errors instead of doing this?
> >>>
> >>
> >> Yah...unfortunately we only get a boolean flag with ferror() so it's not
> >> all that useful, but I can add an error flag to the calls to capture it
> >> at least. clearerr() is only being used here to clear the eof flag.
> >
> > I meant to check fwrite()'s return.
> >
> 
> Ahh, right. Definitely.
> 
> >>
> >>> Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
> >>> here, but that's probably bad).
> >>>
> >>
> >> Yah, I'd been planning on adding a guest-file-flush() for a while now.
> >> I'll add that for the respin.
> >>
> >>>> +
> >>>> +    return write_data;
> >>>> +}
> >>>> +
> >>>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
> >>>> +                                          int64_t whence, Error **err)
> >>>> +{
> >>>> +    GuestFileSeek *seek_data;
> >>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>> +    FILE *fh;
> >>>> +    int ret;
> >>>> +
> >>>> +    if (!gfh) {
> >>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>> +        return NULL;
> >>>> +    }
> >>>> +
> >>>> +    fh = gfh->fh;
> >>>> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
> >>>> +    ret = fseek(fh, offset, whence);
> >>>> +    if (ret == -1) {
> >>>> +        error_set(err, QERR_UNDEFINED_ERROR);
> >>>> +        qemu_free(seek_data);
> >>>> +        return NULL;
> >>>> +    }
> >>>> +    seek_data->position = ftell(fh);
> >>>> +    seek_data->eof = feof(fh);
> >>>> +    clearerr(fh);
> >>>
> >>> Again, I don't see why we should do this.
> >
> > This is probably ok, as we're checking fseek() above.
> >
> >>>
> >>>> +
> >>>> +    return seek_data;
> >>>> +}
> >>>> +
> >>>> +void qmp_guest_file_close(int64_t filehandle, Error **err)
> >>>> +{
> >>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>> +
> >>>> +    slog("guest-file-close called, filehandle: %ld", filehandle);
> >>>> +    if (!gfh) {
> >>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    fclose(gfh->fh);
> >>>> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> >>>> +    qemu_free(gfh);
> >>>> +}
> >>>> +
> >>>> +static void guest_file_init(void)
> >>>> +{
> >>>> +    QTAILQ_INIT(&guest_file_state.filehandles);
> >>>> +}
> >>>> +
> >>>> +typedef struct GuestFsfreezeMount {
> >>>> +    char *dirname;
> >>>> +    char *devtype;
> >>>> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
> >>>> +} GuestFsfreezeMount;
> >>>> +
> >>>> +struct {
> >>>> +    GuestFsfreezeStatus status;
> >>>> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
> >>>> +} guest_fsfreeze_state;
> >>>> +
> >>>> +/*
> >>>> + * Walk the mount table and build a list of local file systems
> >>>> + */
> >>>> +static int guest_fsfreeze_build_mount_list(void)
> >>>> +{
> >>>> +    struct mntent *ment;
> >>>> +    GuestFsfreezeMount *mount, *temp;
> >>>> +    char const *mtab = MOUNTED;
> >>>> +    FILE *fp;
> >>>> +
> >>>> +    fp = setmntent(mtab, "r");
> >>>> +    if (!fp) {
> >>>> +        g_warning("fsfreeze: unable to read mtab");
> >>>> +        goto fail;
> >>>> +    }
> >>>> +
> >>>> +    while ((ment = getmntent(fp))) {
> >>>> +        /*
> >>>> +         * An entry which device name doesn't start with a '/' is
> >>>> +         * either a dummy file system or a network file system.
> >>>> +         * Add special handling for smbfs and cifs as is done by
> >>>> +         * coreutils as well.
> >>>> +         */
> >>>> +        if ((ment->mnt_fsname[0] != '/') ||
> >>>> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
> >>>> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
> >>>> +            continue;
> >>>> +        }
> >>>> +
> >>>> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
> >>>> +        mount->dirname = qemu_strdup(ment->mnt_dir);
> >>>> +        mount->devtype = qemu_strdup(ment->mnt_type);
> >>>> +
> >>>> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
> >>>> +    }
> >>>> +
> >>>> +    endmntent(fp);
> >>>> +
> >>>> +    return 0;
> >>>> +
> >>>> +fail:
> >>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >>>> +        qemu_free(mount->dirname);
> >>>> +        qemu_free(mount->devtype);
> >>>> +        qemu_free(mount);
> >>>> +    }
> >>>
> >>> This doesn't seem to be used.
> >>>
> >>
> >> It can get used even a 2nd invocation of this function gets called that
> >> results in a goto fail. But looking again this should be done
> >> unconditionally at the start of the function, since the mount list is
> >> part of global state now.
> >>
> >>>> +
> >>>> +    return -1;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Return status of freeze/thaw
> >>>> + */
> >>>> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> >>>> +{
> >>>> +    return guest_fsfreeze_state.status;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Walk list of mounted file systems in the guest, and freeze the ones which
> >>>> + * are real local file systems.
> >>>> + */
> >>>> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
> >>>> +{
> >>>> +    int ret = 0, i = 0;
> >>>> +    struct GuestFsfreezeMount *mount, *temp;
> >>>> +    int fd;
> >>>> +
> >>>> +    slog("guest-fsfreeze called");
> >>>> +
> >>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
> >>>
> >>> return 0;
> >>>
> >>>> +        ret = 0;
> >>>> +        goto out;
> >>>> +    }
> >>>> +
> >>>> +    ret = guest_fsfreeze_build_mount_list();
> >>>> +    if (ret<   0) {
> >>>
> >>> return ret;
> >>>
> >>>> +        goto out;
> >>>> +    }
> >>>> +
> >>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
> >>>> +
> >>>> +    /* cannot risk guest agent blocking itself on a write in this state */
> >>>> +    disable_logging();
> >>>> +
> >>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >>>> +        if (fd == -1) {
> >>>> +            ret = errno;
> >>>> +            goto error;
> >>>> +        }
> >>>> +
> >>>> +        /* we try to cull filesytems we know won't work in advance, but other
> >>>> +         * filesytems may not implement fsfreeze for less obvious reasons.
> >>>> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
> >>>> +         * thawing, these filesystems will return an EINVAL instead, due to
> >>>> +         * not being in a frozen state. Other filesystem-specific
> >>>> +         * errors may result in EINVAL, however, so the user should check the
> >>>> +         * number * of filesystems returned here against those returned by the
> >>>> +         * thaw operation to determine whether everything completed
> >>>> +         * successfully
> >>>> +         */
> >>>> +        ret = ioctl(fd, FIFREEZE);
> >>>> +        if (ret<   0&&   errno != EOPNOTSUPP) {
> >>>> +            close(fd);
> >>>> +            goto error;
> >>>> +        }
> >>>> +        close(fd);
> >>>> +
> >>>> +        i++;
> >>>> +    }
> >>>> +
> >>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> >>>> +    ret = i;
> >>>> +out:
> >>>> +    return ret;
> >>>> +error:
> >>>> +    if (i>   0) {
> >>>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> >>>> +    }
> >>>
> >>> Shouldn't you undo everything that has been done so far? Which is
> >>> freeing the build list and thawing the file-systems that were frozen
> >>> before the error?
> >>>
> >>
> >> We can...the way it's done right now is you get a count of how many
> >> filesystems were frozen, along an error status. Depending on the
> >> error/count the user can either ignore the error, do what it is they
> >> want to do, then call thaw(), or just immediately call thaw().
> >
> > But you don't get the count on error, so it's difficult (if possible) to
> > learn how many file-systems were frozen.
> >
> 
> Yah, my mistake. I think the out: label was one line higher, but if we 
> did that we lose error information. Automatically unfreezing should be okay.
> 
> >>
> >> So we can do an automatic thaw() on their behalf, but i figured the
> >> status was good enough.
> >>
> >>>> +    goto out;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * Walk list of frozen file systems in the guest, and thaw them.
> >>>> + */
> >>>> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >>>> +{
> >>>> +    int ret;
> >>>> +    GuestFsfreezeMount *mount, *temp;
> >>>> +    int fd, i = 0;
> >>>> +
> >>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
> >>>> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
> >>>
> >>> I don't follow why we're checking against INPROGRESS here.
> >>>
> >>
> >> To prevent a race I believe...but we're synchronous now so that's
> >> probably no longer needed. I'll look it over and remove it if that's the
> >> case.
> >>
> >>>> +        ret = 0;
> >>>> +        goto out_enable_logging;
> >>>> +    }
> >>>> +
> >>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >>>> +        if (fd == -1) {
> >>>> +            ret = -errno;
> >>>> +            goto out;
> >>>> +        }
> >>>> +        ret = ioctl(fd, FITHAW);
> >>>> +        if (ret<   0&&   errno != EOPNOTSUPP&&   errno != EINVAL) {
> >>>> +            ret = -errno;
> >>>> +            close(fd);
> >>>> +            goto out;
> >>>
> >>> Shouldn't you continue and try to thaw the other file-systems in the list?
> >>>
> >>
> >> That's probably better
> >>
> >>>> +        }
> >>>> +        close(fd);
> >>>> +
> >>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >>>> +        qemu_free(mount->dirname);
> >>>> +        qemu_free(mount->devtype);
> >>>> +        qemu_free(mount);
> >>>> +        i++;
> >>>> +    }
> >>>> +
> >>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >>>> +    ret = i;
> >>>> +out_enable_logging:
> >>>> +    enable_logging();
> >>>> +out:
> >>>> +    return ret;
> >>>> +}
> >>>> +
> >>>> +static void guest_fsfreeze_init(void)
> >>>> +{
> >>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >>>> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
> >>>> +}
> >>>> +
> >>>> +static void guest_fsfreeze_cleanup(void)
> >>>> +{
> >>>> +    int64_t ret;
> >>>> +    Error *err = NULL;
> >>>> +
> >>>> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> >>>> +        ret = qmp_guest_fsfreeze_thaw(&err);
> >>>> +        if (ret<   0 || err) {
> >>>> +            slog("failed to clean up frozen filesystems");
> >>>> +        }
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +/* register init/cleanup routines for stateful command groups */
> >>>> +void ga_command_state_init(GAState *s, GACommandState *cs)
> >>>> +{
> >>>> +    ga_state = s;
> >>>> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
> >>>> +    ga_command_state_add(cs, guest_file_init, NULL);
> >>>> +}
> >>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >>>> index 66d1729..3501ff4 100644
> >>>> --- a/qga/guest-agent-core.h
> >>>> +++ b/qga/guest-agent-core.h
> >>>> @@ -14,10 +14,12 @@
> >>>>    #include "qemu-common.h"
> >>>>
> >>>>    #define QGA_VERSION "1.0"
> >>>> +#define QGA_READ_LIMIT 4<<   20 /* 4MB block size max for chunked reads */
> >>>>
> >>>>    typedef struct GAState GAState;
> >>>>    typedef struct GACommandState GACommandState;
> >>>>
> >>>> +void ga_command_state_init(GAState *s, GACommandState *cs);
> >>>>    void ga_command_state_add(GACommandState *cs,
> >>>>                              void (*init)(void),
> >>>>                              void (*cleanup)(void));
> >>>
> >>
> >
>

On 07/12/2011 09:15 AM, Luiz Capitulino wrote:
> On Mon, 11 Jul 2011 18:11:21 -0500
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
>>> On Mon, 11 Jul 2011 15:11:26 -0500
>>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
>>>
>>>> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
>>>>> On Tue,  5 Jul 2011 08:21:40 -0500
>>>>> Michael Roth<mdroth@linux.vnet.ibm.com>    wrote:
>>>>>
>>>>>> This adds the initial set of QMP/QAPI commands provided by the guest
>>>>>> agent:
>>>>>>
>>>>>> guest-sync
>>>>>> guest-ping
>>>>>> guest-info
>>>>>> guest-shutdown
>>>>>> guest-file-open
>>>>>> guest-file-read
>>>>>> guest-file-write
>>>>>> guest-file-seek
>>>>>> guest-file-close
>>>>>> guest-fsfreeze-freeze
>>>>>> guest-fsfreeze-thaw
>>>>>> guest-fsfreeze-status
>>>>>>
>>>>>> The input/output specification for these commands are documented in the
>>>>>> schema.
>>>>>>
>>>>>> Example usage:
>>>>>>
>>>>>>      host:
>>>>>>        qemu -device virtio-serial \
>>>>>>             -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
>>>>>>             -device virtserialport,chardev=qga0,name=qga0
>>>>>>             ...
>>>>>>
>>>>>>        echo "{'execute':'guest-info'}" | socat stdio \
>>>>>>             unix-connect:/tmp/qga0.sock
>>>>>>
>>>>>>      guest:
>>>>>>        qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
>>>>>>                -p /var/run/qemu-guest-agent.pid -d
>>>>>>
>>>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>     Makefile                   |   15 ++-
>>>>>>     qemu-ga.c                  |    4 +
>>>>>>     qerror.h                   |    3 +
>>>>>>     qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     qga/guest-agent-core.h     |    2 +
>>>>>>     5 files changed, 523 insertions(+), 2 deletions(-)
>>>>>>     create mode 100644 qga/guest-agent-commands.c
>>>>>>
>>>>>> diff --git a/Makefile b/Makefile
>>>>>> index b2e8593..7e4f722 100644
>>>>>> --- a/Makefile
>>>>>> +++ b/Makefile
>>>>>> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
>>>>>>     $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
>>>>>>     	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<    $<, "  GEN   $@")
>>>>>>
>>>>>> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
>>>>>> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
>>>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"<    $<, "  GEN   $@")
>>>>>> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
>>>>>> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
>>>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<    $<, "  GEN   $@")
>>>>>> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
>>>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<    $<, "  GEN   $@")
>>>>>> +
>>>>>>     test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
>>>>>>     test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
>>>>>>
>>>>>>     test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
>>>>>>     test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
>>>>>>
>>>>>> -QGALIB=qga/guest-agent-command-state.o
>>>>>> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
>>>>>> +
>>>>>> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
>>>>>>
>>>>>> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
>>>>>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
>>>>>>
>>>>>>     QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
>>>>>>
>>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
>>>>>> index 649c16a..04ead22 100644
>>>>>> --- a/qemu-ga.c
>>>>>> +++ b/qemu-ga.c
>>>>>> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
>>>>>>         g_log_set_default_handler(ga_log, s);
>>>>>>         g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
>>>>>>         s->logging_enabled = true;
>>>>>> +    s->command_state = ga_command_state_new();
>>>>>> +    ga_command_state_init(s, s->command_state);
>>>>>> +    ga_command_state_init_all(s->command_state);
>>>>>>         ga_state = s;
>>>>>>
>>>>>>         module_call_init(MODULE_INIT_QAPI);
>>>>>> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
>>>>>>
>>>>>>         g_main_loop_run(ga_state->main_loop);
>>>>>>
>>>>>> +    ga_command_state_cleanup_all(ga_state->command_state);
>>>>>>         unlink(pidfile);
>>>>>>
>>>>>>         return 0;
>>>>>> diff --git a/qerror.h b/qerror.h
>>>>>> index 9a9fa5b..0f618ac 100644
>>>>>> --- a/qerror.h
>>>>>> +++ b/qerror.h
>>>>>> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
>>>>>>     #define QERR_FEATURE_DISABLED \
>>>>>>         "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
>>>>>>
>>>>>> +#define QERR_QGA_LOGGING_FAILED \
>>>>>> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
>>>>>> +
>>>>>>     #endif /* QERROR_H */
>>>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>>>>>> new file mode 100644
>>>>>> index 0000000..42390fb
>>>>>> --- /dev/null
>>>>>> +++ b/qga/guest-agent-commands.c
>>>>>> @@ -0,0 +1,501 @@
>>>>>> +/*
>>>>>> + * QEMU Guest Agent commands
>>>>>> + *
>>>>>> + * Copyright IBM Corp. 2011
>>>>>> + *
>>>>>> + * Authors:
>>>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>>>>>> + *
>>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>>>>> + * See the COPYING file in the top-level directory.
>>>>>> + */
>>>>>> +
>>>>>> +#include<glib.h>
>>>>>> +#include<mntent.h>
>>>>>> +#include<sys/types.h>
>>>>>> +#include<sys/ioctl.h>
>>>>>> +#include<linux/fs.h>
>>>>>> +#include "qga/guest-agent-core.h"
>>>>>> +#include "qga-qmp-commands.h"
>>>>>> +#include "qerror.h"
>>>>>> +#include "qemu-queue.h"
>>>>>> +
>>>>>> +static GAState *ga_state;
>>>>>> +
>>>>>> +static void disable_logging(void)
>>>>>> +{
>>>>>> +    ga_disable_logging(ga_state);
>>>>>> +}
>>>>>> +
>>>>>> +static void enable_logging(void)
>>>>>> +{
>>>>>> +    ga_enable_logging(ga_state);
>>>>>> +}
>>>>>> +
>>>>>> +/* Note: in some situations, like with the fsfreeze, logging may be
>>>>>> + * temporarilly disabled. if it is necessary that a command be able
>>>>>> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
>>>>>> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
>>>>>> + */
>>>>>> +static void slog(const char *fmt, ...)
>>>>>> +{
>>>>>> +    va_list ap;
>>>>>> +
>>>>>> +    va_start(ap, fmt);
>>>>>> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
>>>>>> +    va_end(ap);
>>>>>> +}
>>>>>> +
>>>>>> +int64_t qmp_guest_sync(int64_t id, Error **errp)
>>>>>> +{
>>>>>> +    return id;
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_guest_ping(Error **err)
>>>>>> +{
>>>>>> +    slog("guest-ping called");
>>>>>> +}
>>>>>> +
>>>>>> +struct GuestAgentInfo *qmp_guest_info(Error **err)
>>>>>> +{
>>>>>> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
>>>>>> +
>>>>>> +    info->version = g_strdup(QGA_VERSION);
>>>>>> +
>>>>>> +    return info;
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_guest_shutdown(const char *mode, Error **err)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    const char *shutdown_flag;
>>>>>> +
>>>>>> +    slog("guest-shutdown called, mode: %s", mode);
>>>>>> +    if (strcmp(mode, "halt") == 0) {
>>>>>> +        shutdown_flag = "-H";
>>>>>> +    } else if (strcmp(mode, "powerdown") == 0) {
>>>>>> +        shutdown_flag = "-P";
>>>>>> +    } else if (strcmp(mode, "reboot") == 0) {
>>>>>> +        shutdown_flag = "-r";
>>>>>> +    } else {
>>>>>> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
>>>>>> +                  "halt|powerdown|reboot");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = fork();
>>>>>> +    if (ret == 0) {
>>>>>> +        /* child, start the shutdown */
>>>>>> +        setsid();
>>>>>> +        fclose(stdin);
>>>>>> +        fclose(stdout);
>>>>>> +        fclose(stderr);
>>>>>> +
>>>>>> +        sleep(5);
>>>>>
>>>>> If we're required to return a response before the shutdown happens, this
>>>>> is a bug and I'm afraid that the right way to this is a bit complex.
>>>>>
>>>>> Otherwise we can just leave it out.
>>>>>
>>>>
>>>> Yah, I ran this by Anthony and Adam and just leaving it out seems to be
>>>> the preferred approach, for now at least.
>>>>
>>>>>> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
>>>>>> +                    "hypervisor initiated shutdown", (char*)NULL);
>>>>>> +        if (ret) {
>>>>>> +            slog("guest-shutdown failed: %s", strerror(errno));
>>>>>> +        }
>>>>>> +        exit(!!ret);
>>>>>> +    } else if (ret<    0) {
>>>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +typedef struct GuestFileHandle {
>>>>>> +    uint64_t id;
>>>>>> +    FILE *fh;
>>>>>> +    QTAILQ_ENTRY(GuestFileHandle) next;
>>>>>> +} GuestFileHandle;
>>>>>> +
>>>>>> +static struct {
>>>>>> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
>>>>>> +} guest_file_state;
>>>>>> +
>>>>>> +static void guest_file_handle_add(FILE *fh)
>>>>>> +{
>>>>>> +    GuestFileHandle *gfh;
>>>>>> +
>>>>>> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
>>>>>> +    gfh->id = fileno(fh);
>>>>>> +    gfh->fh = fh;
>>>>>> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
>>>>>> +}
>>>>>> +
>>>>>> +static GuestFileHandle *guest_file_handle_find(int64_t id)
>>>>>> +{
>>>>>> +    GuestFileHandle *gfh;
>>>>>> +
>>>>>> +    QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
>>>>>> +    {
>>>>>> +        if (gfh->id == id) {
>>>>>> +            return gfh;
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    return NULL;
>>>>>> +}
>>>>>> +
>>>>>> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
>>>>>> +{
>>>>>> +    FILE *fh;
>>>>>> +    int fd;
>>>>>> +    int64_t ret = -1;
>>>>>> +
>>>>>> +    if (!has_mode) {
>>>>>> +        mode = "r";
>>>>>> +    }
>>>>>> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
>>>>>> +    fh = fopen(filepath, mode);
>>>>>> +    if (!fh) {
>>>>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
>>>>>> +        return -1;
>>>>>> +    }
>>>>>> +
>>>>>> +    /* set fd non-blocking to avoid common use cases (like reading from a
>>>>>> +     * named pipe) from hanging the agent
>>>>>> +     */
>>>>>> +    fd = fileno(fh);
>>>>>> +    ret = fcntl(fd, F_GETFL);
>>>>>> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
>>>>>> +    if (ret == -1) {
>>>>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
>>>>>> +        fclose(fh);
>>>>>> +        return -1;
>>>>>> +    }
>>>>>> +
>>>>>> +    guest_file_handle_add(fh);
>>>>>> +    slog("guest-file-open, filehandle: %d", fd);
>>>>>> +    return fd;
>>>>>> +}
>>>>>> +
>>>>>> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
>>>>>> +                                          Error **err)
>>>>>> +{
>>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>>>> +    GuestFileRead *read_data;
>>>>>> +    guchar *buf;
>>>>>> +    FILE *fh;
>>>>>> +    size_t read_count;
>>>>>> +
>>>>>> +    if (!gfh) {
>>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (count<    0 || count>    QGA_READ_LIMIT) {
>>>>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>
>>>>> Are we imposing that limit because of the malloc() call below? If that's
>>>>> the case I think it's wrong, because we don't know the VM (neither the guest)
>>>>> better than the client.
>>>>>
>>>>> The best thing we can do here is to limit it to the file size. Additionally
>>>>> to this we could have a command-line option to allow the sysadmin set his/her
>>>>> own limit.
>>>>>
>>>>
>>>> That's technically the better approach, but we're also bound by the
>>>> maximum token size limit in the JSON parser, which is 64MB. Best we can
>>>> do is set QGA_READ_LIMIT accordingly.
>>>
>>> Good point, but I think it's ok to let the parser do this check itself, as
>>> control won't get here anyway. Maybe we should only document that the server
>>> imposes a limit on the token size.
>>>
>>
>>    From a usability perspective I think the QERR_INVALID_PARAMETER, or
>> perhaps something more descriptive, is a little nicer. Plus, they won't
>> have to wait for 64MB to get streamed back before they get the error :)
>
> That's why I suggested documenting it. Are we going to add this check
> to all commands that make it more likely to reach the parser's limit?
> (also note that theoretically all commands can do it).
>

Good point, and I agree that it needs to be documented either way. Might 
not hurt to further clarify the limitations this imposes on a command 
like guest-file-read in the schema documentation where appropriate though.

I'm not sure about the alternative suggestion of bounding this by 
filesize though, since they may also be writing to the same fd via 
guest-file-write, and the current file size wouldn't be meaningful 
except after an fflush() or fclose(). So we'd be forced to fflush() in 
guest-file-write to implement this somewhat reliably, but I think we 
agree that a separate guest-file-flush would be better.

So let's just let the client blow up their guest if they're so inclined.

>>
>>>>
>>>>>> +
>>>>>> +    fh = gfh->fh;
>>>>>> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
>>>>>> +    buf = qemu_mallocz(count+1);
>>>>>> +    if (!buf) {
>>>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>
>>>>> qemu_malloc() functions never fail...
>>>>>
>>>>>> +
>>>>>> +    read_count = fread(buf, 1, count, fh);
>>>>>
>>>>> Isn't 'nmemb' and 'size' swapped?
>>>>>
>>>>
>>>> I'm not sure...
>>>>
>>>> size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
>>>>
>>>> I wrote it as $count number of 1-bytes elements. This seems logical to
>>>> me, since it's a non-blocking FD which way result in a partial of some
>>>> lesser number of bytes than count.
>>>
>>> Ok. I think either way will work.
>>>
>>>>
>>>>>> +    buf[read_count] = 0;
>>>>>> +    read_data->count = read_count;
>>>>>> +    read_data->eof = feof(fh);
>>>>>> +    if (read_count) {
>>>>>> +        read_data->buf = g_base64_encode(buf, read_count);
>>>>>> +    }
>>>>>> +    qemu_free(buf);
>>>>>> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
>>>>>> +     * mode, and no real way to differenitate from a real error since we only
>>>>>> +     * get boolean error flag from ferror()
>>>>>> +     */
>>>>>> +    clearerr(fh);
>>>>>> +
>>>>>> +    return read_data;
>>>>>> +}
>>>>>> +
>>>>>> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
>>>>>> +                                     int64_t count, Error **err)
>>>>>> +{
>>>>>> +    GuestFileWrite *write_data;
>>>>>> +    guchar *data;
>>>>>> +    gsize data_len;
>>>>>> +    int write_count;
>>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>>>> +    FILE *fh;
>>>>>> +
>>>>>> +    if (!gfh) {
>>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +
>>>>>> +    fh = gfh->fh;
>>>>>> +    data = g_base64_decode(data_b64,&data_len);
>>>>>> +    if (count>    data_len) {
>>>>>> +        qemu_free(data);
>>>>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
>>>>>> +    write_count = fwrite(data, 1, count, fh);
>>>>>> +    write_data->count = write_count;
>>>>>> +    write_data->eof = feof(fh);
>>>>>> +    qemu_free(data);
>>>>>> +    clearerr(fh);
>>>>>
>>>>> Shouldn't we check for errors instead of doing this?
>>>>>
>>>>
>>>> Yah...unfortunately we only get a boolean flag with ferror() so it's not
>>>> all that useful, but I can add an error flag to the calls to capture it
>>>> at least. clearerr() is only being used here to clear the eof flag.
>>>
>>> I meant to check fwrite()'s return.
>>>
>>
>> Ahh, right. Definitely.
>>
>>>>
>>>>> Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
>>>>> here, but that's probably bad).
>>>>>
>>>>
>>>> Yah, I'd been planning on adding a guest-file-flush() for a while now.
>>>> I'll add that for the respin.
>>>>
>>>>>> +
>>>>>> +    return write_data;
>>>>>> +}
>>>>>> +
>>>>>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
>>>>>> +                                          int64_t whence, Error **err)
>>>>>> +{
>>>>>> +    GuestFileSeek *seek_data;
>>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>>>> +    FILE *fh;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!gfh) {
>>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +
>>>>>> +    fh = gfh->fh;
>>>>>> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
>>>>>> +    ret = fseek(fh, offset, whence);
>>>>>> +    if (ret == -1) {
>>>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
>>>>>> +        qemu_free(seek_data);
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +    seek_data->position = ftell(fh);
>>>>>> +    seek_data->eof = feof(fh);
>>>>>> +    clearerr(fh);
>>>>>
>>>>> Again, I don't see why we should do this.
>>>
>>> This is probably ok, as we're checking fseek() above.
>>>
>>>>>
>>>>>> +
>>>>>> +    return seek_data;
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_guest_file_close(int64_t filehandle, Error **err)
>>>>>> +{
>>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
>>>>>> +
>>>>>> +    slog("guest-file-close called, filehandle: %ld", filehandle);
>>>>>> +    if (!gfh) {
>>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    fclose(gfh->fh);
>>>>>> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
>>>>>> +    qemu_free(gfh);
>>>>>> +}
>>>>>> +
>>>>>> +static void guest_file_init(void)
>>>>>> +{
>>>>>> +    QTAILQ_INIT(&guest_file_state.filehandles);
>>>>>> +}
>>>>>> +
>>>>>> +typedef struct GuestFsfreezeMount {
>>>>>> +    char *dirname;
>>>>>> +    char *devtype;
>>>>>> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
>>>>>> +} GuestFsfreezeMount;
>>>>>> +
>>>>>> +struct {
>>>>>> +    GuestFsfreezeStatus status;
>>>>>> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
>>>>>> +} guest_fsfreeze_state;
>>>>>> +
>>>>>> +/*
>>>>>> + * Walk the mount table and build a list of local file systems
>>>>>> + */
>>>>>> +static int guest_fsfreeze_build_mount_list(void)
>>>>>> +{
>>>>>> +    struct mntent *ment;
>>>>>> +    GuestFsfreezeMount *mount, *temp;
>>>>>> +    char const *mtab = MOUNTED;
>>>>>> +    FILE *fp;
>>>>>> +
>>>>>> +    fp = setmntent(mtab, "r");
>>>>>> +    if (!fp) {
>>>>>> +        g_warning("fsfreeze: unable to read mtab");
>>>>>> +        goto fail;
>>>>>> +    }
>>>>>> +
>>>>>> +    while ((ment = getmntent(fp))) {
>>>>>> +        /*
>>>>>> +         * An entry which device name doesn't start with a '/' is
>>>>>> +         * either a dummy file system or a network file system.
>>>>>> +         * Add special handling for smbfs and cifs as is done by
>>>>>> +         * coreutils as well.
>>>>>> +         */
>>>>>> +        if ((ment->mnt_fsname[0] != '/') ||
>>>>>> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
>>>>>> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
>>>>>> +            continue;
>>>>>> +        }
>>>>>> +
>>>>>> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
>>>>>> +        mount->dirname = qemu_strdup(ment->mnt_dir);
>>>>>> +        mount->devtype = qemu_strdup(ment->mnt_type);
>>>>>> +
>>>>>> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
>>>>>> +    }
>>>>>> +
>>>>>> +    endmntent(fp);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +
>>>>>> +fail:
>>>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>>>>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
>>>>>> +        qemu_free(mount->dirname);
>>>>>> +        qemu_free(mount->devtype);
>>>>>> +        qemu_free(mount);
>>>>>> +    }
>>>>>
>>>>> This doesn't seem to be used.
>>>>>
>>>>
>>>> It can get used even a 2nd invocation of this function gets called that
>>>> results in a goto fail. But looking again this should be done
>>>> unconditionally at the start of the function, since the mount list is
>>>> part of global state now.
>>>>
>>>>>> +
>>>>>> +    return -1;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Return status of freeze/thaw
>>>>>> + */
>>>>>> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
>>>>>> +{
>>>>>> +    return guest_fsfreeze_state.status;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Walk list of mounted file systems in the guest, and freeze the ones which
>>>>>> + * are real local file systems.
>>>>>> + */
>>>>>> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
>>>>>> +{
>>>>>> +    int ret = 0, i = 0;
>>>>>> +    struct GuestFsfreezeMount *mount, *temp;
>>>>>> +    int fd;
>>>>>> +
>>>>>> +    slog("guest-fsfreeze called");
>>>>>> +
>>>>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
>>>>>
>>>>> return 0;
>>>>>
>>>>>> +        ret = 0;
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = guest_fsfreeze_build_mount_list();
>>>>>> +    if (ret<    0) {
>>>>>
>>>>> return ret;
>>>>>
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> +
>>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
>>>>>> +
>>>>>> +    /* cannot risk guest agent blocking itself on a write in this state */
>>>>>> +    disable_logging();
>>>>>> +
>>>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>>>>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>>>>>> +        if (fd == -1) {
>>>>>> +            ret = errno;
>>>>>> +            goto error;
>>>>>> +        }
>>>>>> +
>>>>>> +        /* we try to cull filesytems we know won't work in advance, but other
>>>>>> +         * filesytems may not implement fsfreeze for less obvious reasons.
>>>>>> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
>>>>>> +         * thawing, these filesystems will return an EINVAL instead, due to
>>>>>> +         * not being in a frozen state. Other filesystem-specific
>>>>>> +         * errors may result in EINVAL, however, so the user should check the
>>>>>> +         * number * of filesystems returned here against those returned by the
>>>>>> +         * thaw operation to determine whether everything completed
>>>>>> +         * successfully
>>>>>> +         */
>>>>>> +        ret = ioctl(fd, FIFREEZE);
>>>>>> +        if (ret<    0&&    errno != EOPNOTSUPP) {
>>>>>> +            close(fd);
>>>>>> +            goto error;
>>>>>> +        }
>>>>>> +        close(fd);
>>>>>> +
>>>>>> +        i++;
>>>>>> +    }
>>>>>> +
>>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
>>>>>> +    ret = i;
>>>>>> +out:
>>>>>> +    return ret;
>>>>>> +error:
>>>>>> +    if (i>    0) {
>>>>>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
>>>>>> +    }
>>>>>
>>>>> Shouldn't you undo everything that has been done so far? Which is
>>>>> freeing the build list and thawing the file-systems that were frozen
>>>>> before the error?
>>>>>
>>>>
>>>> We can...the way it's done right now is you get a count of how many
>>>> filesystems were frozen, along an error status. Depending on the
>>>> error/count the user can either ignore the error, do what it is they
>>>> want to do, then call thaw(), or just immediately call thaw().
>>>
>>> But you don't get the count on error, so it's difficult (if possible) to
>>> learn how many file-systems were frozen.
>>>
>>
>> Yah, my mistake. I think the out: label was one line higher, but if we
>> did that we lose error information. Automatically unfreezing should be okay.
>>
>>>>
>>>> So we can do an automatic thaw() on their behalf, but i figured the
>>>> status was good enough.
>>>>
>>>>>> +    goto out;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Walk list of frozen file systems in the guest, and thaw them.
>>>>>> + */
>>>>>> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    GuestFsfreezeMount *mount, *temp;
>>>>>> +    int fd, i = 0;
>>>>>> +
>>>>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
>>>>>> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
>>>>>
>>>>> I don't follow why we're checking against INPROGRESS here.
>>>>>
>>>>
>>>> To prevent a race I believe...but we're synchronous now so that's
>>>> probably no longer needed. I'll look it over and remove it if that's the
>>>> case.
>>>>
>>>>>> +        ret = 0;
>>>>>> +        goto out_enable_logging;
>>>>>> +    }
>>>>>> +
>>>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
>>>>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
>>>>>> +        if (fd == -1) {
>>>>>> +            ret = -errno;
>>>>>> +            goto out;
>>>>>> +        }
>>>>>> +        ret = ioctl(fd, FITHAW);
>>>>>> +        if (ret<    0&&    errno != EOPNOTSUPP&&    errno != EINVAL) {
>>>>>> +            ret = -errno;
>>>>>> +            close(fd);
>>>>>> +            goto out;
>>>>>
>>>>> Shouldn't you continue and try to thaw the other file-systems in the list?
>>>>>
>>>>
>>>> That's probably better
>>>>
>>>>>> +        }
>>>>>> +        close(fd);
>>>>>> +
>>>>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
>>>>>> +        qemu_free(mount->dirname);
>>>>>> +        qemu_free(mount->devtype);
>>>>>> +        qemu_free(mount);
>>>>>> +        i++;
>>>>>> +    }
>>>>>> +
>>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>>>>>> +    ret = i;
>>>>>> +out_enable_logging:
>>>>>> +    enable_logging();
>>>>>> +out:
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void guest_fsfreeze_init(void)
>>>>>> +{
>>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
>>>>>> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
>>>>>> +}
>>>>>> +
>>>>>> +static void guest_fsfreeze_cleanup(void)
>>>>>> +{
>>>>>> +    int64_t ret;
>>>>>> +    Error *err = NULL;
>>>>>> +
>>>>>> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
>>>>>> +        ret = qmp_guest_fsfreeze_thaw(&err);
>>>>>> +        if (ret<    0 || err) {
>>>>>> +            slog("failed to clean up frozen filesystems");
>>>>>> +        }
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +/* register init/cleanup routines for stateful command groups */
>>>>>> +void ga_command_state_init(GAState *s, GACommandState *cs)
>>>>>> +{
>>>>>> +    ga_state = s;
>>>>>> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
>>>>>> +    ga_command_state_add(cs, guest_file_init, NULL);
>>>>>> +}
>>>>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>>>>>> index 66d1729..3501ff4 100644
>>>>>> --- a/qga/guest-agent-core.h
>>>>>> +++ b/qga/guest-agent-core.h
>>>>>> @@ -14,10 +14,12 @@
>>>>>>     #include "qemu-common.h"
>>>>>>
>>>>>>     #define QGA_VERSION "1.0"
>>>>>> +#define QGA_READ_LIMIT 4<<    20 /* 4MB block size max for chunked reads */
>>>>>>
>>>>>>     typedef struct GAState GAState;
>>>>>>     typedef struct GACommandState GACommandState;
>>>>>>
>>>>>> +void ga_command_state_init(GAState *s, GACommandState *cs);
>>>>>>     void ga_command_state_add(GACommandState *cs,
>>>>>>                               void (*init)(void),
>>>>>>                               void (*cleanup)(void));
>>>>>
>>>>
>>>
>>
>

On Tue, 12 Jul 2011 10:44:14 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 07/12/2011 09:15 AM, Luiz Capitulino wrote:
> > On Mon, 11 Jul 2011 18:11:21 -0500
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
> >>> On Mon, 11 Jul 2011 15:11:26 -0500
> >>> Michael Roth<mdroth@linux.vnet.ibm.com>   wrote:
> >>>
> >>>> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> >>>>> On Tue,  5 Jul 2011 08:21:40 -0500
> >>>>> Michael Roth<mdroth@linux.vnet.ibm.com>    wrote:
> >>>>>
> >>>>>> This adds the initial set of QMP/QAPI commands provided by the guest
> >>>>>> agent:
> >>>>>>
> >>>>>> guest-sync
> >>>>>> guest-ping
> >>>>>> guest-info
> >>>>>> guest-shutdown
> >>>>>> guest-file-open
> >>>>>> guest-file-read
> >>>>>> guest-file-write
> >>>>>> guest-file-seek
> >>>>>> guest-file-close
> >>>>>> guest-fsfreeze-freeze
> >>>>>> guest-fsfreeze-thaw
> >>>>>> guest-fsfreeze-status
> >>>>>>
> >>>>>> The input/output specification for these commands are documented in the
> >>>>>> schema.
> >>>>>>
> >>>>>> Example usage:
> >>>>>>
> >>>>>>      host:
> >>>>>>        qemu -device virtio-serial \
> >>>>>>             -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
> >>>>>>             -device virtserialport,chardev=qga0,name=qga0
> >>>>>>             ...
> >>>>>>
> >>>>>>        echo "{'execute':'guest-info'}" | socat stdio \
> >>>>>>             unix-connect:/tmp/qga0.sock
> >>>>>>
> >>>>>>      guest:
> >>>>>>        qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
> >>>>>>                -p /var/run/qemu-guest-agent.pid -d
> >>>>>>
> >>>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>>     Makefile                   |   15 ++-
> >>>>>>     qemu-ga.c                  |    4 +
> >>>>>>     qerror.h                   |    3 +
> >>>>>>     qga/guest-agent-commands.c |  501 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>     qga/guest-agent-core.h     |    2 +
> >>>>>>     5 files changed, 523 insertions(+), 2 deletions(-)
> >>>>>>     create mode 100644 qga/guest-agent-commands.c
> >>>>>>
> >>>>>> diff --git a/Makefile b/Makefile
> >>>>>> index b2e8593..7e4f722 100644
> >>>>>> --- a/Makefile
> >>>>>> +++ b/Makefile
> >>>>>> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h: $(qapi-dir)/test-qmp-marshal.c
> >>>>>>     $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-commands.py
> >>>>>>     	    $(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<    $<, "  GEN   $@")
> >>>>>>
> >>>>>> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
> >>>>>> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-types.py
> >>>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py -o "$(qapi-dir)" -p "qga-"<    $<, "  GEN   $@")
> >>>>>> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> >>>>>> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-visit.py
> >>>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py -o "$(qapi-dir)" -p "qga-"<    $<, "  GEN   $@")
> >>>>>> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json $(SRC_PATH)/scripts/qapi-commands.py
> >>>>>> +	$(call quiet-command,python $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<    $<, "  GEN   $@")
> >>>>>> +
> >>>>>>     test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
> >>>>>>     test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o
> >>>>>>
> >>>>>>     test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c test-qapi-types.h test-qapi-visit.c test-qapi-visit.h test-qmp-marshal.c test-qmp-commands.h)
> >>>>>>     test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
> >>>>>>
> >>>>>> -QGALIB=qga/guest-agent-command-state.o
> >>>>>> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
> >>>>>> +
> >>>>>> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
> >>>>>>
> >>>>>> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
> >>>>>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o module.o qapi/qmp-dispatch.o qapi/qmp-registry.o $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
> >>>>>>
> >>>>>>     QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
> >>>>>>
> >>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>>>> index 649c16a..04ead22 100644
> >>>>>> --- a/qemu-ga.c
> >>>>>> +++ b/qemu-ga.c
> >>>>>> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
> >>>>>>         g_log_set_default_handler(ga_log, s);
> >>>>>>         g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> >>>>>>         s->logging_enabled = true;
> >>>>>> +    s->command_state = ga_command_state_new();
> >>>>>> +    ga_command_state_init(s, s->command_state);
> >>>>>> +    ga_command_state_init_all(s->command_state);
> >>>>>>         ga_state = s;
> >>>>>>
> >>>>>>         module_call_init(MODULE_INIT_QAPI);
> >>>>>> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
> >>>>>>
> >>>>>>         g_main_loop_run(ga_state->main_loop);
> >>>>>>
> >>>>>> +    ga_command_state_cleanup_all(ga_state->command_state);
> >>>>>>         unlink(pidfile);
> >>>>>>
> >>>>>>         return 0;
> >>>>>> diff --git a/qerror.h b/qerror.h
> >>>>>> index 9a9fa5b..0f618ac 100644
> >>>>>> --- a/qerror.h
> >>>>>> +++ b/qerror.h
> >>>>>> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
> >>>>>>     #define QERR_FEATURE_DISABLED \
> >>>>>>         "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
> >>>>>>
> >>>>>> +#define QERR_QGA_LOGGING_FAILED \
> >>>>>> +    "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> >>>>>> +
> >>>>>>     #endif /* QERROR_H */
> >>>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >>>>>> new file mode 100644
> >>>>>> index 0000000..42390fb
> >>>>>> --- /dev/null
> >>>>>> +++ b/qga/guest-agent-commands.c
> >>>>>> @@ -0,0 +1,501 @@
> >>>>>> +/*
> >>>>>> + * QEMU Guest Agent commands
> >>>>>> + *
> >>>>>> + * Copyright IBM Corp. 2011
> >>>>>> + *
> >>>>>> + * Authors:
> >>>>>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
> >>>>>> + *
> >>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >>>>>> + * See the COPYING file in the top-level directory.
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include<glib.h>
> >>>>>> +#include<mntent.h>
> >>>>>> +#include<sys/types.h>
> >>>>>> +#include<sys/ioctl.h>
> >>>>>> +#include<linux/fs.h>
> >>>>>> +#include "qga/guest-agent-core.h"
> >>>>>> +#include "qga-qmp-commands.h"
> >>>>>> +#include "qerror.h"
> >>>>>> +#include "qemu-queue.h"
> >>>>>> +
> >>>>>> +static GAState *ga_state;
> >>>>>> +
> >>>>>> +static void disable_logging(void)
> >>>>>> +{
> >>>>>> +    ga_disable_logging(ga_state);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void enable_logging(void)
> >>>>>> +{
> >>>>>> +    ga_enable_logging(ga_state);
> >>>>>> +}
> >>>>>> +
> >>>>>> +/* Note: in some situations, like with the fsfreeze, logging may be
> >>>>>> + * temporarilly disabled. if it is necessary that a command be able
> >>>>>> + * to log for accounting purposes, check ga_logging_enabled() beforehand,
> >>>>>> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
> >>>>>> + */
> >>>>>> +static void slog(const char *fmt, ...)
> >>>>>> +{
> >>>>>> +    va_list ap;
> >>>>>> +
> >>>>>> +    va_start(ap, fmt);
> >>>>>> +    g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
> >>>>>> +    va_end(ap);
> >>>>>> +}
> >>>>>> +
> >>>>>> +int64_t qmp_guest_sync(int64_t id, Error **errp)
> >>>>>> +{
> >>>>>> +    return id;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qmp_guest_ping(Error **err)
> >>>>>> +{
> >>>>>> +    slog("guest-ping called");
> >>>>>> +}
> >>>>>> +
> >>>>>> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> >>>>>> +{
> >>>>>> +    GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
> >>>>>> +
> >>>>>> +    info->version = g_strdup(QGA_VERSION);
> >>>>>> +
> >>>>>> +    return info;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qmp_guest_shutdown(const char *mode, Error **err)
> >>>>>> +{
> >>>>>> +    int ret;
> >>>>>> +    const char *shutdown_flag;
> >>>>>> +
> >>>>>> +    slog("guest-shutdown called, mode: %s", mode);
> >>>>>> +    if (strcmp(mode, "halt") == 0) {
> >>>>>> +        shutdown_flag = "-H";
> >>>>>> +    } else if (strcmp(mode, "powerdown") == 0) {
> >>>>>> +        shutdown_flag = "-P";
> >>>>>> +    } else if (strcmp(mode, "reboot") == 0) {
> >>>>>> +        shutdown_flag = "-r";
> >>>>>> +    } else {
> >>>>>> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
> >>>>>> +                  "halt|powerdown|reboot");
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    ret = fork();
> >>>>>> +    if (ret == 0) {
> >>>>>> +        /* child, start the shutdown */
> >>>>>> +        setsid();
> >>>>>> +        fclose(stdin);
> >>>>>> +        fclose(stdout);
> >>>>>> +        fclose(stderr);
> >>>>>> +
> >>>>>> +        sleep(5);
> >>>>>
> >>>>> If we're required to return a response before the shutdown happens, this
> >>>>> is a bug and I'm afraid that the right way to this is a bit complex.
> >>>>>
> >>>>> Otherwise we can just leave it out.
> >>>>>
> >>>>
> >>>> Yah, I ran this by Anthony and Adam and just leaving it out seems to be
> >>>> the preferred approach, for now at least.
> >>>>
> >>>>>> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> >>>>>> +                    "hypervisor initiated shutdown", (char*)NULL);
> >>>>>> +        if (ret) {
> >>>>>> +            slog("guest-shutdown failed: %s", strerror(errno));
> >>>>>> +        }
> >>>>>> +        exit(!!ret);
> >>>>>> +    } else if (ret<    0) {
> >>>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +typedef struct GuestFileHandle {
> >>>>>> +    uint64_t id;
> >>>>>> +    FILE *fh;
> >>>>>> +    QTAILQ_ENTRY(GuestFileHandle) next;
> >>>>>> +} GuestFileHandle;
> >>>>>> +
> >>>>>> +static struct {
> >>>>>> +    QTAILQ_HEAD(, GuestFileHandle) filehandles;
> >>>>>> +} guest_file_state;
> >>>>>> +
> >>>>>> +static void guest_file_handle_add(FILE *fh)
> >>>>>> +{
> >>>>>> +    GuestFileHandle *gfh;
> >>>>>> +
> >>>>>> +    gfh = qemu_mallocz(sizeof(GuestFileHandle));
> >>>>>> +    gfh->id = fileno(fh);
> >>>>>> +    gfh->fh = fh;
> >>>>>> +    QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static GuestFileHandle *guest_file_handle_find(int64_t id)
> >>>>>> +{
> >>>>>> +    GuestFileHandle *gfh;
> >>>>>> +
> >>>>>> +    QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
> >>>>>> +    {
> >>>>>> +        if (gfh->id == id) {
> >>>>>> +            return gfh;
> >>>>>> +        }
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    return NULL;
> >>>>>> +}
> >>>>>> +
> >>>>>> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode, const char *mode, Error **err)
> >>>>>> +{
> >>>>>> +    FILE *fh;
> >>>>>> +    int fd;
> >>>>>> +    int64_t ret = -1;
> >>>>>> +
> >>>>>> +    if (!has_mode) {
> >>>>>> +        mode = "r";
> >>>>>> +    }
> >>>>>> +    slog("guest-file-open called, filepath: %s, mode: %s", filepath, mode);
> >>>>>> +    fh = fopen(filepath, mode);
> >>>>>> +    if (!fh) {
> >>>>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >>>>>> +        return -1;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    /* set fd non-blocking to avoid common use cases (like reading from a
> >>>>>> +     * named pipe) from hanging the agent
> >>>>>> +     */
> >>>>>> +    fd = fileno(fh);
> >>>>>> +    ret = fcntl(fd, F_GETFL);
> >>>>>> +    ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
> >>>>>> +    if (ret == -1) {
> >>>>>> +        error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >>>>>> +        fclose(fh);
> >>>>>> +        return -1;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    guest_file_handle_add(fh);
> >>>>>> +    slog("guest-file-open, filehandle: %d", fd);
> >>>>>> +    return fd;
> >>>>>> +}
> >>>>>> +
> >>>>>> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t count,
> >>>>>> +                                          Error **err)
> >>>>>> +{
> >>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> +    GuestFileRead *read_data;
> >>>>>> +    guchar *buf;
> >>>>>> +    FILE *fh;
> >>>>>> +    size_t read_count;
> >>>>>> +
> >>>>>> +    if (!gfh) {
> >>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    if (count<    0 || count>    QGA_READ_LIMIT) {
> >>>>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>
> >>>>> Are we imposing that limit because of the malloc() call below? If that's
> >>>>> the case I think it's wrong, because we don't know the VM (neither the guest)
> >>>>> better than the client.
> >>>>>
> >>>>> The best thing we can do here is to limit it to the file size. Additionally
> >>>>> to this we could have a command-line option to allow the sysadmin set his/her
> >>>>> own limit.
> >>>>>
> >>>>
> >>>> That's technically the better approach, but we're also bound by the
> >>>> maximum token size limit in the JSON parser, which is 64MB. Best we can
> >>>> do is set QGA_READ_LIMIT accordingly.
> >>>
> >>> Good point, but I think it's ok to let the parser do this check itself, as
> >>> control won't get here anyway. Maybe we should only document that the server
> >>> imposes a limit on the token size.
> >>>
> >>
> >>    From a usability perspective I think the QERR_INVALID_PARAMETER, or
> >> perhaps something more descriptive, is a little nicer. Plus, they won't
> >> have to wait for 64MB to get streamed back before they get the error :)
> >
> > That's why I suggested documenting it. Are we going to add this check
> > to all commands that make it more likely to reach the parser's limit?
> > (also note that theoretically all commands can do it).
> >
> 
> Good point, and I agree that it needs to be documented either way. Might 
> not hurt to further clarify the limitations this imposes on a command 
> like guest-file-read in the schema documentation where appropriate though.
> 
> I'm not sure about the alternative suggestion of bounding this by 
> filesize though, since they may also be writing to the same fd via 
> guest-file-write, and the current file size wouldn't be meaningful 
> except after an fflush() or fclose().

This is just like the OS, the application has to what's doing.

> So we'd be forced to fflush() in 
> guest-file-write to implement this somewhat reliably, but I think we 
> agree that a separate guest-file-flush would be better.

Yes.

> So let's just let the client blow up their guest if they're so inclined.

Exactly :) I mean, if something like this turns out to be a problem, we'd
have to fix it differently IMO.

> 
> >>
> >>>>
> >>>>>> +
> >>>>>> +    fh = gfh->fh;
> >>>>>> +    read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
> >>>>>> +    buf = qemu_mallocz(count+1);
> >>>>>> +    if (!buf) {
> >>>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>
> >>>>> qemu_malloc() functions never fail...
> >>>>>
> >>>>>> +
> >>>>>> +    read_count = fread(buf, 1, count, fh);
> >>>>>
> >>>>> Isn't 'nmemb' and 'size' swapped?
> >>>>>
> >>>>
> >>>> I'm not sure...
> >>>>
> >>>> size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
> >>>>
> >>>> I wrote it as $count number of 1-bytes elements. This seems logical to
> >>>> me, since it's a non-blocking FD which way result in a partial of some
> >>>> lesser number of bytes than count.
> >>>
> >>> Ok. I think either way will work.
> >>>
> >>>>
> >>>>>> +    buf[read_count] = 0;
> >>>>>> +    read_data->count = read_count;
> >>>>>> +    read_data->eof = feof(fh);
> >>>>>> +    if (read_count) {
> >>>>>> +        read_data->buf = g_base64_encode(buf, read_count);
> >>>>>> +    }
> >>>>>> +    qemu_free(buf);
> >>>>>> +    /* clear error and eof. error is generally due to EAGAIN from non-blocking
> >>>>>> +     * mode, and no real way to differenitate from a real error since we only
> >>>>>> +     * get boolean error flag from ferror()
> >>>>>> +     */
> >>>>>> +    clearerr(fh);
> >>>>>> +
> >>>>>> +    return read_data;
> >>>>>> +}
> >>>>>> +
> >>>>>> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char *data_b64,
> >>>>>> +                                     int64_t count, Error **err)
> >>>>>> +{
> >>>>>> +    GuestFileWrite *write_data;
> >>>>>> +    guchar *data;
> >>>>>> +    gsize data_len;
> >>>>>> +    int write_count;
> >>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> +    FILE *fh;
> >>>>>> +
> >>>>>> +    if (!gfh) {
> >>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    fh = gfh->fh;
> >>>>>> +    data = g_base64_decode(data_b64,&data_len);
> >>>>>> +    if (count>    data_len) {
> >>>>>> +        qemu_free(data);
> >>>>>> +        error_set(err, QERR_INVALID_PARAMETER, "count");
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>> +    write_data = qemu_mallocz(sizeof(GuestFileWrite));
> >>>>>> +    write_count = fwrite(data, 1, count, fh);
> >>>>>> +    write_data->count = write_count;
> >>>>>> +    write_data->eof = feof(fh);
> >>>>>> +    qemu_free(data);
> >>>>>> +    clearerr(fh);
> >>>>>
> >>>>> Shouldn't we check for errors instead of doing this?
> >>>>>
> >>>>
> >>>> Yah...unfortunately we only get a boolean flag with ferror() so it's not
> >>>> all that useful, but I can add an error flag to the calls to capture it
> >>>> at least. clearerr() is only being used here to clear the eof flag.
> >>>
> >>> I meant to check fwrite()'s return.
> >>>
> >>
> >> Ahh, right. Definitely.
> >>
> >>>>
> >>>>> Btw, I think it's a good idea to offer guest-file-flush() too (or do a flush()
> >>>>> here, but that's probably bad).
> >>>>>
> >>>>
> >>>> Yah, I'd been planning on adding a guest-file-flush() for a while now.
> >>>> I'll add that for the respin.
> >>>>
> >>>>>> +
> >>>>>> +    return write_data;
> >>>>>> +}
> >>>>>> +
> >>>>>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t offset,
> >>>>>> +                                          int64_t whence, Error **err)
> >>>>>> +{
> >>>>>> +    GuestFileSeek *seek_data;
> >>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> +    FILE *fh;
> >>>>>> +    int ret;
> >>>>>> +
> >>>>>> +    if (!gfh) {
> >>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    fh = gfh->fh;
> >>>>>> +    seek_data = qemu_mallocz(sizeof(GuestFileRead));
> >>>>>> +    ret = fseek(fh, offset, whence);
> >>>>>> +    if (ret == -1) {
> >>>>>> +        error_set(err, QERR_UNDEFINED_ERROR);
> >>>>>> +        qemu_free(seek_data);
> >>>>>> +        return NULL;
> >>>>>> +    }
> >>>>>> +    seek_data->position = ftell(fh);
> >>>>>> +    seek_data->eof = feof(fh);
> >>>>>> +    clearerr(fh);
> >>>>>
> >>>>> Again, I don't see why we should do this.
> >>>
> >>> This is probably ok, as we're checking fseek() above.
> >>>
> >>>>>
> >>>>>> +
> >>>>>> +    return seek_data;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qmp_guest_file_close(int64_t filehandle, Error **err)
> >>>>>> +{
> >>>>>> +    GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> +
> >>>>>> +    slog("guest-file-close called, filehandle: %ld", filehandle);
> >>>>>> +    if (!gfh) {
> >>>>>> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    fclose(gfh->fh);
> >>>>>> +    QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> >>>>>> +    qemu_free(gfh);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void guest_file_init(void)
> >>>>>> +{
> >>>>>> +    QTAILQ_INIT(&guest_file_state.filehandles);
> >>>>>> +}
> >>>>>> +
> >>>>>> +typedef struct GuestFsfreezeMount {
> >>>>>> +    char *dirname;
> >>>>>> +    char *devtype;
> >>>>>> +    QTAILQ_ENTRY(GuestFsfreezeMount) next;
> >>>>>> +} GuestFsfreezeMount;
> >>>>>> +
> >>>>>> +struct {
> >>>>>> +    GuestFsfreezeStatus status;
> >>>>>> +    QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
> >>>>>> +} guest_fsfreeze_state;
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Walk the mount table and build a list of local file systems
> >>>>>> + */
> >>>>>> +static int guest_fsfreeze_build_mount_list(void)
> >>>>>> +{
> >>>>>> +    struct mntent *ment;
> >>>>>> +    GuestFsfreezeMount *mount, *temp;
> >>>>>> +    char const *mtab = MOUNTED;
> >>>>>> +    FILE *fp;
> >>>>>> +
> >>>>>> +    fp = setmntent(mtab, "r");
> >>>>>> +    if (!fp) {
> >>>>>> +        g_warning("fsfreeze: unable to read mtab");
> >>>>>> +        goto fail;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    while ((ment = getmntent(fp))) {
> >>>>>> +        /*
> >>>>>> +         * An entry which device name doesn't start with a '/' is
> >>>>>> +         * either a dummy file system or a network file system.
> >>>>>> +         * Add special handling for smbfs and cifs as is done by
> >>>>>> +         * coreutils as well.
> >>>>>> +         */
> >>>>>> +        if ((ment->mnt_fsname[0] != '/') ||
> >>>>>> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
> >>>>>> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
> >>>>>> +            continue;
> >>>>>> +        }
> >>>>>> +
> >>>>>> +        mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
> >>>>>> +        mount->dirname = qemu_strdup(ment->mnt_dir);
> >>>>>> +        mount->devtype = qemu_strdup(ment->mnt_type);
> >>>>>> +
> >>>>>> +        QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount, next);
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    endmntent(fp);
> >>>>>> +
> >>>>>> +    return 0;
> >>>>>> +
> >>>>>> +fail:
> >>>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >>>>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >>>>>> +        qemu_free(mount->dirname);
> >>>>>> +        qemu_free(mount->devtype);
> >>>>>> +        qemu_free(mount);
> >>>>>> +    }
> >>>>>
> >>>>> This doesn't seem to be used.
> >>>>>
> >>>>
> >>>> It can get used even a 2nd invocation of this function gets called that
> >>>> results in a goto fail. But looking again this should be done
> >>>> unconditionally at the start of the function, since the mount list is
> >>>> part of global state now.
> >>>>
> >>>>>> +
> >>>>>> +    return -1;
> >>>>>> +}
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Return status of freeze/thaw
> >>>>>> + */
> >>>>>> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> >>>>>> +{
> >>>>>> +    return guest_fsfreeze_state.status;
> >>>>>> +}
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Walk list of mounted file systems in the guest, and freeze the ones which
> >>>>>> + * are real local file systems.
> >>>>>> + */
> >>>>>> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
> >>>>>> +{
> >>>>>> +    int ret = 0, i = 0;
> >>>>>> +    struct GuestFsfreezeMount *mount, *temp;
> >>>>>> +    int fd;
> >>>>>> +
> >>>>>> +    slog("guest-fsfreeze called");
> >>>>>> +
> >>>>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
> >>>>>
> >>>>> return 0;
> >>>>>
> >>>>>> +        ret = 0;
> >>>>>> +        goto out;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    ret = guest_fsfreeze_build_mount_list();
> >>>>>> +    if (ret<    0) {
> >>>>>
> >>>>> return ret;
> >>>>>
> >>>>>> +        goto out;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
> >>>>>> +
> >>>>>> +    /* cannot risk guest agent blocking itself on a write in this state */
> >>>>>> +    disable_logging();
> >>>>>> +
> >>>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >>>>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >>>>>> +        if (fd == -1) {
> >>>>>> +            ret = errno;
> >>>>>> +            goto error;
> >>>>>> +        }
> >>>>>> +
> >>>>>> +        /* we try to cull filesytems we know won't work in advance, but other
> >>>>>> +         * filesytems may not implement fsfreeze for less obvious reasons.
> >>>>>> +         * these will reason EOPNOTSUPP, so we simply ignore them. when
> >>>>>> +         * thawing, these filesystems will return an EINVAL instead, due to
> >>>>>> +         * not being in a frozen state. Other filesystem-specific
> >>>>>> +         * errors may result in EINVAL, however, so the user should check the
> >>>>>> +         * number * of filesystems returned here against those returned by the
> >>>>>> +         * thaw operation to determine whether everything completed
> >>>>>> +         * successfully
> >>>>>> +         */
> >>>>>> +        ret = ioctl(fd, FIFREEZE);
> >>>>>> +        if (ret<    0&&    errno != EOPNOTSUPP) {
> >>>>>> +            close(fd);
> >>>>>> +            goto error;
> >>>>>> +        }
> >>>>>> +        close(fd);
> >>>>>> +
> >>>>>> +        i++;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> >>>>>> +    ret = i;
> >>>>>> +out:
> >>>>>> +    return ret;
> >>>>>> +error:
> >>>>>> +    if (i>    0) {
> >>>>>> +        guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> >>>>>> +    }
> >>>>>
> >>>>> Shouldn't you undo everything that has been done so far? Which is
> >>>>> freeing the build list and thawing the file-systems that were frozen
> >>>>> before the error?
> >>>>>
> >>>>
> >>>> We can...the way it's done right now is you get a count of how many
> >>>> filesystems were frozen, along an error status. Depending on the
> >>>> error/count the user can either ignore the error, do what it is they
> >>>> want to do, then call thaw(), or just immediately call thaw().
> >>>
> >>> But you don't get the count on error, so it's difficult (if possible) to
> >>> learn how many file-systems were frozen.
> >>>
> >>
> >> Yah, my mistake. I think the out: label was one line higher, but if we
> >> did that we lose error information. Automatically unfreezing should be okay.
> >>
> >>>>
> >>>> So we can do an automatic thaw() on their behalf, but i figured the
> >>>> status was good enough.
> >>>>
> >>>>>> +    goto out;
> >>>>>> +}
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Walk list of frozen file systems in the guest, and thaw them.
> >>>>>> + */
> >>>>>> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >>>>>> +{
> >>>>>> +    int ret;
> >>>>>> +    GuestFsfreezeMount *mount, *temp;
> >>>>>> +    int fd, i = 0;
> >>>>>> +
> >>>>>> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
> >>>>>> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
> >>>>>
> >>>>> I don't follow why we're checking against INPROGRESS here.
> >>>>>
> >>>>
> >>>> To prevent a race I believe...but we're synchronous now so that's
> >>>> probably no longer needed. I'll look it over and remove it if that's the
> >>>> case.
> >>>>
> >>>>>> +        ret = 0;
> >>>>>> +        goto out_enable_logging;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next, temp) {
> >>>>>> +        fd = qemu_open(mount->dirname, O_RDONLY);
> >>>>>> +        if (fd == -1) {
> >>>>>> +            ret = -errno;
> >>>>>> +            goto out;
> >>>>>> +        }
> >>>>>> +        ret = ioctl(fd, FITHAW);
> >>>>>> +        if (ret<    0&&    errno != EOPNOTSUPP&&    errno != EINVAL) {
> >>>>>> +            ret = -errno;
> >>>>>> +            close(fd);
> >>>>>> +            goto out;
> >>>>>
> >>>>> Shouldn't you continue and try to thaw the other file-systems in the list?
> >>>>>
> >>>>
> >>>> That's probably better
> >>>>
> >>>>>> +        }
> >>>>>> +        close(fd);
> >>>>>> +
> >>>>>> +        QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >>>>>> +        qemu_free(mount->dirname);
> >>>>>> +        qemu_free(mount->devtype);
> >>>>>> +        qemu_free(mount);
> >>>>>> +        i++;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >>>>>> +    ret = i;
> >>>>>> +out_enable_logging:
> >>>>>> +    enable_logging();
> >>>>>> +out:
> >>>>>> +    return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void guest_fsfreeze_init(void)
> >>>>>> +{
> >>>>>> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >>>>>> +    QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void guest_fsfreeze_cleanup(void)
> >>>>>> +{
> >>>>>> +    int64_t ret;
> >>>>>> +    Error *err = NULL;
> >>>>>> +
> >>>>>> +    if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> >>>>>> +        ret = qmp_guest_fsfreeze_thaw(&err);
> >>>>>> +        if (ret<    0 || err) {
> >>>>>> +            slog("failed to clean up frozen filesystems");
> >>>>>> +        }
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +/* register init/cleanup routines for stateful command groups */
> >>>>>> +void ga_command_state_init(GAState *s, GACommandState *cs)
> >>>>>> +{
> >>>>>> +    ga_state = s;
> >>>>>> +    ga_command_state_add(cs, guest_fsfreeze_init, guest_fsfreeze_cleanup);
> >>>>>> +    ga_command_state_add(cs, guest_file_init, NULL);
> >>>>>> +}
> >>>>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >>>>>> index 66d1729..3501ff4 100644
> >>>>>> --- a/qga/guest-agent-core.h
> >>>>>> +++ b/qga/guest-agent-core.h
> >>>>>> @@ -14,10 +14,12 @@
> >>>>>>     #include "qemu-common.h"
> >>>>>>
> >>>>>>     #define QGA_VERSION "1.0"
> >>>>>> +#define QGA_READ_LIMIT 4<<    20 /* 4MB block size max for chunked reads */
> >>>>>>
> >>>>>>     typedef struct GAState GAState;
> >>>>>>     typedef struct GACommandState GACommandState;
> >>>>>>
> >>>>>> +void ga_command_state_init(GAState *s, GACommandState *cs);
> >>>>>>     void ga_command_state_add(GACommandState *cs,
> >>>>>>                               void (*init)(void),
> >>>>>>                               void (*cleanup)(void));
> >>>>>
> >>>>
> >>>
> >>
> >
>