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

[Luiz Capitulino]

On Tue, 12 Jul 2011 10:44:14 -0500
Michael Roth <address@hidden> wrote:

> On 07/12/2011 09:15 AM, Luiz Capitulino wrote:
> > On Mon, 11 Jul 2011 18:11:21 -0500
> > Michael Roth<address@hidden>  wrote:
> >
> >> On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
> >>> On Mon, 11 Jul 2011 15:11:26 -0500
> >>> Michael Roth<address@hidden>   wrote:
> >>>
> >>>> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> >>>>> On Tue,  5 Jul 2011 08:21:40 -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.
> >>>>>>
> >>>>>> 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<address@hidden>
> >>>>>> ---
> >>>>>>     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<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"
> >>>>>> +#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));
> >>>>>
> >>>>
> >>>
> >>
> >
>