Re: [Qemu-devel] [PATCH v5 3/5] guest agent: add guest agent RPCs/commands

[Luiz Capitulino]

On Fri, 17 Jun 2011 15:19:56 -0500
Michael Roth <address@hidden> wrote:

> On 06/16/2011 01:52 PM, Luiz Capitulino wrote:
> > On Tue, 14 Jun 2011 15:06:23 -0500
> > Michael Roth<address@hidden>  wrote:
> >
> >> This adds the initial set of QMP/QAPI commands provided by the guest
> >> agent:
> >>
> >> guest-sync
> >> guest-ping
> >> guest-info
> >> guest-shutdown
> >> guest-file-open
> >> guest-file-read
> >> guest-file-write
> >> guest-file-seek
> >> guest-file-close
> >> guest-fsfreeze-freeze
> >> guest-fsfreeze-thaw
> >> guest-fsfreeze-status
> >>
> >> The input/output specification for these commands are documented in the
> >> schema.
> >>
> >> Signed-off-by: Michael Roth<address@hidden>
> >> ---
> >>   qerror.c                   |    4 +
> >>   qerror.h                   |    3 +
> >>   qga/guest-agent-commands.c |  522 
> >> ++++++++++++++++++++++++++++++++++++++++++++
> >>   qga/guest-agent-core.h     |    1 +
> >>   4 files changed, 530 insertions(+), 0 deletions(-)
> >>   create mode 100644 qga/guest-agent-commands.c
> >>
> >> diff --git a/qerror.c b/qerror.c
> >> index d7fcd93..24f0c48 100644
> >> --- a/qerror.c
> >> +++ b/qerror.c
> >> @@ -213,6 +213,10 @@ static const QErrorStringTable qerror_table[] = {
> >>           .error_fmt = QERR_VNC_SERVER_FAILED,
> >>           .desc      = "Could not start VNC server on %(target)",
> >>       },
> >> +    {
> >> +        .error_fmt = QERR_QGA_LOGGING_FAILED,
> >> +        .desc      = "failed to write log statement due to logging being 
> >> disabled",
> >> +    },
> >>       {}
> >>   };
> >>
> >> diff --git a/qerror.h b/qerror.h
> >> index 7a89a50..bf3d5a9 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..6f9886a
> >> --- /dev/null
> >> +++ b/qga/guest-agent-commands.c
> >> @@ -0,0 +1,522 @@
> >> +/*
> >> + * QEMU Guest Agent commands
> >> + *
> >> + * Copyright IBM Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Michael Roth<address@hidden>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or 
> >> later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include<glib.h>
> >> +#include<mntent.h>
> >> +#include<sys/types.h>
> >> +#include<sys/ioctl.h>
> >> +#include<linux/fs.h>
> >> +#include "qga/guest-agent-core.h"
> >> +#include "qga-qmp-commands.h"
> >> +#include "qerror.h"
> >> +
> >> +static GAState *ga_state;
> >> +
> >> +static bool logging_enabled(void)
> >> +{
> >> +    return ga_logging_enabled(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 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 = g_malloc0(sizeof(GuestAgentInfo));
> >> +
> >> +    info->version = g_strdup(QGA_VERSION);
> >> +
> >> +    return info;
> >> +}
> >> +
> >> +void qmp_guest_shutdown(const char *shutdown_mode, Error **err)
> >
> > I'd call the argument just 'mode'.
> >
> >> +{
> >> +    int ret;
> >> +    const char *shutdown_flag;
> >> +
> >> +    if (!logging_enabled()) {
> >> +        error_set(err, QERR_QGA_LOGGING_FAILED);
> >> +        return;
> >> +    }
> >> +
> >> +    slog("guest-shutdown called, shutdown_mode: %s", shutdown_mode);
> >> +    if (strcmp(shutdown_mode, "halt") == 0) {
> >> +        shutdown_flag = "-H";
> >> +    } else if (strcmp(shutdown_mode, "powerdown") == 0) {
> >> +        shutdown_flag = "-P";
> >> +    } else if (strcmp(shutdown_mode, "reboot") == 0) {
> >> +        shutdown_flag = "-r";
> >> +    } else {
> >> +        error_set(err, QERR_INVALID_PARAMETER_VALUE, "shutdown_mode",
> >> +                  "halt|powerdown|reboot");
> >> +        return;
> >> +    }
> >> +
> >> +    ret = fork();
> >> +    if (ret == 0) {
> >> +        /* child, start the shutdown */
> >> +        setsid();
> >> +        fclose(stdin);
> >> +        fclose(stdout);
> >> +        fclose(stderr);
> >
> > Would be nice to have a log file, whose descriptor is passed to the
> > child.
> >
> >> +
> >> +        sleep(5);
> >
> > Why sleep()?
> >
> 
> Want to give the agent time to send a response. It's still racy, but 
> less so that immediately issuing the shutdown.

