On Tue, 12 Jul 2011 10:44:14 -0500
Michael Roth <mdr...@linux.vnet.ibm.com> wrote:
> On 07/12/2011 09:15 AM, Luiz Capitulino wrote:
> > On Mon, 11 Jul 2011 18:11:21 -0500
> > Michael Roth<mdr...@linux.vnet.ibm.com> wrote:
> >
> >> On 07/11/2011 04:12 PM, Luiz Capitulino wrote:
> >>> On Mon, 11 Jul 2011 15:11:26 -0500
> >>> Michael Roth<mdr...@linux.vnet.ibm.com> wrote:
> >>>
> >>>> On 07/08/2011 10:14 AM, Luiz Capitulino wrote:
> >>>>> On Tue, 5 Jul 2011 08:21:40 -0500
> >>>>> Michael Roth<mdr...@linux.vnet.ibm.com> wrote:
> >>>>>
> >>>>>> This adds the initial set of QMP/QAPI commands provided by the guest
> >>>>>> agent:
> >>>>>>
> >>>>>> guest-sync
> >>>>>> guest-ping
> >>>>>> guest-info
> >>>>>> guest-shutdown
> >>>>>> guest-file-open
> >>>>>> guest-file-read
> >>>>>> guest-file-write
> >>>>>> guest-file-seek
> >>>>>> guest-file-close
> >>>>>> guest-fsfreeze-freeze
> >>>>>> guest-fsfreeze-thaw
> >>>>>> guest-fsfreeze-status
> >>>>>>
> >>>>>> The input/output specification for these commands are documented in the
> >>>>>> schema.
> >>>>>>
> >>>>>> Example usage:
> >>>>>>
> >>>>>> host:
> >>>>>> qemu -device virtio-serial \
> >>>>>> -chardev socket,path=/tmp/vs0.sock,server,nowait,id=qga0 \
> >>>>>> -device virtserialport,chardev=qga0,name=qga0
> >>>>>> ...
> >>>>>>
> >>>>>> echo "{'execute':'guest-info'}" | socat stdio \
> >>>>>> unix-connect:/tmp/qga0.sock
> >>>>>>
> >>>>>> guest:
> >>>>>> qemu-ga -c virtio-serial -p /dev/virtio-ports/qga0 \
> >>>>>> -p /var/run/qemu-guest-agent.pid -d
> >>>>>>
> >>>>>> Signed-off-by: Michael Roth<mdr...@linux.vnet.ibm.com>
> >>>>>> ---
> >>>>>> Makefile | 15 ++-
> >>>>>> qemu-ga.c | 4 +
> >>>>>> qerror.h | 3 +
> >>>>>> qga/guest-agent-commands.c | 501
> >>>>>> ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>> qga/guest-agent-core.h | 2 +
> >>>>>> 5 files changed, 523 insertions(+), 2 deletions(-)
> >>>>>> create mode 100644 qga/guest-agent-commands.c
> >>>>>>
> >>>>>> diff --git a/Makefile b/Makefile
> >>>>>> index b2e8593..7e4f722 100644
> >>>>>> --- a/Makefile
> >>>>>> +++ b/Makefile
> >>>>>> @@ -175,15 +175,26 @@ $(qapi-dir)/test-qmp-commands.h:
> >>>>>> $(qapi-dir)/test-qmp-marshal.c
> >>>>>> $(qapi-dir)/test-qmp-marshal.c: $(SRC_PATH)/qapi-schema-test.json
> >>>>>> $(SRC_PATH)/scripts/qapi-commands.py
> >>>>>> $(call quiet-command,python
> >>>>>> $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "test-"<
> >>>>>> $<, " GEN $@")
> >>>>>>
> >>>>>> +$(qapi-dir)/qga-qapi-types.c: $(qapi-dir)/qga-qapi-types.h
> >>>>>> +$(qapi-dir)/qga-qapi-types.h: $(SRC_PATH)/qapi-schema-guest.json
> >>>>>> $(SRC_PATH)/scripts/qapi-types.py
> >>>>>> + $(call quiet-command,python $(SRC_PATH)/scripts/qapi-types.py
> >>>>>> -o "$(qapi-dir)" -p "qga-"< $<, " GEN $@")
> >>>>>> +$(qapi-dir)/qga-qapi-visit.c: $(qapi-dir)/qga-qapi-visit.h
> >>>>>> +$(qapi-dir)/qga-qapi-visit.h: $(SRC_PATH)/qapi-schema-guest.json
> >>>>>> $(SRC_PATH)/scripts/qapi-visit.py
> >>>>>> + $(call quiet-command,python $(SRC_PATH)/scripts/qapi-visit.py
> >>>>>> -o "$(qapi-dir)" -p "qga-"< $<, " GEN $@")
> >>>>>> +$(qapi-dir)/qga-qmp-marshal.c: $(SRC_PATH)/qapi-schema-guest.json
> >>>>>> $(SRC_PATH)/scripts/qapi-commands.py
> >>>>>> + $(call quiet-command,python
> >>>>>> $(SRC_PATH)/scripts/qapi-commands.py -o "$(qapi-dir)" -p "qga-"<
> >>>>>> $<, " GEN $@")
> >>>>>> +
> >>>>>> test-visitor.o: $(addprefix $(qapi-dir)/, test-qapi-types.c
> >>>>>> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h)
> >>>>>> test-visitor: test-visitor.o qfloat.o qint.o qdict.o qstring.o
> >>>>>> qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o
> >>>>>> $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o
> >>>>>> qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o
> >>>>>> $(qapi-dir)/test-qapi-types.o
> >>>>>>
> >>>>>> test-qmp-commands.o: $(addprefix $(qapi-dir)/, test-qapi-types.c
> >>>>>> test-qapi-types.h test-qapi-visit.c test-qapi-visit.h
> >>>>>> test-qmp-marshal.c test-qmp-commands.h)
> >>>>>> test-qmp-commands: test-qmp-commands.o qfloat.o qint.o qdict.o
> >>>>>> qstring.o qlist.o qbool.o $(qapi-obj-y) error.o osdep.o qemu-malloc.o
> >>>>>> $(oslib-obj-y) qjson.o json-streamer.o json-lexer.o json-parser.o
> >>>>>> qerror.o qemu-error.o qemu-tool.o $(qapi-dir)/test-qapi-visit.o
> >>>>>> $(qapi-dir)/test-qapi-types.o $(qapi-dir)/test-qmp-marshal.o module.o
> >>>>>>
> >>>>>> -QGALIB=qga/guest-agent-command-state.o
> >>>>>> +QGALIB=qga/guest-agent-command-state.o qga/guest-agent-commands.o
> >>>>>> +
> >>>>>> +qemu-ga.o: $(qapi-dir)/qga-qapi-types.c $(qapi-dir)/qga-qapi-types.h
> >>>>>> $(qapi-dir)/qga-qapi-visit.c $(qapi-dir)/qga-qmp-marshal.c
> >>>>>>
> >>>>>> -qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o
> >>>>>> error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y)
> >>>>>> $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o
> >>>>>> module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
> >>>>>> +qemu-ga$(EXESUF): qemu-ga.o $(QGALIB) qemu-tool.o qemu-error.o
> >>>>>> error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y)
> >>>>>> $(version-obj-y) $(qapi-obj-y) qemu-timer-common.o qemu-sockets.o
> >>>>>> module.o qapi/qmp-dispatch.o qapi/qmp-registry.o
> >>>>>> $(qapi-dir)/qga-qapi-visit.o $(qapi-dir)/qga-qmp-marshal.o
> >>>>>>
> >>>>>> QEMULIBS=libhw32 libhw64 libuser libdis libdis-user
> >>>>>>
> >>>>>> diff --git a/qemu-ga.c b/qemu-ga.c
> >>>>>> index 649c16a..04ead22 100644
> >>>>>> --- a/qemu-ga.c
> >>>>>> +++ b/qemu-ga.c
> >>>>>> @@ -637,6 +637,9 @@ int main(int argc, char **argv)
> >>>>>> g_log_set_default_handler(ga_log, s);
> >>>>>> g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> >>>>>> s->logging_enabled = true;
> >>>>>> + s->command_state = ga_command_state_new();
> >>>>>> + ga_command_state_init(s, s->command_state);
> >>>>>> + ga_command_state_init_all(s->command_state);
> >>>>>> ga_state = s;
> >>>>>>
> >>>>>> module_call_init(MODULE_INIT_QAPI);
> >>>>>> @@ -645,6 +648,7 @@ int main(int argc, char **argv)
> >>>>>>
> >>>>>> g_main_loop_run(ga_state->main_loop);
> >>>>>>
> >>>>>> + ga_command_state_cleanup_all(ga_state->command_state);
> >>>>>> unlink(pidfile);
> >>>>>>
> >>>>>> return 0;
> >>>>>> diff --git a/qerror.h b/qerror.h
> >>>>>> index 9a9fa5b..0f618ac 100644
> >>>>>> --- a/qerror.h
> >>>>>> +++ b/qerror.h
> >>>>>> @@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
> >>>>>> #define QERR_FEATURE_DISABLED \
> >>>>>> "{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
> >>>>>>
> >>>>>> +#define QERR_QGA_LOGGING_FAILED \
> >>>>>> + "{ 'class': 'QgaLoggingFailed', 'data': {} }"
> >>>>>> +
> >>>>>> #endif /* QERROR_H */
> >>>>>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >>>>>> new file mode 100644
> >>>>>> index 0000000..42390fb
> >>>>>> --- /dev/null
> >>>>>> +++ b/qga/guest-agent-commands.c
> >>>>>> @@ -0,0 +1,501 @@
> >>>>>> +/*
> >>>>>> + * QEMU Guest Agent commands
> >>>>>> + *
> >>>>>> + * Copyright IBM Corp. 2011
> >>>>>> + *
> >>>>>> + * Authors:
> >>>>>> + * Michael Roth<mdr...@linux.vnet.ibm.com>
> >>>>>> + *
> >>>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
> >>>>>> later.
> >>>>>> + * See the COPYING file in the top-level directory.
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include<glib.h>
> >>>>>> +#include<mntent.h>
> >>>>>> +#include<sys/types.h>
> >>>>>> +#include<sys/ioctl.h>
> >>>>>> +#include<linux/fs.h>
> >>>>>> +#include "qga/guest-agent-core.h"
> >>>>>> +#include "qga-qmp-commands.h"
> >>>>>> +#include "qerror.h"
> >>>>>> +#include "qemu-queue.h"
> >>>>>> +
> >>>>>> +static GAState *ga_state;
> >>>>>> +
> >>>>>> +static void disable_logging(void)
> >>>>>> +{
> >>>>>> + ga_disable_logging(ga_state);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void enable_logging(void)
> >>>>>> +{
> >>>>>> + ga_enable_logging(ga_state);
> >>>>>> +}
> >>>>>> +
> >>>>>> +/* Note: in some situations, like with the fsfreeze, logging may be
> >>>>>> + * temporarilly disabled. if it is necessary that a command be able
> >>>>>> + * to log for accounting purposes, check ga_logging_enabled()
> >>>>>> beforehand,
> >>>>>> + * and use the QERR_QGA_LOGGING_DISABLED to generate an error
> >>>>>> + */
> >>>>>> +static void slog(const char *fmt, ...)
> >>>>>> +{
> >>>>>> + va_list ap;
> >>>>>> +
> >>>>>> + va_start(ap, fmt);
> >>>>>> + g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
> >>>>>> + va_end(ap);
> >>>>>> +}
> >>>>>> +
> >>>>>> +int64_t qmp_guest_sync(int64_t id, Error **errp)
> >>>>>> +{
> >>>>>> + return id;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qmp_guest_ping(Error **err)
> >>>>>> +{
> >>>>>> + slog("guest-ping called");
> >>>>>> +}
> >>>>>> +
> >>>>>> +struct GuestAgentInfo *qmp_guest_info(Error **err)
> >>>>>> +{
> >>>>>> + GuestAgentInfo *info = qemu_mallocz(sizeof(GuestAgentInfo));
> >>>>>> +
> >>>>>> + info->version = g_strdup(QGA_VERSION);
> >>>>>> +
> >>>>>> + return info;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qmp_guest_shutdown(const char *mode, Error **err)
> >>>>>> +{
> >>>>>> + int ret;
> >>>>>> + const char *shutdown_flag;
> >>>>>> +
> >>>>>> + slog("guest-shutdown called, mode: %s", mode);
> >>>>>> + if (strcmp(mode, "halt") == 0) {
> >>>>>> + shutdown_flag = "-H";
> >>>>>> + } else if (strcmp(mode, "powerdown") == 0) {
> >>>>>> + shutdown_flag = "-P";
> >>>>>> + } else if (strcmp(mode, "reboot") == 0) {
> >>>>>> + shutdown_flag = "-r";
> >>>>>> + } else {
> >>>>>> + error_set(err, QERR_INVALID_PARAMETER_VALUE, "mode",
> >>>>>> + "halt|powerdown|reboot");
> >>>>>> + return;
> >>>>>> + }
> >>>>>> +
> >>>>>> + ret = fork();
> >>>>>> + if (ret == 0) {
> >>>>>> + /* child, start the shutdown */
> >>>>>> + setsid();
> >>>>>> + fclose(stdin);
> >>>>>> + fclose(stdout);
> >>>>>> + fclose(stderr);
> >>>>>> +
> >>>>>> + sleep(5);
> >>>>>
> >>>>> If we're required to return a response before the shutdown happens, this
> >>>>> is a bug and I'm afraid that the right way to this is a bit complex.
> >>>>>
> >>>>> Otherwise we can just leave it out.
> >>>>>
> >>>>
> >>>> Yah, I ran this by Anthony and Adam and just leaving it out seems to be
> >>>> the preferred approach, for now at least.
> >>>>
> >>>>>> + ret = execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> >>>>>> + "hypervisor initiated shutdown", (char*)NULL);
> >>>>>> + if (ret) {
> >>>>>> + slog("guest-shutdown failed: %s", strerror(errno));
> >>>>>> + }
> >>>>>> + exit(!!ret);
> >>>>>> + } else if (ret< 0) {
> >>>>>> + error_set(err, QERR_UNDEFINED_ERROR);
> >>>>>> + }
> >>>>>> +}
> >>>>>> +
> >>>>>> +typedef struct GuestFileHandle {
> >>>>>> + uint64_t id;
> >>>>>> + FILE *fh;
> >>>>>> + QTAILQ_ENTRY(GuestFileHandle) next;
> >>>>>> +} GuestFileHandle;
> >>>>>> +
> >>>>>> +static struct {
> >>>>>> + QTAILQ_HEAD(, GuestFileHandle) filehandles;
> >>>>>> +} guest_file_state;
> >>>>>> +
> >>>>>> +static void guest_file_handle_add(FILE *fh)
> >>>>>> +{
> >>>>>> + GuestFileHandle *gfh;
> >>>>>> +
> >>>>>> + gfh = qemu_mallocz(sizeof(GuestFileHandle));
> >>>>>> + gfh->id = fileno(fh);
> >>>>>> + gfh->fh = fh;
> >>>>>> + QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static GuestFileHandle *guest_file_handle_find(int64_t id)
> >>>>>> +{
> >>>>>> + GuestFileHandle *gfh;
> >>>>>> +
> >>>>>> + QTAILQ_FOREACH(gfh,&guest_file_state.filehandles, next)
> >>>>>> + {
> >>>>>> + if (gfh->id == id) {
> >>>>>> + return gfh;
> >>>>>> + }
> >>>>>> + }
> >>>>>> +
> >>>>>> + return NULL;
> >>>>>> +}
> >>>>>> +
> >>>>>> +int64_t qmp_guest_file_open(const char *filepath, bool has_mode,
> >>>>>> const char *mode, Error **err)
> >>>>>> +{
> >>>>>> + FILE *fh;
> >>>>>> + int fd;
> >>>>>> + int64_t ret = -1;
> >>>>>> +
> >>>>>> + if (!has_mode) {
> >>>>>> + mode = "r";
> >>>>>> + }
> >>>>>> + slog("guest-file-open called, filepath: %s, mode: %s", filepath,
> >>>>>> mode);
> >>>>>> + fh = fopen(filepath, mode);
> >>>>>> + if (!fh) {
> >>>>>> + error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >>>>>> + return -1;
> >>>>>> + }
> >>>>>> +
> >>>>>> + /* set fd non-blocking to avoid common use cases (like reading
> >>>>>> from a
> >>>>>> + * named pipe) from hanging the agent
> >>>>>> + */
> >>>>>> + fd = fileno(fh);
> >>>>>> + ret = fcntl(fd, F_GETFL);
> >>>>>> + ret = fcntl(fd, F_SETFL, ret | O_NONBLOCK);
> >>>>>> + if (ret == -1) {
> >>>>>> + error_set(err, QERR_OPEN_FILE_FAILED, filepath);
> >>>>>> + fclose(fh);
> >>>>>> + return -1;
> >>>>>> + }
> >>>>>> +
> >>>>>> + guest_file_handle_add(fh);
> >>>>>> + slog("guest-file-open, filehandle: %d", fd);
> >>>>>> + return fd;
> >>>>>> +}
> >>>>>> +
> >>>>>> +struct GuestFileRead *qmp_guest_file_read(int64_t filehandle, int64_t
> >>>>>> count,
> >>>>>> + Error **err)
> >>>>>> +{
> >>>>>> + GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> + GuestFileRead *read_data;
> >>>>>> + guchar *buf;
> >>>>>> + FILE *fh;
> >>>>>> + size_t read_count;
> >>>>>> +
> >>>>>> + if (!gfh) {
> >>>>>> + error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (count< 0 || count> QGA_READ_LIMIT) {
> >>>>>> + error_set(err, QERR_INVALID_PARAMETER, "count");
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>
> >>>>> Are we imposing that limit because of the malloc() call below? If that's
> >>>>> the case I think it's wrong, because we don't know the VM (neither the
> >>>>> guest)
> >>>>> better than the client.
> >>>>>
> >>>>> The best thing we can do here is to limit it to the file size.
> >>>>> Additionally
> >>>>> to this we could have a command-line option to allow the sysadmin set
> >>>>> his/her
> >>>>> own limit.
> >>>>>
> >>>>
> >>>> That's technically the better approach, but we're also bound by the
> >>>> maximum token size limit in the JSON parser, which is 64MB. Best we can
> >>>> do is set QGA_READ_LIMIT accordingly.
> >>>
> >>> Good point, but I think it's ok to let the parser do this check itself, as
> >>> control won't get here anyway. Maybe we should only document that the
> >>> server
> >>> imposes a limit on the token size.
> >>>
> >>
> >> From a usability perspective I think the QERR_INVALID_PARAMETER, or
> >> perhaps something more descriptive, is a little nicer. Plus, they won't
> >> have to wait for 64MB to get streamed back before they get the error :)
> >
> > That's why I suggested documenting it. Are we going to add this check
> > to all commands that make it more likely to reach the parser's limit?
> > (also note that theoretically all commands can do it).
> >
>
> Good point, and I agree that it needs to be documented either way. Might
> not hurt to further clarify the limitations this imposes on a command
> like guest-file-read in the schema documentation where appropriate though.
>
> I'm not sure about the alternative suggestion of bounding this by
> filesize though, since they may also be writing to the same fd via
> guest-file-write, and the current file size wouldn't be meaningful
> except after an fflush() or fclose().
This is just like the OS, the application has to what's doing.
> So we'd be forced to fflush() in
> guest-file-write to implement this somewhat reliably, but I think we
> agree that a separate guest-file-flush would be better.
Yes.
> So let's just let the client blow up their guest if they're so inclined.
Exactly :) I mean, if something like this turns out to be a problem, we'd
have to fix it differently IMO.
>
> >>
> >>>>
> >>>>>> +
> >>>>>> + fh = gfh->fh;
> >>>>>> + read_data = qemu_mallocz(sizeof(GuestFileRead) + 1);
> >>>>>> + buf = qemu_mallocz(count+1);
> >>>>>> + if (!buf) {
> >>>>>> + error_set(err, QERR_UNDEFINED_ERROR);
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>
> >>>>> qemu_malloc() functions never fail...
> >>>>>
> >>>>>> +
> >>>>>> + read_count = fread(buf, 1, count, fh);
> >>>>>
> >>>>> Isn't 'nmemb' and 'size' swapped?
> >>>>>
> >>>>
> >>>> I'm not sure...
> >>>>
> >>>> size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
> >>>>
> >>>> I wrote it as $count number of 1-bytes elements. This seems logical to
> >>>> me, since it's a non-blocking FD which way result in a partial of some
> >>>> lesser number of bytes than count.
> >>>
> >>> Ok. I think either way will work.
> >>>
> >>>>
> >>>>>> + buf[read_count] = 0;
> >>>>>> + read_data->count = read_count;
> >>>>>> + read_data->eof = feof(fh);
> >>>>>> + if (read_count) {
> >>>>>> + read_data->buf = g_base64_encode(buf, read_count);
> >>>>>> + }
> >>>>>> + qemu_free(buf);
> >>>>>> + /* clear error and eof. error is generally due to EAGAIN from
> >>>>>> non-blocking
> >>>>>> + * mode, and no real way to differenitate from a real error since
> >>>>>> we only
> >>>>>> + * get boolean error flag from ferror()
> >>>>>> + */
> >>>>>> + clearerr(fh);
> >>>>>> +
> >>>>>> + return read_data;
> >>>>>> +}
> >>>>>> +
> >>>>>> +GuestFileWrite *qmp_guest_file_write(int64_t filehandle, const char
> >>>>>> *data_b64,
> >>>>>> + int64_t count, Error **err)
> >>>>>> +{
> >>>>>> + GuestFileWrite *write_data;
> >>>>>> + guchar *data;
> >>>>>> + gsize data_len;
> >>>>>> + int write_count;
> >>>>>> + GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> + FILE *fh;
> >>>>>> +
> >>>>>> + if (!gfh) {
> >>>>>> + error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>> +
> >>>>>> + fh = gfh->fh;
> >>>>>> + data = g_base64_decode(data_b64,&data_len);
> >>>>>> + if (count> data_len) {
> >>>>>> + qemu_free(data);
> >>>>>> + error_set(err, QERR_INVALID_PARAMETER, "count");
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>> + write_data = qemu_mallocz(sizeof(GuestFileWrite));
> >>>>>> + write_count = fwrite(data, 1, count, fh);
> >>>>>> + write_data->count = write_count;
> >>>>>> + write_data->eof = feof(fh);
> >>>>>> + qemu_free(data);
> >>>>>> + clearerr(fh);
> >>>>>
> >>>>> Shouldn't we check for errors instead of doing this?
> >>>>>
> >>>>
> >>>> Yah...unfortunately we only get a boolean flag with ferror() so it's not
> >>>> all that useful, but I can add an error flag to the calls to capture it
> >>>> at least. clearerr() is only being used here to clear the eof flag.
> >>>
> >>> I meant to check fwrite()'s return.
> >>>
> >>
> >> Ahh, right. Definitely.
> >>
> >>>>
> >>>>> Btw, I think it's a good idea to offer guest-file-flush() too (or do a
> >>>>> flush()
> >>>>> here, but that's probably bad).
> >>>>>
> >>>>
> >>>> Yah, I'd been planning on adding a guest-file-flush() for a while now.
> >>>> I'll add that for the respin.
> >>>>
> >>>>>> +
> >>>>>> + return write_data;
> >>>>>> +}
> >>>>>> +
> >>>>>> +struct GuestFileSeek *qmp_guest_file_seek(int64_t filehandle, int64_t
> >>>>>> offset,
> >>>>>> + int64_t whence, Error **err)
> >>>>>> +{
> >>>>>> + GuestFileSeek *seek_data;
> >>>>>> + GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> + FILE *fh;
> >>>>>> + int ret;
> >>>>>> +
> >>>>>> + if (!gfh) {
> >>>>>> + error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>> +
> >>>>>> + fh = gfh->fh;
> >>>>>> + seek_data = qemu_mallocz(sizeof(GuestFileRead));
> >>>>>> + ret = fseek(fh, offset, whence);
> >>>>>> + if (ret == -1) {
> >>>>>> + error_set(err, QERR_UNDEFINED_ERROR);
> >>>>>> + qemu_free(seek_data);
> >>>>>> + return NULL;
> >>>>>> + }
> >>>>>> + seek_data->position = ftell(fh);
> >>>>>> + seek_data->eof = feof(fh);
> >>>>>> + clearerr(fh);
> >>>>>
> >>>>> Again, I don't see why we should do this.
> >>>
> >>> This is probably ok, as we're checking fseek() above.
> >>>
> >>>>>
> >>>>>> +
> >>>>>> + return seek_data;
> >>>>>> +}
> >>>>>> +
> >>>>>> +void qmp_guest_file_close(int64_t filehandle, Error **err)
> >>>>>> +{
> >>>>>> + GuestFileHandle *gfh = guest_file_handle_find(filehandle);
> >>>>>> +
> >>>>>> + slog("guest-file-close called, filehandle: %ld", filehandle);
> >>>>>> + if (!gfh) {
> >>>>>> + error_set(err, QERR_FD_NOT_FOUND, "filehandle");
> >>>>>> + return;
> >>>>>> + }
> >>>>>> +
> >>>>>> + fclose(gfh->fh);
> >>>>>> + QTAILQ_REMOVE(&guest_file_state.filehandles, gfh, next);
> >>>>>> + qemu_free(gfh);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void guest_file_init(void)
> >>>>>> +{
> >>>>>> + QTAILQ_INIT(&guest_file_state.filehandles);
> >>>>>> +}
> >>>>>> +
> >>>>>> +typedef struct GuestFsfreezeMount {
> >>>>>> + char *dirname;
> >>>>>> + char *devtype;
> >>>>>> + QTAILQ_ENTRY(GuestFsfreezeMount) next;
> >>>>>> +} GuestFsfreezeMount;
> >>>>>> +
> >>>>>> +struct {
> >>>>>> + GuestFsfreezeStatus status;
> >>>>>> + QTAILQ_HEAD(, GuestFsfreezeMount) mount_list;
> >>>>>> +} guest_fsfreeze_state;
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Walk the mount table and build a list of local file systems
> >>>>>> + */
> >>>>>> +static int guest_fsfreeze_build_mount_list(void)
> >>>>>> +{
> >>>>>> + struct mntent *ment;
> >>>>>> + GuestFsfreezeMount *mount, *temp;
> >>>>>> + char const *mtab = MOUNTED;
> >>>>>> + FILE *fp;
> >>>>>> +
> >>>>>> + fp = setmntent(mtab, "r");
> >>>>>> + if (!fp) {
> >>>>>> + g_warning("fsfreeze: unable to read mtab");
> >>>>>> + goto fail;
> >>>>>> + }
> >>>>>> +
> >>>>>> + while ((ment = getmntent(fp))) {
> >>>>>> + /*
> >>>>>> + * An entry which device name doesn't start with a '/' is
> >>>>>> + * either a dummy file system or a network file system.
> >>>>>> + * Add special handling for smbfs and cifs as is done by
> >>>>>> + * coreutils as well.
> >>>>>> + */
> >>>>>> + if ((ment->mnt_fsname[0] != '/') ||
> >>>>>> + (strcmp(ment->mnt_type, "smbfs") == 0) ||
> >>>>>> + (strcmp(ment->mnt_type, "cifs") == 0)) {
> >>>>>> + continue;
> >>>>>> + }
> >>>>>> +
> >>>>>> + mount = qemu_mallocz(sizeof(GuestFsfreezeMount));
> >>>>>> + mount->dirname = qemu_strdup(ment->mnt_dir);
> >>>>>> + mount->devtype = qemu_strdup(ment->mnt_type);
> >>>>>> +
> >>>>>> + QTAILQ_INSERT_TAIL(&guest_fsfreeze_state.mount_list, mount,
> >>>>>> next);
> >>>>>> + }
> >>>>>> +
> >>>>>> + endmntent(fp);
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +
> >>>>>> +fail:
> >>>>>> + QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next,
> >>>>>> temp) {
> >>>>>> + QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >>>>>> + qemu_free(mount->dirname);
> >>>>>> + qemu_free(mount->devtype);
> >>>>>> + qemu_free(mount);
> >>>>>> + }
> >>>>>
> >>>>> This doesn't seem to be used.
> >>>>>
> >>>>
> >>>> It can get used even a 2nd invocation of this function gets called that
> >>>> results in a goto fail. But looking again this should be done
> >>>> unconditionally at the start of the function, since the mount list is
> >>>> part of global state now.
> >>>>
> >>>>>> +
> >>>>>> + return -1;
> >>>>>> +}
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Return status of freeze/thaw
> >>>>>> + */
> >>>>>> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **err)
> >>>>>> +{
> >>>>>> + return guest_fsfreeze_state.status;
> >>>>>> +}
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Walk list of mounted file systems in the guest, and freeze the
> >>>>>> ones which
> >>>>>> + * are real local file systems.
> >>>>>> + */
> >>>>>> +int64_t qmp_guest_fsfreeze_freeze(Error **err)
> >>>>>> +{
> >>>>>> + int ret = 0, i = 0;
> >>>>>> + struct GuestFsfreezeMount *mount, *temp;
> >>>>>> + int fd;
> >>>>>> +
> >>>>>> + slog("guest-fsfreeze called");
> >>>>>> +
> >>>>>> + if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_THAWED) {
> >>>>>
> >>>>> return 0;
> >>>>>
> >>>>>> + ret = 0;
> >>>>>> + goto out;
> >>>>>> + }
> >>>>>> +
> >>>>>> + ret = guest_fsfreeze_build_mount_list();
> >>>>>> + if (ret< 0) {
> >>>>>
> >>>>> return ret;
> >>>>>
> >>>>>> + goto out;
> >>>>>> + }
> >>>>>> +
> >>>>>> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_INPROGRESS;
> >>>>>> +
> >>>>>> + /* cannot risk guest agent blocking itself on a write in this
> >>>>>> state */
> >>>>>> + disable_logging();
> >>>>>> +
> >>>>>> + QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next,
> >>>>>> temp) {
> >>>>>> + fd = qemu_open(mount->dirname, O_RDONLY);
> >>>>>> + if (fd == -1) {
> >>>>>> + ret = errno;
> >>>>>> + goto error;
> >>>>>> + }
> >>>>>> +
> >>>>>> + /* we try to cull filesytems we know won't work in advance,
> >>>>>> but other
> >>>>>> + * filesytems may not implement fsfreeze for less obvious
> >>>>>> reasons.
> >>>>>> + * these will reason EOPNOTSUPP, so we simply ignore them.
> >>>>>> when
> >>>>>> + * thawing, these filesystems will return an EINVAL instead,
> >>>>>> due to
> >>>>>> + * not being in a frozen state. Other filesystem-specific
> >>>>>> + * errors may result in EINVAL, however, so the user should
> >>>>>> check the
> >>>>>> + * number * of filesystems returned here against those
> >>>>>> returned by the
> >>>>>> + * thaw operation to determine whether everything completed
> >>>>>> + * successfully
> >>>>>> + */
> >>>>>> + ret = ioctl(fd, FIFREEZE);
> >>>>>> + if (ret< 0&& errno != EOPNOTSUPP) {
> >>>>>> + close(fd);
> >>>>>> + goto error;
> >>>>>> + }
> >>>>>> + close(fd);
> >>>>>> +
> >>>>>> + i++;
> >>>>>> + }
> >>>>>> +
> >>>>>> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_FROZEN;
> >>>>>> + ret = i;
> >>>>>> +out:
> >>>>>> + return ret;
> >>>>>> +error:
> >>>>>> + if (i> 0) {
> >>>>>> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_ERROR;
> >>>>>> + }
> >>>>>
> >>>>> Shouldn't you undo everything that has been done so far? Which is
> >>>>> freeing the build list and thawing the file-systems that were frozen
> >>>>> before the error?
> >>>>>
> >>>>
> >>>> We can...the way it's done right now is you get a count of how many
> >>>> filesystems were frozen, along an error status. Depending on the
> >>>> error/count the user can either ignore the error, do what it is they
> >>>> want to do, then call thaw(), or just immediately call thaw().
> >>>
> >>> But you don't get the count on error, so it's difficult (if possible) to
> >>> learn how many file-systems were frozen.
> >>>
> >>
> >> Yah, my mistake. I think the out: label was one line higher, but if we
> >> did that we lose error information. Automatically unfreezing should be
> >> okay.
> >>
> >>>>
> >>>> So we can do an automatic thaw() on their behalf, but i figured the
> >>>> status was good enough.
> >>>>
> >>>>>> + goto out;
> >>>>>> +}
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Walk list of frozen file systems in the guest, and thaw them.
> >>>>>> + */
> >>>>>> +int64_t qmp_guest_fsfreeze_thaw(Error **err)
> >>>>>> +{
> >>>>>> + int ret;
> >>>>>> + GuestFsfreezeMount *mount, *temp;
> >>>>>> + int fd, i = 0;
> >>>>>> +
> >>>>>> + if (guest_fsfreeze_state.status != GUEST_FSFREEZE_STATUS_FROZEN&&
> >>>>>> + guest_fsfreeze_state.status !=
> >>>>>> GUEST_FSFREEZE_STATUS_INPROGRESS) {
> >>>>>
> >>>>> I don't follow why we're checking against INPROGRESS here.
> >>>>>
> >>>>
> >>>> To prevent a race I believe...but we're synchronous now so that's
> >>>> probably no longer needed. I'll look it over and remove it if that's the
> >>>> case.
> >>>>
> >>>>>> + ret = 0;
> >>>>>> + goto out_enable_logging;
> >>>>>> + }
> >>>>>> +
> >>>>>> + QTAILQ_FOREACH_SAFE(mount,&guest_fsfreeze_state.mount_list, next,
> >>>>>> temp) {
> >>>>>> + fd = qemu_open(mount->dirname, O_RDONLY);
> >>>>>> + if (fd == -1) {
> >>>>>> + ret = -errno;
> >>>>>> + goto out;
> >>>>>> + }
> >>>>>> + ret = ioctl(fd, FITHAW);
> >>>>>> + if (ret< 0&& errno != EOPNOTSUPP&& errno != EINVAL) {
> >>>>>> + ret = -errno;
> >>>>>> + close(fd);
> >>>>>> + goto out;
> >>>>>
> >>>>> Shouldn't you continue and try to thaw the other file-systems in the
> >>>>> list?
> >>>>>
> >>>>
> >>>> That's probably better
> >>>>
> >>>>>> + }
> >>>>>> + close(fd);
> >>>>>> +
> >>>>>> + QTAILQ_REMOVE(&guest_fsfreeze_state.mount_list, mount, next);
> >>>>>> + qemu_free(mount->dirname);
> >>>>>> + qemu_free(mount->devtype);
> >>>>>> + qemu_free(mount);
> >>>>>> + i++;
> >>>>>> + }
> >>>>>> +
> >>>>>> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >>>>>> + ret = i;
> >>>>>> +out_enable_logging:
> >>>>>> + enable_logging();
> >>>>>> +out:
> >>>>>> + return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void guest_fsfreeze_init(void)
> >>>>>> +{
> >>>>>> + guest_fsfreeze_state.status = GUEST_FSFREEZE_STATUS_THAWED;
> >>>>>> + QTAILQ_INIT(&guest_fsfreeze_state.mount_list);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void guest_fsfreeze_cleanup(void)
> >>>>>> +{
> >>>>>> + int64_t ret;
> >>>>>> + Error *err = NULL;
> >>>>>> +
> >>>>>> + if (guest_fsfreeze_state.status == GUEST_FSFREEZE_STATUS_FROZEN) {
> >>>>>> + ret = qmp_guest_fsfreeze_thaw(&err);
> >>>>>> + if (ret< 0 || err) {
> >>>>>> + slog("failed to clean up frozen filesystems");
> >>>>>> + }
> >>>>>> + }
> >>>>>> +}
> >>>>>> +
> >>>>>> +/* register init/cleanup routines for stateful command groups */
> >>>>>> +void ga_command_state_init(GAState *s, GACommandState *cs)
> >>>>>> +{
> >>>>>> + ga_state = s;
> >>>>>> + ga_command_state_add(cs, guest_fsfreeze_init,
> >>>>>> guest_fsfreeze_cleanup);
> >>>>>> + ga_command_state_add(cs, guest_file_init, NULL);
> >>>>>> +}
> >>>>>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >>>>>> index 66d1729..3501ff4 100644
> >>>>>> --- a/qga/guest-agent-core.h
> >>>>>> +++ b/qga/guest-agent-core.h
> >>>>>> @@ -14,10 +14,12 @@
> >>>>>> #include "qemu-common.h"
> >>>>>>
> >>>>>> #define QGA_VERSION "1.0"
> >>>>>> +#define QGA_READ_LIMIT 4<< 20 /* 4MB block size max for chunked
> >>>>>> reads */
> >>>>>>
> >>>>>> typedef struct GAState GAState;
> >>>>>> typedef struct GACommandState GACommandState;
> >>>>>>
> >>>>>> +void ga_command_state_init(GAState *s, GACommandState *cs);
> >>>>>> void ga_command_state_add(GACommandState *cs,
> >>>>>> void (*init)(void),
> >>>>>> void (*cleanup)(void));
> >>>>>
> >>>>
> >>>
> >>
> >
>
