qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Luiz Capitulino
Subject: Re: [Qemu-devel] [PATCH v6 4/4] guest agent: add guest agent RPCs/commands
Date: Tue, 12 Jul 2011 11:15:54 -0300

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

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]