[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 3/5] guest agent: add guest agent RPCs/comman
|
From: |
Michael Roth |
|
Subject: |
Re: [Qemu-devel] [PATCH v5 3/5] guest agent: add guest agent RPCs/commands |
|
Date: |
Fri, 17 Jun 2011 15:19:56 -0500 |
|
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.17) Gecko/20110414 Thunderbird/3.1.10 |
On 06/16/2011 01:52 PM, Luiz Capitulino wrote:
On Tue, 14 Jun 2011 15:06:23 -0500
Michael Roth<address@hidden> wrote:
This adds the initial set of QMP/QAPI commands provided by the guest
agent:
guest-sync
guest-ping
guest-info
guest-shutdown
guest-file-open
guest-file-read
guest-file-write
guest-file-seek
guest-file-close
guest-fsfreeze-freeze
guest-fsfreeze-thaw
guest-fsfreeze-status
The input/output specification for these commands are documented in the
schema.
Signed-off-by: Michael Roth<address@hidden>
---
qerror.c | 4 +
qerror.h | 3 +
qga/guest-agent-commands.c | 522 ++++++++++++++++++++++++++++++++++++++++++++
qga/guest-agent-core.h | 1 +
4 files changed, 530 insertions(+), 0 deletions(-)
create mode 100644 qga/guest-agent-commands.c
diff --git a/qerror.c b/qerror.c
index d7fcd93..24f0c48 100644
--- a/qerror.c
+++ b/qerror.c
@@ -213,6 +213,10 @@ static const QErrorStringTable qerror_table[] = {
.error_fmt = QERR_VNC_SERVER_FAILED,
.desc = "Could not start VNC server on %(target)",
},
+ {
+ .error_fmt = QERR_QGA_LOGGING_FAILED,
+ .desc = "failed to write log statement due to logging being
disabled",
+ },
{}
};
diff --git a/qerror.h b/qerror.h
index 7a89a50..bf3d5a9 100644
--- a/qerror.h
+++ b/qerror.h
@@ -184,4 +184,7 @@ QError *qobject_to_qerror(const QObject *obj);
#define QERR_FEATURE_DISABLED \
"{ 'class': 'FeatureDisabled', 'data': { 'name': %s } }"
+#define QERR_QGA_LOGGING_FAILED \
+ "{ 'class': 'QgaLoggingFailed', 'data': {} }"
+
#endif /* QERROR_H */
diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
new file mode 100644
index 0000000..6f9886a
--- /dev/null
+++ b/qga/guest-agent-commands.c
@@ -0,0 +1,522 @@
+/*
+ * QEMU Guest Agent commands
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ * Michael Roth<address@hidden>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include<glib.h>
+#include<mntent.h>
+#include<sys/types.h>
+#include<sys/ioctl.h>
+#include<linux/fs.h>
+#include "qga/guest-agent-core.h"
+#include "qga-qmp-commands.h"
+#include "qerror.h"
+
+static GAState *ga_state;
+
+static bool logging_enabled(void)
+{
+ return ga_logging_enabled(ga_state);
+}
+
+static void disable_logging(void)
+{
+ ga_disable_logging(ga_state);
+}
+
+static void enable_logging(void)
+{
+ ga_enable_logging(ga_state);
+}
+
+/* Note: in some situations, like with the fsfreeze, logging may be
+ * temporarilly disabled. if it is necessary that a command be able
+ * to log for accounting purposes, check logging_enabled() beforehand,
+ * and use the QERR_QGA_LOGGING_DISABLED to generate an error
+ */
+static void slog(const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ g_logv("syslog", G_LOG_LEVEL_INFO, fmt, ap);
+ va_end(ap);
+}
+
+int64_t qmp_guest_sync(int64_t id, Error **errp)
+{
+ return id;
+}
+
+void qmp_guest_ping(Error **err)
+{
+ slog("guest-ping called");
+}
+
+struct GuestAgentInfo *qmp_guest_info(Error **err)
+{
+ GuestAgentInfo *info = g_malloc0(sizeof(GuestAgentInfo));
+
+ info->version = g_strdup(QGA_VERSION);
+
+ return info;
+}
+
+void qmp_guest_shutdown(const char *shutdown_mode, Error **err)
I'd call the argument just 'mode'.
+{
+ int ret;
+ const char *shutdown_flag;
+
+ if (!logging_enabled()) {
+ error_set(err, QERR_QGA_LOGGING_FAILED);
+ return;
+ }
+
+ slog("guest-shutdown called, shutdown_mode: %s", shutdown_mode);
+ if (strcmp(shutdown_mode, "halt") == 0) {
+ shutdown_flag = "-H";
+ } else if (strcmp(shutdown_mode, "powerdown") == 0) {
+ shutdown_flag = "-P";
+ } else if (strcmp(shutdown_mode, "reboot") == 0) {
+ shutdown_flag = "-r";
+ } else {
+ error_set(err, QERR_INVALID_PARAMETER_VALUE, "shutdown_mode",
+ "halt|powerdown|reboot");
+ return;
+ }
+
+ ret = fork();
+ if (ret == 0) {
+ /* child, start the shutdown */
+ setsid();
+ fclose(stdin);
+ fclose(stdout);
+ fclose(stderr);
Would be nice to have a log file, whose descriptor is passed to the
child.
+
+ sleep(5);
Why sleep()?
Want to give the agent time to send a response. It's still racy, but
less so that immediately issuing the shutdown. 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));
- Re: [Qemu-devel] [PATCH v5 2/5] guest agent: qemu-ga daemon, (continued)
[Qemu-devel] [PATCH v5 1/5] guest agent: command state class, Michael Roth, 2011/06/14
[Qemu-devel] [PATCH v5 3/5] guest agent: add guest agent RPCs/commands, Michael Roth, 2011/06/14
[Qemu-devel] [PATCH v5 4/5] guest agent: add guest agent commands schema file, Michael Roth, 2011/06/14
[Qemu-devel] [PATCH v5 5/5] guest agent: Makefile, build qemu-ga, Michael Roth, 2011/06/14