It's only less race for the particular test-case you're testing :)

> Ideal we'd push the 
> sleep() into shutdown's time param, but that only has minute resolution.

I don't think so, because this is an implementation detail. The right thing
would be to make the child wait for an "go ahead" signal from the parent.

This could be done by calling pause() in the child and making the parent
send a SIGUSR1 when the response has been sent.

One way to do it w/o tying sever actions (like to send a response) to
agent commands, would be to introduce a "response sent" callback, so that
commands could register and have their specific actions executed at the
right time.

> 
> >> +        ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> >> +                    "hypervisor initiated shutdown", (char*)NULL);
> >> +        exit(!!ret);
> >> +    } else if (ret<  0) {
> >> +        error_set(err, QERR_UNDEFINED_ERROR);
> >> +    }
> >
> > Doesn't have the parent process wait for the child, so that exec() errors
> > can be reported?
> 
> The exec() won't return until the shutdown is executed, so the RPC's 
> behavior would be racy. At some point I documented that the shutdown is 
> an async "request" that may or may not complete but that was lost in the 
> reworking. I'll clarify in the schema.

Seems fine.

> 
> >
> >> +}
> >> +
> >> +typedef struct GuestFileHandle {
> >> +    uint64_t id;
> >> +    FILE *fh;
> >> +} GuestFileHandle;
> >> +
> >> +static struct {
> >> +    GSList *filehandles;
> >
> > Wouldn't this be simpler if we use qemu list implementation instead?
> >
> 
> YES! This stuff is terribly verbose. I was trying to stick with glib 
> outside of QMP/QAPI stuff though...and I think more glib stuff will find 
> it's way into here over time that'll make appearances of 
> GSList/gmalloc/etc inevitable.
> 
> But if intermixing is not a big deal I'm more than happy to "allow" qemu 
> malloc/list stuff where it makes sense, and refactor these accordingly.

It makes sense to me.

> >> +    uint64_t last_id;
> >> +} guest_file_state;
> >> +
> >> +static int64_t guest_file_handle_add(FILE *fh)
> >> +{
> >> +    GuestFileHandle *gfh;
> >> +
> >> +    gfh = g_malloc(sizeof(GuestFileHandle));
> >> +    gfh->id = guest_file_state.last_id++;
> >
> > I don't know if the uint64_t limit can be reached in practice, but I'd
> > expect a bitmap, so that you can return ids in guest_file_handle_remove().
> >
> > Another simpler option would be to use the real fd instead. I mean, the one
> > returned by the guest kernel.
> >
> 
> I think I'll go with this. I was hesitant to do this at first since I 
> didn't want users specifying FDs opened outside of guest-file-open, but 
> so long as we only rely on our internal list of open FDs I guess that's 
> not applicable.

Yes.

> >> +    gfh->fh = fh;
> >> +    guest_file_state.filehandles = 
> >> g_slist_append(guest_file_state.filehandles,
> >> +                                                  gfh);
> >> +    return gfh->id;
> >> +}
> >> +
> >> +static gint guest_file_handle_match(gconstpointer elem, gconstpointer 
> >> id_p)
> >> +{
> >> +    const uint64_t *id = id_p;
> >> +    const GuestFileHandle *gfh = elem;
> >> +
> >> +    g_assert(gfh);
> >> +    return (gfh->id != *id);
> >> +}
> >> +
> >> +static FILE *guest_file_handle_find(int64_t id)
> >> +{
> >> +    GSList *elem = g_slist_find_custom(guest_file_state.filehandles,&id,
> >> +                                       guest_file_handle_match);
> >> +    GuestFileHandle *gfh;
> >> +
> >> +    if (elem) {
> >> +        g_assert(elem->data);
> >> +        gfh = elem->data;
> >> +        return gfh->fh;
> >> +    }
> >> +
> >> +    return NULL;
> >> +}
> >> +
> >> +static void guest_file_handle_remove(int64_t id)
> >> +{
> >> +    GSList *elem = g_slist_find_custom(guest_file_state.filehandles,&id,
> >> +                                       guest_file_handle_match);
> >> +    gpointer data = elem->data;
> >> +
> >> +    if (!data) {
> >> +        return;
> >> +    }
> >> +    guest_file_state.filehandles = 
> >> g_slist_remove(guest_file_state.filehandles,
> >> +                                                  data);
> >> +    g_free(data);
> >> +}
> >> +
> >> +int64_t qmp_guest_file_open(const char *filepath, const char *mode, Error 
> >> **err)
> >> +{
> >> +    FILE *fh;
> >> +    int fd, ret;
> >> +    int64_t id = -1;
> >> +
> >> +    if (!logging_enabled()) {
> >> +        error_set(err, QERR_QGA_LOGGING_FAILED);
> >> +        goto out;
> >
> > No need to have a goto here, just do return -1. This true for other 
> > functions
> > too.
> >
> >> +    }
> >> +    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);
> >> +        goto out;
> >> +    }
> >> +
> >> +    /* 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);
> >> +        goto out;
> >> +    }
> >> +
> >> +    id = guest_file_handle_add(fh);
> >> +    slog("guest-file-open, filehandle: %ld", id);
> >> +out:
> >> +    return id;
> >> +}
> >> +
> >> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t 
> >> count,
> >> +                                          Error **err)
> >> +{
> >> +    GuestFileRead *read_data;
> >> +    guchar *buf;
> >> +    FILE *fh = guest_file_handle_find(filehandle);
> >> +    size_t read_count;
> >> +
> >> +    if (!fh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    read_data = g_malloc0(sizeof(GuestFileRead));
> >> +    buf = g_malloc(count);
> >
> > What happens if the client passes a bogus value? Like -1 or a very big
> > number?
> >
> > I think count has to be checked against the file size. You could call stat()
> > and store the value. Also, you're not checking g_malloc()'s return, so a bad
> > allocation can crash the agent.
> >
> 
> All good points
> 
> >> +
> >> +    read_count = fread(buf, 1, count, fh);
> >> +    buf[read_count] = 0;
> >
> > We need to allocate an additional byte to do that.
> >
> >> +    read_data->count = read_count;
> >> +    read_data->eof = feof(fh);
> >> +    if (read_count) {
> >> +        read_data->buf = g_base64_encode(buf, read_count);
> >> +    }
> >> +    g_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;
> >> +    FILE *fh = guest_file_handle_find(filehandle);
> >> +
> >> +    if (!fh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    write_data = g_malloc0(sizeof(GuestFileWrite));
> >> +    data = g_base64_decode(data_b64,&data_len);
> >> +    write_count = fwrite(data, 1, MIN(count, data_len), fh);
> >
> > IMO, we should do what the user is asking us to do, if it's impossible we
> > should return an error. IOW, we should use count. It's okay if the buffer
> > is bigger then count, but if count is bigger then we should return an error.
> >
> > What does write() do in this case? segfaults?
> >
> >> +    write_data->count = write_count;
> >> +    write_data->eof = feof(fh);
> >> +    g_free(data);
> >> +    clearerr(fh);
> >> +
> >> +    return write_data;
> >> +}
> >> +
> >> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t 
> >> offset,
> >> +                                          int64_t whence, Error **err)
> >> +{
> >> +    GuestFileSeek *seek_data;
> >> +    FILE *fh = guest_file_handle_find(filehandle);
> >> +    int ret;
> >> +
> >> +    if (!fh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return NULL;
> >> +    }
> >> +
> >> +    seek_data = g_malloc0(sizeof(GuestFileRead));
> >> +    ret = fseek(fh, offset, whence);
> >> +    if (ret == -1) {
> >> +        error_set(err, QERR_UNDEFINED_ERROR);
> >> +        g_free(seek_data);
> >> +        return NULL;
> >> +    }
> >> +    seek_data->position = ftell(fh);
> >> +    seek_data->eof = feof(fh);
> >> +    clearerr(fh);
> >> +
> >> +    return seek_data;
> >> +}
> >> +
> >> +void qmp_guest_file_close(int64_t filehandle, Error **err)
> >> +{
> >> +    FILE *fh = guest_file_handle_find(filehandle);
> >> +
> >> +    if (!logging_enabled()) {
> >> +        error_set(err, QERR_QGA_LOGGING_FAILED);
> >> +        return;
> >> +    }
> >> +    slog("guest-file-close called, filehandle: %ld", filehandle);
> >> +    if (!fh) {
> >> +        error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >> +        return;
> >> +    }
> >> +
> >> +    fclose(fh);
> >> +    guest_file_handle_remove(filehandle);
> >> +}
> >> +
> >> +/*
> >> + * Walk the mount table and build a list of local file systems
> >> + */
> >> +
> >> +struct direntry {
> >> +    char *dirname;
> >> +    char *devtype;
> >> +    struct direntry *next;
> >
> > Wouldn't it be better to use qemu's list implementation?
> >
> 
> yes, ill fix all the list stuff up to use qemu's
> 
> >> +};
> >> +
> >> +struct {
> >> +    struct direntry *mount_list;
> >> +    GuestFsfreezeStatus status;
> >> +} guest_fsfreeze_state;
> >> +
> >> +static int guest_fsfreeze_build_mount_list(void)
> >> +{
> >> +    struct mntent *mnt;
> >> +    struct direntry *entry;
> >> +    struct direntry *next;
> >> +    char const *mtab = MOUNTED;
> >> +    FILE *fp;
> >> +
> >> +    fp = setmntent(mtab, "r");
> >> +    if (!fp) {
> >> +        g_warning("fsfreeze: unable to read mtab");
> >> +        goto fail;
> >> +    }
> >> +
> >> +    while ((mnt = 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 ((mnt->mnt_fsname[0] != '/') ||
> >> +            (strcmp(mnt->mnt_type, "smbfs") == 0) ||
> >> +            (strcmp(mnt->mnt_type, "cifs") == 0)) {
> >> +            continue;
> >> +        }
> >> +
> >> +        entry = g_malloc(sizeof(struct direntry));
> >> +        entry->dirname = qemu_strdup(mnt->mnt_dir);
> >> +        entry->devtype = qemu_strdup(mnt->mnt_type);
> >> +        entry->next = guest_fsfreeze_state.mount_list;
> >> +
> >> +        guest_fsfreeze_state.mount_list = entry;
> >> +    }
> >> +
> >> +    endmntent(fp);
> >> +
> >> +    return 0;
> >> +
> >> +fail:
> >> +    while(guest_fsfreeze_state.mount_list) {
> >> +        next = guest_fsfreeze_state.mount_list->next;
> >> +        g_free(guest_fsfreeze_state.mount_list->dirname);
> >> +        g_free(guest_fsfreeze_state.mount_list->devtype);
> >> +        g_free(guest_fsfreeze_state.mount_list);
> >> +        guest_fsfreeze_state.mount_list = next;
> >> +    }
> >> +
> >> +    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 direntry *entry;
> >> +    int fd;
> >> +
> >> +    if (!logging_enabled()) {
> >> +        error_set(err, QERR_QGA_LOGGING_FAILED);
> >> +        goto out;
> >> +    }
> >> +
> >> +    slog("guest-fsfreeze called");
> >> +
> >> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
> >> +        ret = 0;
> >> +        goto out;
> >> +    }
> >> +
> >> +    ret = guest_fsfreeze_build_mount_list();
> >> +    if (ret<  0) {
> >> +        goto out;
> >> +    }
> >> +
> >> +    guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
> >> +
> >> +    /* cannot risk guest agent blocking itself on a write in this state */
> >> +    disable_logging();
> >> +
> >> +    entry = guest_fsfreeze_state.mount_list;
> >> +    while(entry) {
> >
> > A for() loop would be clearer, imho.
> >
> >> +        fd = qemu_open(entry->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;
> >
> > Does the FIFREEZE ioctl() call returns the errno code? If it doesn't, then
> > we have to set it as the qemu_open() does above.
> >
> 
> Nope, -1 + errno, good catch.
> 
> >> +        }
> >> +        close(fd);
> >> +
> >> +        entry = entry->next;
> >> +        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;
> >> +    }
> >
> > I'm not sure this correct. What happens if an error happens in the
> > first iteration of the while loop? A better way would to check 'ret'
> > instead.
> >
> 
> I believe this is meant to track states where some filesystems have been 
> frozen and others not. We want to return the count, but not the error 
> via fsfreeze_status. If we bail in the first iteration it means FIFREEZE 
> was never completed successfully. We should reset state the THAWED then 
> though.
> 
> >> +    goto out;
> >> +}
> >> +
> >> +/*
> >> + * Walk list of frozen file systems in the guest, and thaw them.
> >> + */
> >> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >> +{
> >> +    int ret;
> >> +    struct direntry *entry;
> >> +    int fd, i = 0;
> >> +
> >> +    if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
> >> +        guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_INPROGRESS) {
> >> +        ret = 0;
> >> +        goto out_enable_logging;
> >> +    }
> >> +
> >> +    while((entry = guest_fsfreeze_state.mount_list)) {
> >> +        fd = qemu_open(entry->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;
> >> +        }
> >> +        close(fd);
> >> +
> >> +        guest_fsfreeze_state.mount_list = entry->next;
> >> +        g_free(entry->dirname);
> >> +        g_free(entry->devtype);
> >> +        g_free(entry);
> >> +        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;
> >> +}
> >> +
> >> +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);
> >> +}
> >> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >> index 66d1729..a85a5e4 100644
> >> --- a/qga/guest-agent-core.h
> >> +++ b/qga/guest-agent-core.h
> >> @@ -18,6 +18,7 @@
> >>   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));
> >
>