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

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

On 06/16/2011 01:52 PM, Luiz Capitulino wrote:
> On Tue, 14 Jun 2011 15:06:23 -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.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   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<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"
>> +
>> +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. Ideal we'd push the 
sleep() into shutdown's time param, but that only has minute resolution.

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

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

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

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