From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38846) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QXfWj-0000sA-S4 for qemu-devel@nongnu.org; Fri, 17 Jun 2011 16:20:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QXfWd-0001Qv-QR for qemu-devel@nongnu.org; Fri, 17 Jun 2011 16:20:24 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:43099) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QXfWd-0001D6-3y for qemu-devel@nongnu.org; Fri, 17 Jun 2011 16:20:19 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by e3.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p5HJvJbu026293 for ; Fri, 17 Jun 2011 15:57:19 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p5HKJwtX130166 for ; Fri, 17 Jun 2011 16:19:58 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p5HKJvxU004770 for ; Fri, 17 Jun 2011 17:19:58 -0300 Message-ID: <4DFBB6EC.70805@linux.vnet.ibm.com> Date: Fri, 17 Jun 2011 15:19:56 -0500 From: Michael Roth MIME-Version: 1.0 References: <1308081985-32394-1-git-send-email-mdroth@linux.vnet.ibm.com> <1308081985-32394-4-git-send-email-mdroth@linux.vnet.ibm.com> <20110616155247.1df00b53@doriath> In-Reply-To: <20110616155247.1df00b53@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 3/5] guest agent: add guest agent RPCs/commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: aliguori@linux.vnet.ibm.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org, Jes.Sorensen@redhat.com On 06/16/2011 01:52 PM, Luiz Capitulino wrote: > On Tue, 14 Jun 2011 15:06:23 -0500 > Michael Roth 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 >> --- >> 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 >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#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)); >