We already have a precedent of function documentation in C files, where it is closer to the implementation (witness libvirt.h vs. libvirt.c); maintaining docs in both files risks docs going stale. While I was at it, I used consistent doxygen style on all comments. * src/util/command.h: Remove duplicate docs, and move unique documentation... * src/util/command.c: ...here. Suggested by Matthias Bolte. --- prereq patch to 2/16 (not directly related to bypass-cache, but I don't want to rebase across this one) v2: no change from v1, previously posted here: https://www.redhat.com/archives/libvir-list/2011-July/msg00879.html src/util/command.c | 371 ++++++++++++++++++++++++++++++++++++++++------------ src/util/command.h | 223 ++------------------------------ 2 files changed, 299 insertions(+), 295 deletions(-) diff --git a/src/util/command.c b/src/util/command.c index 5bc6272..11dd7b0 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -130,23 +130,25 @@ static int virClearCapabilities(void) # endif -/* virFork() - fork a new process while avoiding various race/deadlock - conditions - - @pid - a pointer to a pid_t that will receive the return value from - fork() - - on return from virFork(), if *pid < 0, the fork failed and there is - no new process. Otherwise, just like fork(), if *pid == 0, it is the - child process returning, and if *pid > 0, it is the parent. - - Even if *pid >= 0, if the return value from virFork() is < 0, it - indicates a failure that occurred in the parent or child process - after the fork. In this case, the child process should call - _exit(EXIT_FAILURE) after doing any additional error reporting. - +/** + * virFork: + * @pid - a pointer to a pid_t that will receive the return value from + * fork() + * + * fork a new process while avoiding various race/deadlock conditions + * + * on return from virFork(), if *pid < 0, the fork failed and there is + * no new process. Otherwise, just like fork(), if *pid == 0, it is the + * child process returning, and if *pid > 0, it is the parent. + * + * Even if *pid >= 0, if the return value from virFork() is < 0, it + * indicates a failure that occurred in the parent or child process + * after the fork. In this case, the child process should call + * _exit(EXIT_FAILURE) after doing any additional error reporting. */ -int virFork(pid_t *pid) { +int +virFork(pid_t *pid) +{ sigset_t oldmask, newmask; struct sigaction sig_action; int saved_errno, ret = -1; @@ -573,6 +575,7 @@ virExecWithHook(const char *const*argv, } /** + * virRun: * @argv NULL terminated argv to run * @status optional variable to return exit status in * @@ -645,8 +648,13 @@ virFork(pid_t *pid) #endif /* WIN32 */ -/* - * Create a new command for named binary +/** + * virCommandNew: + * @binary: program to run + * + * Create a new command for named binary. If @binary is relative, + * it will be found via a PATH search of the parent's PATH (and not + * any altered PATH set by virCommandAddEnv* commands). */ virCommandPtr virCommandNew(const char *binary) @@ -656,9 +664,13 @@ virCommandNew(const char *binary) return virCommandNewArgs(args); } -/* +/** + * virCommandNewArgs: + * @args: array of arguments + * * Create a new command with a NULL terminated - * set of args, taking binary from args[0] + * set of args, taking binary from args[0]. More arguments can + * be added later. @args[0] is handled like @binary of virCommandNew. */ virCommandPtr virCommandNewArgs(const char *const*args) @@ -684,9 +696,14 @@ virCommandNewArgs(const char *const*args) return cmd; } -/* +/** + * virCommandNewArgList: + * @binary: program to run + * @...: additional arguments + * * Create a new command with a NULL terminated - * list of args, starting with the binary to run + * list of args, starting with the binary to run. More arguments can + * be added later. @binary is handled as in virCommandNew. */ virCommandPtr virCommandNewArgList(const char *binary, ...) @@ -730,9 +747,13 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) FD_SET(fd, &cmd->transfer); } -/* +/** + * virCommandPreserveFD: + * @cmd: the command to modify + * @fd: fd to mark for inheritance into child + * * Preserve the specified file descriptor - * in the child, instead of closing it. + * in the child, instead of closing it on exec. * The parent is still responsible for managing fd. */ void @@ -741,9 +762,13 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) return virCommandKeepFD(cmd, fd, false); } -/* +/** + * virCommandTransferFD: + * @cmd: the command to modify + * @fd: fd to reassign to the child + * * Transfer the specified file descriptor - * to the child, instead of closing it. + * to the child, instead of closing it on exec. * Close the fd in the parent during Run/RunAsync/Free. */ void @@ -753,8 +778,13 @@ virCommandTransferFD(virCommandPtr cmd, int fd) } -/* - * Save the child PID in a pidfile +/** + * virCommandSetPidFile: + * @cmd: the command to modify + * @pidfile: filename to use + * + * Save the child PID in a pidfile. The pidfile will be populated + * before the exec of the child. */ void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) @@ -769,8 +799,11 @@ virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) } -/* - * Remove all capabilities from the child +/** + * virCommandClearCaps: + * @cmd: the command to modify + * + * Remove all capabilities from the child, after any hooks have been run. */ void virCommandClearCaps(virCommandPtr cmd) @@ -783,7 +816,11 @@ virCommandClearCaps(virCommandPtr cmd) #if 0 /* XXX Enable if we have a need for capability management. */ -/* +/** + * virCommandAllowCap: + * @cmd: the command to modify + * @capability: what to allow + * * Re-allow a specific capability */ void @@ -799,8 +836,13 @@ virCommandAllowCap(virCommandPtr cmd, #endif /* 0 */ -/* - * Daemonize the child process +/** + * virCommandDaemonize: + * @cmd: the command to modify + * + * Daemonize the child process. The child will have a current working + * directory of /, and must be started with virCommandRun, which will + * complete as soon as the daemon grandchild has started. */ void virCommandDaemonize(virCommandPtr cmd) @@ -811,7 +853,10 @@ virCommandDaemonize(virCommandPtr cmd) cmd->flags |= VIR_EXEC_DAEMON; } -/* +/** + * virCommandNonblockingFDs: + * @cmd: the command to modify + * * Set FDs created by virCommandSetOutputFD and virCommandSetErrorFD * as non-blocking in the parent. */ @@ -824,8 +869,13 @@ virCommandNonblockingFDs(virCommandPtr cmd) cmd->flags |= VIR_EXEC_NONBLOCK; } -/* - * Add an environment variable to the child created by a printf-style format +/** + * virCommandAddEnvFormat: + * @cmd: the command to modify + * @format: format of arguments, end result must be in name=value format + * @...: arguments to be formatted + * + * Add an environment variable to the child created by a printf-style format. */ void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) @@ -854,7 +904,12 @@ virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) cmd->env[cmd->nenv++] = env; } -/* +/** + * virCommandAddEnvPair: + * @cmd: the command to modify + * @name: variable name, must not contain = + * @value: value to assign to name + * * Add an environment variable to the child * using separate name & value strings */ @@ -865,7 +920,11 @@ virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) } -/* +/** + * virCommandAddEnvString: + * @cmd: the command to modify + * @str: name=value format + * * Add an environment variable to the child * using a preformatted env string FOO=BAR */ @@ -893,7 +952,11 @@ virCommandAddEnvString(virCommandPtr cmd, const char *str) } -/* +/** + * virCommandAddEnvBuffer: + * @cmd: the command to modify + * @buf: buffer that contains name=value string, which will be reset on return + * * Convert a buffer containing preformatted name=value into an * environment variable of the child. * Correctly transfers memory errors or contents from buf to cmd. @@ -918,7 +981,11 @@ virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) } -/* +/** + * virCommandAddEnvPass: + * @cmd: the command to modify + * @name: the name to look up in current environment + * * Pass an environment variable to the child * using current process' value */ @@ -935,9 +1002,12 @@ virCommandAddEnvPass(virCommandPtr cmd, const char *name) } -/* +/** + * virCommandAddEnvPassCommon: + * @cmd: the command to modify + * * Set LC_ALL to C, and propagate other essential environment - * variables from the parent process. + * variables (such as PATH) from the parent process. */ void virCommandAddEnvPassCommon(virCommandPtr cmd) @@ -960,7 +1030,11 @@ virCommandAddEnvPassCommon(virCommandPtr cmd) virCommandAddEnvPass(cmd, "TMPDIR"); } -/* +/** + * virCommandAddArg: + * @cmd: the command to modify + * @val: the argument to add + * * Add a command line argument to the child */ void @@ -987,7 +1061,11 @@ virCommandAddArg(virCommandPtr cmd, const char *val) } -/* +/** + * virCommandAddArgBuffer: + * @cmd: the command to modify + * @buf: buffer that contains argument string, which will be reset on return + * * Convert a buffer into a command line argument to the child. * Correctly transfers memory errors or contents from buf to cmd. */ @@ -1011,8 +1089,13 @@ virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf) } -/* - * Add a command line argument created by a printf-style format +/** + * virCommandAddArgFormat: + * @cmd: the command to modify + * @format: format of arguments, end result must be in name=value format + * @...: arguments to be formatted + * + * Add a command line argument created by a printf-style format. */ void virCommandAddArgFormat(virCommandPtr cmd, const char *format, ...) @@ -1041,7 +1124,12 @@ virCommandAddArgFormat(virCommandPtr cmd, const char *format, ...) cmd->args[cmd->nargs++] = arg; } -/* +/** + * virCommandAddArgPair: + * @cmd: the command to modify + * @name: left half of argument + * @value: right half of argument + * * Add "NAME=VAL" as a single command line argument to the child */ void @@ -1050,7 +1138,11 @@ virCommandAddArgPair(virCommandPtr cmd, const char *name, const char *val) virCommandAddArgFormat(cmd, "%s=%s", name, val); } -/* +/** + * virCommandAddArgSet: + * @cmd: the command to modify + * @vals: array of arguments to add + * * Add a NULL terminated list of args */ void @@ -1086,8 +1178,12 @@ virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) } } -/* - * Add a NULL terminated list of args +/** + * virCommandAddArgList: + * @cmd: the command to modify + * @...: list of arguments to add + * + * Add a NULL terminated list of args. */ void virCommandAddArgList(virCommandPtr cmd, ...) @@ -1125,7 +1221,11 @@ virCommandAddArgList(virCommandPtr cmd, ...) va_end(list); } -/* +/** + * virCommandSetWorkingDirectory: + * @cmd: the command to modify + * @pwd: directory to use + * * Set the working directory of a non-daemon child process, rather * than the parent's working directory. Daemons automatically get / * without using this call. @@ -1147,8 +1247,13 @@ virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd) } -/* - * Feed the child's stdin from a string buffer +/** + * virCommandSetInputBuffer: + * @cmd: the command to modify + * @inbuf: string to feed to stdin + * + * Feed the child's stdin from a string buffer. This requires the use + * of virCommandRun(). */ void virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) @@ -1168,11 +1273,16 @@ virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) } -/* +/** + * virCommandSetOutputBuffer: + * @cmd: the command to modify + * @outbuf: address of variable to store malloced result buffer + * * Capture the child's stdout to a string buffer. *outbuf is * guaranteed to be allocated after successful virCommandRun or * virCommandWait, and is best-effort allocated after failed * virCommandRun; caller is responsible for freeing *outbuf. + * This requires the use of virCommandRun. */ void virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) @@ -1192,11 +1302,16 @@ virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) } -/* +/** + * virCommandSetErrorBuffer: + * @cmd: the command to modify + * @errbuf: address of variable to store malloced result buffer + * * Capture the child's stderr to a string buffer. *errbuf is * guaranteed to be allocated after successful virCommandRun or * virCommandWait, and is best-effort allocated after failed * virCommandRun; caller is responsible for freeing *errbuf. + * This requires the use of virCommandRun. */ void virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) @@ -1216,7 +1331,11 @@ virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) } -/* +/** + * virCommandSetInputFD: + * @cmd: the command to modify + * @infd: the descriptor to use + * * Attach a file descriptor to the child's stdin */ void @@ -1240,8 +1359,14 @@ virCommandSetInputFD(virCommandPtr cmd, int infd) } -/* - * Attach a file descriptor to the child's stdout +/** + * virCommandSetOutputFD: + * @cmd: the command to modify + * @outfd: location of output fd + * + * Attach a file descriptor to the child's stdout. If *@outfd is -1 on + * entry, then a pipe will be created and returned in this variable when + * the child is run. Otherwise, *@outfd is used as the output. */ void virCommandSetOutputFD(virCommandPtr cmd, int *outfd) @@ -1259,8 +1384,15 @@ virCommandSetOutputFD(virCommandPtr cmd, int *outfd) } -/* - * Attach a file descriptor to the child's stderr +/** + * virCommandSetErrorFD: + * @cmd: the command to modify + * @errfd: location of error fd + * + * Attach a file descriptor to the child's stderr. If *@errfd is -1 on + * entry, then a pipe will be created and returned in this variable when + * the child is run. Otherwise, *@errfd is used for error collection, + * and may be the same as outfd given to virCommandSetOutputFD(). */ void virCommandSetErrorFD(virCommandPtr cmd, int *errfd) @@ -1278,10 +1410,18 @@ virCommandSetErrorFD(virCommandPtr cmd, int *errfd) } -/* +/** + * virCommandSetPreExecHook: + * @cmd: the command to modify + * @hook: the hook to run + * @opaque: argument to pass to the hook + * * Run HOOK(OPAQUE) in the child as the last thing before changing * directories, dropping capabilities, and executing the new process. * Force the child to fail if HOOK does not return zero. + * + * Since @hook runs in the child, it should be careful to avoid + * any functions that are not async-signal-safe. */ void virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) @@ -1299,7 +1439,11 @@ virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) } -/* +/** + * virCommandWriteArgLog: + * @cmd: the command to log + * @logfd: where to log the results + * * Call after adding all arguments and environment settings, but before * Run/RunAsync, to immediately output the environment and arguments of * cmd to logfd. If virCommandRun cannot succeed (because of an @@ -1337,7 +1481,10 @@ virCommandWriteArgLog(virCommandPtr cmd, int logfd) } -/* +/** + * virCommandToString: + * @cmd: the command to convert + * * Call after adding all arguments and environment settings, but before * Run/RunAsync, to return a string representation of the environment and * arguments of cmd. If virCommandRun cannot succeed (because of an @@ -1382,10 +1529,13 @@ virCommandToString(virCommandPtr cmd) } -/* +/** + * virCommandTranslateStatus: + * @status: child exit status to translate + * * Translate an exit status into a malloc'd string. Generic helper - * for virCommandRun, virCommandWait, and virPidWait status argument, - * as well as raw waitpid and older virRun status. + * for virCommandRun(), virCommandWait(), virRun(), and virPidWait() + * status argument, as well as raw waitpid(). */ char * virCommandTranslateStatus(int status) @@ -1551,10 +1701,13 @@ cleanup: return ret; } -/* +/** + * virCommandExec: + * @cmd: command to run + * * Exec the command, replacing the current process. Meant to be called - * after already forking / cloning, so does not attempt to daemonize or - * preserve any FDs. + * in the hook after already forking / cloning, so does not attempt to + * daemonize or preserve any FDs. * * Returns -1 on any error executing the command. * Will not return on success. @@ -1587,11 +1740,16 @@ int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED) } #endif -/* +/** + * virCommandRun: + * @cmd: command to run + * @exitstatus: optional status collection + * * Run the command and wait for completion. * Returns -1 on any error executing the * command. Returns 0 if the command executed, - * with the exit status set + * with the exit status set. If @exitstatus is NULL, then the + * child must exit with status 0 for this to succeed. */ int virCommandRun(virCommandPtr cmd, int *exitstatus) @@ -1803,7 +1961,11 @@ virCommandHook(void *data) } -/* +/** + * virCommandRunAsync: + * @cmd: command to start + * @pid: optional variable to track child pid + * * Run the command asynchronously * Returns -1 on any error executing the * command. Returns 0 if the command executed. @@ -1909,11 +2071,16 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) } -/* +/** + * virPidWait: + * @pid: child to wait on + * @exitstatus: optional status collection + * * Wait for a child process to complete. * Return -1 on any error waiting for * completion. Returns 0 if the command - * finished with the exit status set + * finished with the exit status set. If @exitstatus is NULL, then the + * child must exit with status 0 for this to succeed. */ int virPidWait(pid_t pid, int *exitstatus) @@ -1951,11 +2118,16 @@ virPidWait(pid_t pid, int *exitstatus) return 0; } -/* - * Wait for the async command to complete. - * Return -1 on any error waiting for +/** + * virCommandWait: + * @cmd: command to wait on + * @exitstatus: optional status collection + * + * Wait for the command previously started with virCommandRunAsync() + * to complete. Return -1 on any error waiting for * completion. Returns 0 if the command - * finished with the exit status set + * finished with the exit status set. If @exitstatus is NULL, then the + * child must exit with status 0 for this to succeed. */ int virCommandWait(virCommandPtr cmd, int *exitstatus) @@ -2006,11 +2178,15 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) #ifndef WIN32 -/* +/** + * virPidAbort: + * @pid: child process to kill + * * Abort a child process if PID is positive and that child is still * running, without issuing any errors or affecting errno. Designed * for error paths where some but not all paths to the cleanup code - * might have started the child process. + * might have started the child process. If @pid is 0 or negative, + * this does nothing. */ void virPidAbort(pid_t pid) @@ -2063,7 +2239,10 @@ cleanup: errno = saved_errno; } -/* +/** + * virCommandAbort: + * @cmd: command to abort + * * Abort an async command if it is running, without issuing * any errors or affecting errno. Designed for error paths * where some but not all paths to the cleanup code might @@ -2096,6 +2275,15 @@ virCommandAbort(virCommandPtr cmd ATTRIBUTE_UNUSED) #endif +/** + * virCommandRequireHandshake: + * @cmd: command to modify + * + * Request that the child perform a handshake with + * the parent when the hook function has completed + * execution. The child will not exec() until the + * parent has notified + */ void virCommandRequireHandshake(virCommandPtr cmd) { if (!cmd || cmd->has_error) @@ -2125,6 +2313,13 @@ void virCommandRequireHandshake(virCommandPtr cmd) cmd->handshake = true; } +/** + * virCommandHandshakeWait: + * @cmd: command to wait on + * + * Wait for the child to complete execution of its + * hook function. To be called in the parent. + */ int virCommandHandshakeWait(virCommandPtr cmd) { char c; @@ -2178,6 +2373,13 @@ int virCommandHandshakeWait(virCommandPtr cmd) return 0; } +/** + * virCommandHandshakeNotify: + * @cmd: command to resume + * + * Notify the child that it is OK to exec() the + * real binary now. To be called in the parent. + */ int virCommandHandshakeNotify(virCommandPtr cmd) { char c = '1'; @@ -2208,10 +2410,13 @@ int virCommandHandshakeNotify(virCommandPtr cmd) } -/* +/** + * virCommandFree: + * @cmd: optional command to free + * * Release all resources. The only exception is that if you called * virCommandRunAsync with a non-null pid, then the asynchronous child - * is not reaped, and you must call virPidWait() yourself. + * is not reaped, and you must call virPidWait() or virPidAbort() yourself. */ void virCommandFree(virCommandPtr cmd) diff --git a/src/util/command.h b/src/util/command.h index f766839..1386d57 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -30,34 +30,18 @@ typedef struct _virCommand virCommand; typedef virCommand *virCommandPtr; /* This will execute in the context of the first child - * after fork() but before execve() */ + * after fork() but before execve(). As such, it is unsafe to + * call any function that is not async-signal-safe. */ typedef int (*virExecHook)(void *data); -/* - * Fork wrapper with extra error checking - */ int virFork(pid_t *pid) ATTRIBUTE_RETURN_CHECK; -/* - * Simple synchronous command wrapper - */ int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; -/* - * Create a new command for named binary - */ virCommandPtr virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1); -/* - * Create a new command with a NULL terminated - * set of args, taking binary from argv[0] - */ virCommandPtr virCommandNewArgs(const char *const*args) ATTRIBUTE_NONNULL(1); -/* - * Create a new command with a NULL terminated - * list of args, starting with the binary to run - */ virCommandPtr virCommandNewArgList(const char *binary, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; @@ -65,310 +49,125 @@ virCommandPtr virCommandNewArgList(const char *binary, ...) * delayed until the Run/RunAsync methods */ -/* - * Preserve the specified file descriptor - * in the child, instead of closing it. - * The parent is still responsible for managing fd. - */ void virCommandPreserveFD(virCommandPtr cmd, int fd); -/* - * Transfer the specified file descriptor - * to the child, instead of closing it. - * Close the fd in the parent during Run/RunAsync/Free. - */ void virCommandTransferFD(virCommandPtr cmd, int fd); -/* - * Save the child PID in a pidfile - */ void virCommandSetPidFile(virCommandPtr cmd, const char *pidfile) ATTRIBUTE_NONNULL(2); -/* - * Remove all capabilities from the child - */ void virCommandClearCaps(virCommandPtr cmd); # if 0 -/* - * Re-allow a specific capability - */ void virCommandAllowCap(virCommandPtr cmd, int capability); # endif -/* - * Daemonize the child process - */ void virCommandDaemonize(virCommandPtr cmd); -/* - * Set FDs created by virCommandSetOutputFD and virCommandSetErrorFD - * as non-blocking in the parent. - */ void virCommandNonblockingFDs(virCommandPtr cmd); -/* - * Add an environment variable to the child created by a printf-style format - */ -void -virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); +void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); -/* - * Add an environment variable to the child - * using separate name & value strings - */ void virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) ATTRIBUTE_NONNULL(2); -/* - * Add an environemnt variable to the child - * using a preformated env string FOO=BAR - */ void virCommandAddEnvString(virCommandPtr cmd, const char *str) ATTRIBUTE_NONNULL(2); -/* - * Convert a buffer containing preformatted name=value into an - * environment variable of the child. - * Correctly transfers memory errors or contents from buf to cmd. - */ void virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf); -/* - * Pass an environment variable to the child - * using current process' value - */ void virCommandAddEnvPass(virCommandPtr cmd, const char *name) ATTRIBUTE_NONNULL(2); -/* - * Pass a common set of environment variables - * to the child using current process' values - */ + void virCommandAddEnvPassCommon(virCommandPtr cmd); -/* - * Add a command line argument to the child - */ void virCommandAddArg(virCommandPtr cmd, const char *val) ATTRIBUTE_NONNULL(2); -/* - * Convert a buffer into a command line argument to the child. - * Correctly transfers memory errors or contents from buf to cmd. - */ void virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf); -/* - * Add a command line argument created by a printf-style format - */ void virCommandAddArgFormat(virCommandPtr cmd, const char *format, ...) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); -/* - * Add a command line argument to the child - */ void virCommandAddArgPair(virCommandPtr cmd, const char *name, const char *val) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); -/* - * Add a NULL terminated array of args - */ + void virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) ATTRIBUTE_NONNULL(2); -/* - * Add a NULL terminated list of args - */ + void virCommandAddArgList(virCommandPtr cmd, ... /* const char *arg, ..., NULL */) ATTRIBUTE_SENTINEL; -/* - * Set the working directory of a non-daemon child process, rather - * than the parent's working directory. Daemons automatically get / - * without using this call. - */ void virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd) ATTRIBUTE_NONNULL(2); -/* - * Feed the child's stdin from a string buffer. - * - * NB: Only works with virCommandRun() - */ void virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) ATTRIBUTE_NONNULL(2); -/* - * Capture the child's stdout to a string buffer - * - * NB: Only works with virCommandRun() - */ + void virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) ATTRIBUTE_NONNULL(2); -/* - * Capture the child's stderr to a string buffer - * - * NB: Only works with virCommandRun() - */ + void virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) ATTRIBUTE_NONNULL(2); -/* - * Set a file descriptor as the child's stdin - */ void virCommandSetInputFD(virCommandPtr cmd, int infd); -/* - * Set a file descriptor as the child's stdout - */ + void virCommandSetOutputFD(virCommandPtr cmd, int *outfd) ATTRIBUTE_NONNULL(2); -/* - * Set a file descriptor as the child's stderr - */ + void virCommandSetErrorFD(virCommandPtr cmd, int *errfd) ATTRIBUTE_NONNULL(2); -/* - * A hook function to run between fork + exec - */ void virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) ATTRIBUTE_NONNULL(2); -/* - * Call after adding all arguments and environment settings, but before - * Run/RunAsync, to immediately output the environment and arguments of - * cmd to logfd. If virCommandRun cannot succeed (because of an - * out-of-memory condition while building cmd), nothing will be logged. - */ void virCommandWriteArgLog(virCommandPtr cmd, int logfd); -/* - * Call after adding all arguments and environment settings, but before - * Run/RunAsync, to return a string representation of the environment and - * arguments of cmd. If virCommandRun cannot succeed (because of an - * out-of-memory condition while building cmd), NULL will be returned. - * Caller is responsible for freeing the resulting string. - */ char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -/* - * Translate an exit status into a malloc'd string. - */ char *virCommandTranslateStatus(int exitstatus) ATTRIBUTE_RETURN_CHECK; -/* - * Exec the command, replacing the current process. Meant to be called - * after already forking / cloning, so does not attempt to daemonize or - * preserve any FDs. - * - * Returns -1 on any error executing the command. - * Will not return on success. - */ int virCommandExec(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -/* - * Run the command and wait for completion. - * Returns -1 on any error executing the - * command. Returns 0 if the command executed, - * with the exit status set - */ int virCommandRun(virCommandPtr cmd, int *exitstatus) ATTRIBUTE_RETURN_CHECK; -/* - * Run the command asynchronously - * Returns -1 on any error executing the - * command. Returns 0 if the command executed. - * - * There are two approaches to child process cleanup. - * 1. Use auto-cleanup, by passing NULL for pid. The child will be - * auto-reaped by virCommandFree, unless you reap it earlier via - * virCommandWait or virCommandAbort. Good for where cmd is in - * scope for the duration of the child process. - * 2. Use manual cleanup, by passing the address of a pid_t variable - * for pid. While cmd is still in scope, you may reap the child via - * virCommandWait or virCommandAbort. But after virCommandFree, if - * you have not yet reaped the child, then it continues to run until - * you call virPidWait or virPidAbort. - */ int virCommandRunAsync(virCommandPtr cmd, pid_t *pid) ATTRIBUTE_RETURN_CHECK; -/* - * Wait for a child process to complete. - * Return -1 on any error waiting for - * completion. Returns 0 if the command - * finished with the exit status set. - */ int virPidWait(pid_t pid, int *exitstatus) ATTRIBUTE_RETURN_CHECK; -/* - * Wait for the async command to complete. - * Return -1 on any error waiting for - * completion. Returns 0 if the command - * finished with the exit status set - */ int virCommandWait(virCommandPtr cmd, int *exitstatus) ATTRIBUTE_RETURN_CHECK; -/* - * Request that the child perform a handshake with - * the parent when the hook function has completed - * execution. The child will not exec() until the - * parent has notified - */ void virCommandRequireHandshake(virCommandPtr cmd); -/* - * Wait for the child to complete execution of its - * hook function - */ int virCommandHandshakeWait(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -/* - * Notify the child that it is OK to exec() the - * real binary now - */ int virCommandHandshakeNotify(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -/* - * Abort a child process if PID is positive and that child is still - * running, without issuing any errors or affecting errno. Designed - * for error paths where some but not all paths to the cleanup code - * might have started the child process. - */ void virPidAbort(pid_t pid); -/* - * Abort an async command if it is running, without issuing - * any errors or affecting errno. Designed for error paths - * where some but not all paths to the cleanup code might - * have started the child process. - */ void virCommandAbort(virCommandPtr cmd); -/* - * Release all resources. The only exception is that if you called - * virCommandRunAsync with a non-null pid, then the asynchronous child - * is not reaped, and you must call virPidWait() yourself. - */ void virCommandFree(virCommandPtr cmd); - #endif /* __VIR_COMMAND_H__ */ -- 1.7.4.4

Since libvirt is multi-threaded, we should use FD_CLOEXEC as much as possible in the parent, and only relax fds to inherited after forking, to avoid leaking an fd created in one thread to a fork run in another thread. This gets us closer to that ideal, by making virCommand automatically clear FD_CLOEXEC on fds intended for the child, as well as avoiding a window of time with non-cloexec pipes created for capturing output. * src/util/command.c (virExecWithHook): Use CLOEXEC in parent. In child, guarantee that all fds to pass to child are inheritable. (getDevNull): Use CLOEXEC. (prepareStdFd): New helper function. (virCommandRun, virCommandRequireHandshake): Use pipe2. * src/qemu/qemu_command.c (qemuBuildCommandLine): Simplify caller. --- Essential for patch 11/16 to work later in this series. v2: rebase to latest https://www.redhat.com/archives/libvir-list/2011-July/msg00675.html I also have an unreviewed command.c fd leak patch here: https://www.redhat.com/archives/libvir-list/2011-July/msg00674.html I tested with v1; I consider v2 as broken beyond repair. However, the leak that it plugs is only on OOM situations, and v1 can be applied either before or after this series. src/qemu/qemu_command.c | 16 ------------- src/util/command.c | 57 ++++++++++++++++++++++++----------------------- 2 files changed, 29 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3bce4a..ee706f9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4575,14 +4575,6 @@ qemuBuildCommandLine(virConnectPtr conn, } else if (STREQ(migrateFrom, "stdio")) { if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { virCommandAddArgFormat(cmd, "fd:%d", migrateFd); - /* migrateFd might be cloexec, but qemu must inherit - * it if vmop indicates qemu will be executed */ - if (vmop != VIR_VM_OP_NO_OP && - virSetInherit(migrateFd, true) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to clear cloexec flag")); - goto error; - } virCommandPreserveFD(cmd, migrateFd); } else if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { virCommandAddArg(cmd, "exec:cat"); @@ -4612,14 +4604,6 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } virCommandAddArg(cmd, migrateFrom); - /* migrateFd might be cloexec, but qemu must inherit - * it if vmop indicates qemu will be executed */ - if (vmop != VIR_VM_OP_NO_OP && - virSetInherit(migrateFd, true) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to clear cloexec flag")); - goto error; - } virCommandPreserveFD(cmd, migrateFd); } else if (STRPREFIX(migrateFrom, "unix")) { if (!qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) { diff --git a/src/util/command.c b/src/util/command.c index 11dd7b0..f8ee8b1 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -259,7 +259,7 @@ cleanup: static int getDevNull(int *null) { - if (*null == -1 && (*null = open("/dev/null", O_RDWR)) < 0) { + if (*null == -1 && (*null = open("/dev/null", O_RDWR|O_CLOEXEC)) < 0) { virReportSystemError(errno, _("cannot open %s"), "/dev/null"); @@ -268,6 +268,18 @@ getDevNull(int *null) return 0; } +/* Ensure that STD is an inheritable copy of FD. Return 0 on success, + * -1 on failure. */ +static int +prepareStdFd(int fd, int std) +{ + if (fd == std) + return virSetInherit(fd, true); + if (dup2(fd, std) != std) + return -1; + return 0; +} + /* * @argv argv to exec * @envp optional environment to use for exec @@ -327,7 +339,7 @@ virExecWithHook(const char *const*argv, if (outfd != NULL) { if (*outfd == -1) { - if (pipe(pipeout) < 0) { + if (pipe2(pipeout, O_CLOEXEC) < 0) { virReportSystemError(errno, "%s", _("cannot create pipe")); goto cleanup; @@ -340,12 +352,6 @@ virExecWithHook(const char *const*argv, goto cleanup; } - if (virSetCloseExec(pipeout[0]) == -1) { - virReportSystemError(errno, - "%s", _("Failed to set close-on-exec file descriptor flag")); - goto cleanup; - } - childout = pipeout[1]; } else { childout = *outfd; @@ -358,7 +364,7 @@ virExecWithHook(const char *const*argv, if (errfd != NULL) { if (*errfd == -1) { - if (pipe(pipeerr) < 0) { + if (pipe2(pipeerr, O_CLOEXEC) < 0) { virReportSystemError(errno, "%s", _("Failed to create pipe")); goto cleanup; @@ -371,12 +377,6 @@ virExecWithHook(const char *const*argv, goto cleanup; } - if (virSetCloseExec(pipeerr[0]) == -1) { - virReportSystemError(errno, - "%s", _("Failed to set close-on-exec file descriptor flag")); - goto cleanup; - } - childerr = pipeerr[1]; } else { childerr = *errfd; @@ -426,28 +426,29 @@ virExecWithHook(const char *const*argv, } openmax = sysconf(_SC_OPEN_MAX); - for (i = 3; i < openmax; i++) - if (i != infd && - i != childout && - i != childerr && - (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd))) { + for (i = 3; i < openmax; i++) { + if (i == infd || i == childout || i == childerr) + continue; + if (!keepfd || i >= FD_SETSIZE || !FD_ISSET(i, keepfd)) { tmpfd = i; VIR_FORCE_CLOSE(tmpfd); + } else if (virSetInherit(i, true) < 0) { + virReportSystemError(errno, _("failed to preserve fd %d"), i); + goto fork_error; } + } - if (dup2(infd, STDIN_FILENO) < 0) { + if (prepareStdFd(infd, STDIN_FILENO) < 0) { virReportSystemError(errno, "%s", _("failed to setup stdin file handle")); goto fork_error; } - if (childout > 0 && - dup2(childout, STDOUT_FILENO) < 0) { + if (childout > 0 && prepareStdFd(childout, STDOUT_FILENO) < 0) { virReportSystemError(errno, "%s", _("failed to setup stdout file handle")); goto fork_error; } - if (childerr > 0 && - dup2(childerr, STDERR_FILENO) < 0) { + if (childerr > 0 && prepareStdFd(childerr, STDERR_FILENO) < 0) { virReportSystemError(errno, "%s", _("failed to setup stderr file handle")); goto fork_error; @@ -1805,7 +1806,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) /* If we have an input buffer, we need * a pipe to feed the data to the child */ if (cmd->inbuf) { - if (pipe(infd) < 0) { + if (pipe2(infd, O_CLOEXEC) < 0) { virReportSystemError(errno, "%s", _("unable to open pipe")); cmd->has_error = -1; @@ -2295,11 +2296,11 @@ void virCommandRequireHandshake(virCommandPtr cmd) return; } - if (pipe(cmd->handshakeWait) < 0) { + if (pipe2(cmd->handshakeWait, O_CLOEXEC) < 0) { cmd->has_error = errno; return; } - if (pipe(cmd->handshakeNotify) < 0) { + if (pipe2(cmd->handshakeNotify, O_CLOEXEC) < 0) { VIR_FORCE_CLOSE(cmd->handshakeWait[0]); VIR_FORCE_CLOSE(cmd->handshakeWait[1]); cmd->has_error = errno; -- 1.7.4.4

In preparation for patch 12 later in this series adding new virFile APIs. * src/util/files.h, src/util/files.c: Move... * src/util/virfile.h, src/util/virfile.c: ...here, and rename functions to virFile prefix. Macro names are intentionally left alone. * *.c: All '#include "files.h"' uses changed. * src/Makefile.am (UTIL_SOURCES): Reflect rename. * cfg.mk (exclude_file_name_regexp--sc_prohibit_close): Likewise. * src/libvirt_private.syms: Likewise. * docs/hacking.html.in: Likewise. * HACKING: Regenerate. --- v2: new patch HACKING | 2 +- cfg.mk | 2 +- daemon/libvirtd.c | 2 +- docs/hacking.html.in | 2 +- src/Makefile.am | 2 +- src/conf/domain_conf.c | 2 +- src/conf/network_conf.c | 2 +- src/conf/nwfilter_conf.c | 2 +- src/conf/storage_conf.c | 2 +- src/conf/storage_encryption_conf.c | 2 +- src/fdstream.c | 2 +- src/libvirt_private.syms | 12 ++++++------ src/libxl/libxl_conf.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/locking/lock_driver_sanlock.c | 2 +- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/node_device/node_device_linux_sysfs.c | 2 +- src/nodeinfo.c | 2 +- src/nwfilter/nwfilter_ebiptables_driver.c | 2 +- src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_conf.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_process.c | 2 +- src/remote/remote_driver.c | 2 +- src/rpc/virnetclient.c | 2 +- src/rpc/virnetserver.c | 2 +- src/rpc/virnetsocket.c | 2 +- src/secret/secret_driver.c | 2 +- src/security/security_apparmor.c | 2 +- src/security/security_selinux.c | 2 +- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.c | 2 +- src/storage/storage_backend_fs.c | 2 +- src/storage/storage_backend_iscsi.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- src/storage/storage_driver.c | 2 +- src/test/test_driver.c | 2 +- src/uml/uml_conf.c | 2 +- src/uml/uml_driver.c | 2 +- src/util/bridge.c | 2 +- src/util/cgroup.c | 2 +- src/util/command.c | 2 +- src/util/conf.c | 2 +- src/util/dnsmasq.c | 2 +- src/util/event_poll.c | 2 +- src/util/hooks.c | 2 +- src/util/interface.c | 2 +- src/util/iohelper.c | 2 +- src/util/logging.c | 2 +- src/util/macvtap.c | 2 +- src/util/pci.c | 2 +- src/util/stats_linux.c | 2 +- src/util/storage_file.c | 2 +- src/util/util.c | 2 +- src/util/uuid.c | 2 +- src/util/viraudit.c | 2 +- src/util/{files.c => virfile.c} | 10 +++++----- src/util/{files.h => virfile.h} | 18 +++++++++--------- src/vbox/vbox_tmpl.c | 2 +- src/vmware/vmware_conf.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/xen/block_stats.c | 2 +- src/xen/xen_driver.c | 2 +- src/xen/xen_hypervisor.c | 2 +- src/xen/xen_inotify.c | 2 +- src/xen/xend_internal.c | 2 +- tests/commandhelper.c | 2 +- tests/commandtest.c | 2 +- tests/nodeinfotest.c | 2 +- tests/testutils.c | 2 +- tests/virnetsockettest.c | 2 +- tests/xencapstest.c | 2 +- tools/console.c | 2 +- tools/virsh.c | 2 +- 86 files changed, 103 insertions(+), 103 deletions(-) rename src/util/{files.c => virfile.c} (88%) rename src/util/{files.h => virfile.h} (70%) diff --git a/HACKING b/HACKING index 8ebbec7..2df560a 100644 --- a/HACKING +++ b/HACKING @@ -417,7 +417,7 @@ File handling Usage of the "fdopen()", "close()", "fclose()" APIs is deprecated in libvirt code base to help avoiding double-closing of files or file descriptors, which is particulary dangerous in a multi-threaded applications. Instead of these -APIs, use the macros from files.h +APIs, use the macros from virfile.h - Open a file from a file descriptor: diff --git a/cfg.mk b/cfg.mk index d243862..0a624f1 100644 --- a/cfg.mk +++ b/cfg.mk @@ -689,7 +689,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ ^(bootstrap.conf$$|src/util/util\.c$$|examples/domain-events/events-c/event-test\.c$$) exclude_file_name_regexp--sc_prohibit_close = \ - (\.p[yl]$$|^docs/|^(src/util/files\.c|src/libvirt\.c)$$) + (\.p[yl]$$|^docs/|^(src/util/virfile\.c|src/libvirt\.c)$$) exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \ (^tests/qemuhelpdata/|\.(gif|ico|png)$$) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 259fcc4..38876d8 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -34,7 +34,7 @@ #include "libvirt_internal.h" #include "virterror_internal.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 6888687..1a32d07 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -506,7 +506,7 @@ Usage of the <code>fdopen()</code>, <code>close()</code>, <code>fclose()</code> APIs is deprecated in libvirt code base to help avoiding double-closing of files or file descriptors, which is particulary dangerous in a multi-threaded - applications. Instead of these APIs, use the macros from files.h + applications. Instead of these APIs, use the macros from virfile.h </p> <ul> diff --git a/src/Makefile.am b/src/Makefile.am index f4ff489..cd8fa46 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -52,7 +52,6 @@ UTIL_SOURCES = \ util/cgroup.c util/cgroup.h \ util/event.c util/event.h \ util/event_poll.c util/event_poll.h \ - util/files.c util/files.h \ util/hash.c util/hash.h \ util/hooks.c util/hooks.h \ util/iptables.c util/iptables.h \ @@ -80,6 +79,7 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/viraudit.c util/viraudit.h \ + util/virfile.c util/virfile.h \ util/xml.c util/xml.h \ util/virterror.c util/virterror_internal.h diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c3ab39..b226574 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -46,7 +46,7 @@ #include "nwfilter_conf.h" #include "ignore-value.h" #include "storage_file.h" -#include "files.h" +#include "virfile.h" #include "bitmap.h" #include "count-one-bits.h" diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ae479bf..9f7ce04 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -41,7 +41,7 @@ #include "util.h" #include "buf.h" #include "c-ctype.h" -#include "files.h" +#include "virfile.h" #define MAX_BRIDGE_ID 256 #define VIR_FROM_THIS VIR_FROM_NETWORK diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 036c61a..04bfa22 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -46,7 +46,7 @@ #include "nwfilter_conf.h" #include "domain_conf.h" #include "c-ctype.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index cc55b80..995f9a6 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -43,7 +43,7 @@ #include "buf.h" #include "util.h" #include "memory.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/conf/storage_encryption_conf.c b/src/conf/storage_encryption_conf.c index 545efad..73e16ed 100644 --- a/src/conf/storage_encryption_conf.c +++ b/src/conf/storage_encryption_conf.c @@ -35,7 +35,7 @@ #include "xml.h" #include "virterror_internal.h" #include "uuid.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/fdstream.c b/src/fdstream.c index dd742e1..d25b3f0 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -38,7 +38,7 @@ #include "logging.h" #include "memory.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_STREAMS diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e3b1dd..d42e0a0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -488,12 +488,6 @@ virFDStreamOpenFile; virFDStreamCreateFile; -# files.h -virClose; -virFclose; -virFdopen; - - # hash.h virHashAddEntry; virHashCreate; @@ -1086,6 +1080,12 @@ virAuditOpen; virAuditSend; +# virfile.h +virFileClose; +virFileFclose; +virFileFdopen; + + # virterror_internal.h virDispatchError; virErrorMsg; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 0f6035e..b74a4b1 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -34,7 +34,7 @@ #include "logging.h" #include "virterror_internal.h" #include "datatypes.h" -#include "files.h" +#include "virfile.h" #include "memory.h" #include "uuid.h" #include "capabilities.h" diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d52a8b6..381d90b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -36,7 +36,7 @@ #include "virterror_internal.h" #include "conf.h" #include "datatypes.h" -#include "files.h" +#include "virfile.h" #include "memory.h" #include "uuid.h" #include "command.h" diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 62eb28b..b85f1fa 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -41,7 +41,7 @@ #include "virterror_internal.h" #include "memory.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "md5.h" #include "conf.h" diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 8e1860b..432b7f8 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -53,7 +53,7 @@ #include "memory.h" #include "veth.h" #include "uuid.h" -#include "files.h" +#include "virfile.h" #include "command.h" #define VIR_FROM_THIS VIR_FROM_LXC diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 346edef..7eda7ef 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -53,7 +53,7 @@ #include "veth.h" #include "memory.h" #include "util.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_LXC diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index afac879..80378d3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -49,7 +49,7 @@ #include "uuid.h" #include "stats_linux.h" #include "hooks.h" -#include "files.h" +#include "virfile.h" #include "fdstream.h" #include "domain_audit.h" #include "domain_nwfilter.h" diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c index 34e4501..f9ff20f 100644 --- a/src/node_device/node_device_linux_sysfs.c +++ b/src/node_device/node_device_linux_sysfs.c @@ -31,7 +31,7 @@ #include "virterror_internal.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include <dirent.h> #define VIR_FROM_THIS VIR_FROM_NODEDEV diff --git a/src/nodeinfo.c b/src/nodeinfo.c index a53c56d..cae2564 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -45,7 +45,7 @@ #include "virterror_internal.h" #include "count-one-bits.h" #include "intprops.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_NONE diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index c9b60da..f87cfa1 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -38,7 +38,7 @@ #include "nwfilter_conf.h" #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" -#include "files.h" +#include "virfile.h" #include "command.h" diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 561cc99..c60a97f 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -50,7 +50,7 @@ #include "memory.h" #include "util.h" #include "nodeinfo.h" -#include "files.h" +#include "virfile.h" #include "command.h" #include "ignore-value.h" diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index d24a5e3..f0c6f57 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -55,7 +55,7 @@ #include "nodeinfo.h" #include "memory.h" #include "bridge.h" -#include "files.h" +#include "virfile.h" #include "logging.h" #include "command.h" diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 2489063..e693e76 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -57,7 +57,7 @@ #include "domain_conf.h" #include "storage_conf.h" #include "nodeinfo.h" -#include "files.h" +#include "virfile.h" #include "interface_conf.h" #include "phyp_driver.h" diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1421a5e..3f36212 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -28,7 +28,7 @@ #include "logging.h" #include "virterror_internal.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "nodeinfo.h" #include "cpu/cpu.h" #include "domain_conf.h" diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ee706f9..938f113 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -31,7 +31,7 @@ #include "logging.h" #include "virterror_internal.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "uuid.h" #include "c-ctype.h" #include "domain_nwfilter.h" diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 3d8aba4..4a17a55 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -55,7 +55,7 @@ #include "macvtap.h" #include "cpu/cpu.h" #include "domain_nwfilter.h" -#include "files.h" +#include "virfile.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f9755a4..1a17f99 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -33,7 +33,7 @@ #include "cpu/cpu.h" #include "ignore-value.h" #include "uuid.h" -#include "files.h" +#include "virfile.h" #include <sys/time.h> #include <fcntl.h> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cd65bce..20eca30 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -82,7 +82,7 @@ #include "domain_nwfilter.h" #include "hooks.h" #include "storage_file.h" -#include "files.h" +#include "virfile.h" #include "fdstream.h" #include "configmake.h" #include "threadpool.h" diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0eae661..20f0daf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -36,7 +36,7 @@ #include "virterror_internal.h" #include "memory.h" #include "pci.h" -#include "files.h" +#include "virfile.h" #include "qemu_cgroup.h" #include "locking/domain_lock.h" diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1803b9f..9c1bde5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -37,7 +37,7 @@ #include "virterror_internal.h" #include "memory.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "datatypes.h" #include "fdstream.h" #include "uuid.h" diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3a30a15..6de65b9 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -35,7 +35,7 @@ #include "virterror_internal.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 448b06e..412a454 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -48,7 +48,7 @@ #include "virterror_internal.h" #include "memory.h" #include "hooks.h" -#include "files.h" +#include "virfile.h" #include "util.h" #include "c-ctype.h" #include "nodeinfo.h" diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 6ba58ed..5b7a338 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -43,7 +43,7 @@ #include "qemu_protocol.h" #include "memory.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "command.h" #include "intprops.h" diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index dfc4ed9..3b25d52 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -31,7 +31,7 @@ #include "virnetsocket.h" #include "memory.h" #include "threads.h" -#include "files.h" +#include "virfile.h" #include "logging.h" #include "util.h" #include "virterror_internal.h" diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 35f18b9..731932c 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -34,7 +34,7 @@ #include "threads.h" #include "threadpool.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "event.h" #if HAVE_AVAHI # include "virnetservermdns.h" diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 4403fc4..8c32ad3 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -38,7 +38,7 @@ #include "memory.h" #include "virterror_internal.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "event.h" #include "threads.h" diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 02cdbb9..59dc687 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -40,7 +40,7 @@ #include "util.h" #include "uuid.h" #include "virterror_internal.h" -#include "files.h" +#include "virfile.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_SECRET diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 6dfe8c9..1d49ff6 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -35,7 +35,7 @@ #include "uuid.h" #include "pci.h" #include "hostusb.h" -#include "files.h" +#include "virfile.h" #include "configmake.h" #include "command.h" diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 50e1978..5e6145f 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -31,7 +31,7 @@ #include "pci.h" #include "hostusb.h" #include "storage_file.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_SECURITY diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 856d32f..1e2feae 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -40,7 +40,7 @@ #include "uuid.h" #include "hostusb.h" #include "pci.h" -#include "files.h" +#include "virfile.h" #include "configmake.h" #define VIR_FROM_THIS VIR_FROM_SECURITY diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f632edd..6243d1e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -56,7 +56,7 @@ #include "storage_file.h" #include "storage_backend.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "command.h" #if WITH_STORAGE_LVM diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 8d6f76d..b77cd3a 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -44,7 +44,7 @@ #include "command.h" #include "memory.h" #include "xml.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 7b8dc97..346e698 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -41,7 +41,7 @@ #include "util.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "command.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 5fe9a1f..c622d2a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -37,7 +37,7 @@ #include "command.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 2d48a9d..f2e1419 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -35,7 +35,7 @@ #include "storage_backend.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index da34547..ae1e19f 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -32,7 +32,7 @@ #include "storage_backend_scsi.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "command.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 997b876..9c353e3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -45,7 +45,7 @@ #include "memory.h" #include "storage_backend.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "fdstream.h" #include "configmake.h" diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a68cc0d..0e3bf53 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -49,7 +49,7 @@ #include "xml.h" #include "threads.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_TEST diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 0122472..c4b6e75 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -47,7 +47,7 @@ #include "bridge.h" #include "logging.h" #include "domain_nwfilter.h" -#include "files.h" +#include "virfile.h" #include "command.h" #define VIR_FROM_THIS VIR_FROM_UML diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 0d6f71a..e1eedd4 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -59,7 +59,7 @@ #include "datatypes.h" #include "logging.h" #include "domain_nwfilter.h" -#include "files.h" +#include "virfile.h" #include "fdstream.h" #include "configmake.h" diff --git a/src/util/bridge.c b/src/util/bridge.c index 0f4b639..d63b2a0 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -24,7 +24,7 @@ #if defined(WITH_BRIDGE) # include "bridge.h" -# include "files.h" +# include "virfile.h" # include <stdlib.h> # include <stdio.h> diff --git a/src/util/cgroup.c b/src/util/cgroup.c index 740cedf..c12cf32 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -31,7 +31,7 @@ #include "memory.h" #include "cgroup.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "hash.h" #define CGROUP_MAX_VAL 512 diff --git a/src/util/command.c b/src/util/command.c index f8ee8b1..29ccaa4 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -38,7 +38,7 @@ #include "virterror_internal.h" #include "util.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "buf.h" #include "ignore-value.h" #include "verify.h" diff --git a/src/util/conf.c b/src/util/conf.c index 377f9cd..00045b5 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -24,7 +24,7 @@ #include "util.h" #include "c-ctype.h" #include "memory.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_CONF diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c index 55db96b..d76cf65 100644 --- a/src/util/dnsmasq.c +++ b/src/util/dnsmasq.c @@ -44,7 +44,7 @@ #include "memory.h" #include "virterror_internal.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_NETWORK #define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile" diff --git a/src/util/event_poll.c b/src/util/event_poll.c index e2ae3a6..e8679b8 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -36,7 +36,7 @@ #include "event_poll.h" #include "memory.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "ignore-value.h" #include "virterror_internal.h" diff --git a/src/util/hooks.c b/src/util/hooks.c index 30e20ac..64adfcb 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -35,7 +35,7 @@ #include "util.h" #include "logging.h" #include "memory.h" -#include "files.h" +#include "virfile.h" #include "configmake.h" #include "command.h" diff --git a/src/util/interface.c b/src/util/interface.c index 837ecce..7b1a296 100644 --- a/src/util/interface.c +++ b/src/util/interface.c @@ -41,7 +41,7 @@ #include "util.h" #include "interface.h" #include "virterror_internal.h" -#include "files.h" +#include "virfile.h" #include "memory.h" #include "netlink.h" diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 0368eba..6a9f355 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -35,7 +35,7 @@ #include "util.h" #include "threads.h" -#include "files.h" +#include "virfile.h" #include "memory.h" #include "virterror_internal.h" #include "configmake.h" diff --git a/src/util/logging.c b/src/util/logging.c index d340f57..a638510 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -42,7 +42,7 @@ #include "util.h" #include "buf.h" #include "threads.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_NONE diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 30343c8..8386f3b 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -64,7 +64,7 @@ VIR_ENUM_IMPL(virMacvtapMode, VIR_MACVTAP_MODE_LAST, # include "interface.h" # include "virterror_internal.h" # include "uuid.h" -# include "files.h" +# include "virfile.h" # include "netlink.h" # define VIR_FROM_THIS VIR_FROM_NET diff --git a/src/util/pci.c b/src/util/pci.c index e3f978c..a79c164 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -37,7 +37,7 @@ #include "memory.h" #include "command.h" #include "virterror_internal.h" -#include "files.h" +#include "virfile.h" /* avoid compilation breakage on some systems */ #ifndef MODPROBE diff --git a/src/util/stats_linux.c b/src/util/stats_linux.c index e728b7b..72318e3 100644 --- a/src/util/stats_linux.c +++ b/src/util/stats_linux.c @@ -25,7 +25,7 @@ # include "util.h" # include "stats_linux.h" # include "memory.h" -# include "files.h" +# include "virfile.h" # define VIR_FROM_THIS VIR_FROM_STATS_LINUX diff --git a/src/util/storage_file.c b/src/util/storage_file.c index d4460d8..68e82a9 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -37,7 +37,7 @@ #include "memory.h" #include "virterror_internal.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_STORAGE diff --git a/src/util/util.c b/src/util/util.c index 910bb04..d83215c 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -73,7 +73,7 @@ #include "memory.h" #include "threads.h" #include "verify.h" -#include "files.h" +#include "virfile.h" #include "command.h" #include "nonblocking.h" #include "passfd.h" diff --git a/src/util/uuid.c b/src/util/uuid.c index 805c20f..b4317df 100644 --- a/src/util/uuid.c +++ b/src/util/uuid.c @@ -39,7 +39,7 @@ #include "virterror_internal.h" #include "logging.h" #include "memory.h" -#include "files.h" +#include "virfile.h" #ifndef ENODATA # define ENODATA EIO diff --git a/src/util/viraudit.c b/src/util/viraudit.c index ebf3119..d0f9cc4 100644 --- a/src/util/viraudit.c +++ b/src/util/viraudit.c @@ -31,7 +31,7 @@ #include "logging.h" #include "viraudit.h" #include "util.h" -#include "files.h" +#include "virfile.h" #include "memory.h" /* Provide the macros in case the header file is old. diff --git a/src/util/files.c b/src/util/virfile.c similarity index 88% rename from src/util/files.c rename to src/util/virfile.c index bef56b6..6576921 100644 --- a/src/util/files.c +++ b/src/util/virfile.c @@ -1,5 +1,5 @@ /* - * files.c: safer file handling + * virfile.c: safer file handling * * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corporation @@ -26,9 +26,9 @@ #include <unistd.h> -#include "files.h" +#include "virfile.h" -int virClose(int *fdptr, bool preserve_errno) +int virFileClose(int *fdptr, bool preserve_errno) { int saved_errno; int rc = 0; @@ -46,7 +46,7 @@ int virClose(int *fdptr, bool preserve_errno) } -int virFclose(FILE **file, bool preserve_errno) +int virFileFclose(FILE **file, bool preserve_errno) { int saved_errno; int rc = 0; @@ -64,7 +64,7 @@ int virFclose(FILE **file, bool preserve_errno) } -FILE *virFdopen(int *fdptr, const char *mode) +FILE *virFileFdopen(int *fdptr, const char *mode) { FILE *file = NULL; diff --git a/src/util/files.h b/src/util/virfile.h similarity index 70% rename from src/util/files.h rename to src/util/virfile.h index 8b681eb..d11f902 100644 --- a/src/util/files.h +++ b/src/util/virfile.h @@ -1,5 +1,5 @@ /* - * files.h: safer file handling + * virfile.h: safer file handling * * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corporation @@ -33,21 +33,21 @@ /* Don't call these directly - use the macros below */ -int virClose(int *fdptr, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; -int virFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; -FILE *virFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; +int virFileClose(int *fdptr, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; +int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; +FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; /* For use on normal paths; caller must check return value, and failure sets errno per close. */ -# define VIR_CLOSE(FD) virClose(&(FD), false) -# define VIR_FCLOSE(FILE) virFclose(&(FILE), false) +# define VIR_CLOSE(FD) virFileClose(&(FD), false) +# define VIR_FCLOSE(FILE) virFileFclose(&(FILE), false) /* Wrapper around fdopen that consumes fd on success. */ -# define VIR_FDOPEN(FD, MODE) virFdopen(&(FD), MODE) +# define VIR_FDOPEN(FD, MODE) virFileFdopen(&(FD), MODE) /* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) ignore_value(virClose(&(FD), true)) -# define VIR_FORCE_FCLOSE(FILE) ignore_value(virFclose(&(FILE), true)) +# define VIR_FORCE_CLOSE(FD) ignore_value(virFileClose(&(FD), true)) +# define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true)) #endif /* __VIR_FILES_H */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 53bac79..f065728 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -54,7 +54,7 @@ #include "logging.h" #include "vbox_driver.h" #include "configmake.h" -#include "files.h" +#include "virfile.h" #include "fdstream.h" /* This one changes from version to version. */ diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index 044784e..efefab4 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -29,7 +29,7 @@ #include "dirname.h" #include "memory.h" #include "nodeinfo.h" -#include "files.h" +#include "virfile.h" #include "uuid.h" #include "virterror_internal.h" #include "vmx.h" diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 7cec310..71f3d22 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -26,7 +26,7 @@ #include "internal.h" #include "virterror_internal.h" #include "datatypes.h" -#include "files.h" +#include "virfile.h" #include "memory.h" #include "uuid.h" #include "command.h" diff --git a/src/xen/block_stats.c b/src/xen/block_stats.c index 1cb5455..0bb8098 100644 --- a/src/xen/block_stats.c +++ b/src/xen/block_stats.c @@ -27,7 +27,7 @@ # include "util.h" # include "block_stats.h" # include "memory.h" -# include "files.h" +# include "virfile.h" # define VIR_FROM_THIS VIR_FROM_STATS_LINUX diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 3b5df46..7e3ef77 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -49,7 +49,7 @@ #include "pci.h" #include "uuid.h" #include "fdstream.h" -#include "files.h" +#include "virfile.h" #include "command.h" #define VIR_FROM_THIS VIR_FROM_XEN diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 543dfb1..9700471 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -65,7 +65,7 @@ #include "buf.h" #include "capabilities.h" #include "memory.h" -#include "files.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_XEN diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index 241dbc7..20e54ea 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -38,7 +38,7 @@ #include "xend_internal.h" #include "logging.h" #include "uuid.h" -#include "files.h" +#include "virfile.h" #include "xm_internal.h" /* for xenXMDomainConfigParse */ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index dec8484..b0e5cb1 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -45,7 +45,7 @@ #include "xs_internal.h" /* To extract VNC port & Serial console TTY */ #include "memory.h" #include "count-one-bits.h" -#include "files.h" +#include "virfile.h" /* required for cpumap_t */ #include <xen/dom0_ops.h> diff --git a/tests/commandhelper.c b/tests/commandhelper.c index d60d505..cba208a 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -29,7 +29,7 @@ #include "internal.h" #include "util.h" #include "memory.h" -#include "files.h" +#include "virfile.h" static int envsort(const void *a, const void *b) { diff --git a/tests/commandtest.c b/tests/commandtest.c index 6757253..9ab446c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -34,7 +34,7 @@ #include "util.h" #include "memory.h" #include "command.h" -#include "files.h" +#include "virfile.h" #ifdef WIN32 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 71e2926..5abee92 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -9,7 +9,7 @@ #include "internal.h" #include "nodeinfo.h" #include "util.h" -#include "files.h" +#include "virfile.h" #if ! (defined __linux__ && (defined(__x86_64__) || \ defined(__amd64__) || \ diff --git a/tests/testutils.c b/tests/testutils.c index ac5d298..d9582af 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -47,7 +47,7 @@ ((((int) ((T)->tv_sec - (U)->tv_sec)) * 1000000.0 + \ ((int) ((T)->tv_usec - (U)->tv_usec))) / 1000.0) -#include "files.h" +#include "virfile.h" static unsigned int testDebug = -1; static unsigned int testVerbose = -1; diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 1697ced..0c86b84 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -31,7 +31,7 @@ #include "virterror_internal.h" #include "memory.h" #include "logging.h" -#include "files.h" +#include "virfile.h" #include "rpc/virnetsocket.h" diff --git a/tests/xencapstest.c b/tests/xencapstest.c index 3f96cf2..9c1eba4 100644 --- a/tests/xencapstest.c +++ b/tests/xencapstest.c @@ -9,7 +9,7 @@ #include "xml.h" #include "testutils.h" #include "xen/xen_hypervisor.h" -#include "files.h" +#include "virfile.h" static int testCompareFiles(const char *hostmachine, const char *xml_rel, diff --git a/tools/console.c b/tools/console.c index 7ca95a3..11087e5 100644 --- a/tools/console.c +++ b/tools/console.c @@ -39,7 +39,7 @@ # include "console.h" # include "logging.h" # include "util.h" -# include "files.h" +# include "virfile.h" # include "memory.h" # include "virterror_internal.h" diff --git a/tools/virsh.c b/tools/virsh.c index ca92f0c..ddefb57 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -52,7 +52,7 @@ #include "memory.h" #include "xml.h" #include "libvirt/libvirt-qemu.h" -#include "files.h" +#include "virfile.h" #include "event_poll.h" #include "configmake.h" #include "threads.h" -- 1.7.4.4

Otherwise, an ABI mismatch gives error messages attributing the target xml string as current, and the current domain state as the new xml. * src/qemu/qemu_migration.c (qemuMigrationBegin): Use correct argument order. --- v2: new patch. Noticed because I used the public migrate2 handling of dxml as the model for my patch 16/16, then noticed that the error messages were backwards as a result. src/qemu/qemu_migration.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9c1bde5..3282271 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1037,7 +1037,7 @@ char *qemuMigrationBegin(struct qemud_driver *driver, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - if (!virDomainDefCheckABIStability(def, vm->def)) + if (!virDomainDefCheckABIStability(vm->def, def)) goto cleanup; rv = qemuDomainDefFormatXML(driver, def, -- 1.7.4.4

In order to choose whether to use O_DIRECT when saving a domain image to a file, we need a new flag. But virDomainSave was implemented before our policy of all new APIs having a flag argument. Likewise for virDomainRestore when restoring from a file. The new flag name is chosen as CACHE_BYPASS so as not to preclude a future solution that uses posix_fadvise once the Linux kernel has a smarter implementation of that interface. * include/libvirt/libvirt.h.in (virDomainCreateFlags) (virDomainCoreDumpFlags): Add a flag. (virDomainSaveFlags, virDomainRestoreFlags): New prototypes. * src/libvirt.c (virDomainSaveFlags, virDomainRestoreFlags): New API. * src/libvirt_public.syms: Export them. * src/driver.h (virDrvDomainSaveFlags, virDrvDomainRestoreFlags): New driver callbacks. --- v2: rename flag from _DIRECT to _BYPASS_CACHE, and merged 1 and 11 of v1 include/libvirt/libvirt.h.in | 30 ++++++- src/driver.h | 12 +++ src/libvirt.c | 181 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 2 + 4 files changed, 219 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 40ce0fc..20c5109 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -232,9 +232,10 @@ typedef virDomainInfo *virDomainInfoPtr; * Domain. */ typedef enum { - VIR_DOMAIN_NONE = 0, /* Default behavior */ - VIR_DOMAIN_START_PAUSED = 1 << 0, /* Launch guest in paused state */ - VIR_DOMAIN_START_AUTODESTROY = 1 << 1, /* Automatically kill guest when virConnectPtr is closed */ + VIR_DOMAIN_NONE = 0, /* Default behavior */ + VIR_DOMAIN_START_PAUSED = 1 << 0, /* Launch guest in paused state */ + VIR_DOMAIN_START_AUTODESTROY = 1 << 1, /* Automatically kill guest when virConnectPtr is closed */ + VIR_DOMAIN_START_BYPASS_CACHE = 1 << 2, /* Avoid file system cache pollution */ } virDomainCreateFlags; @@ -657,8 +658,9 @@ typedef virDomainMemoryStatStruct *virDomainMemoryStatPtr; /* Domain core dump flags. */ typedef enum { - VIR_DUMP_CRASH = (1 << 0), /* crash after dump */ - VIR_DUMP_LIVE = (1 << 1), /* live dump */ + VIR_DUMP_CRASH = (1 << 0), /* crash after dump */ + VIR_DUMP_LIVE = (1 << 1), /* live dump */ + VIR_DUMP_BYPASS_CACHE = (1 << 2), /* avoid file system cache pollution */ } virDomainCoreDumpFlags; /* Domain migration flags. */ @@ -941,10 +943,28 @@ int virDomainResume (virDomainPtr domain); /* * Domain save/restore */ + +/** + * virDomainSaveRestoreFlags: + * Flags for use in virDomainSaveFlags(), virDomainManagedSave(), and + * virDomainRestoreFlags(). + */ +typedef enum { + VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0, /* Avoid file system cache pollution */ +} virDomainSaveRestoreFlags; + int virDomainSave (virDomainPtr domain, const char *to); +int virDomainSaveFlags (virDomainPtr domain, + const char *to, + const char *dxml, + unsigned int flags); int virDomainRestore (virConnectPtr conn, const char *from); +int virDomainRestoreFlags (virConnectPtr conn, + const char *from, + const char *dxml, + unsigned int flags); /* * Managed domain save diff --git a/src/driver.h b/src/driver.h index 4c4955f..d931c9b 100644 --- a/src/driver.h +++ b/src/driver.h @@ -178,9 +178,19 @@ typedef int (*virDrvDomainSave) (virDomainPtr domain, const char *to); typedef int + (*virDrvDomainSaveFlags) (virDomainPtr domain, + const char *to, + const char *dxml, + unsigned int flags); +typedef int (*virDrvDomainRestore) (virConnectPtr conn, const char *from); typedef int + (*virDrvDomainRestoreFlags) (virConnectPtr conn, + const char *from, + const char *dxml, + unsigned int flags); +typedef int (*virDrvDomainCoreDump) (virDomainPtr domain, const char *to, unsigned int flags); @@ -714,7 +724,9 @@ struct _virDriver { virDrvDomainGetState domainGetState; virDrvDomainGetControlInfo domainGetControlInfo; virDrvDomainSave domainSave; + virDrvDomainSaveFlags domainSaveFlags; virDrvDomainRestore domainRestore; + virDrvDomainRestoreFlags domainRestoreFlags; virDrvDomainCoreDump domainCoreDump; virDrvDomainScreenshot domainScreenshot; virDrvDomainSetVcpus domainSetVcpus; diff --git a/src/libvirt.c b/src/libvirt.c index 2f5241a..c769589 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2242,6 +2242,8 @@ error: * listed as running anymore (this may be a problem). * Use virDomainRestore() to restore a domain after saving. * + * See virDomainSaveFlags() for more control. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -2296,12 +2298,93 @@ error: } /** + * virDomainSaveFlags: + * @domain: a domain object + * @to: path for the output file + * @dxml: (optional) XML config for adjusting guest xml used on restore + * @flags: bitwise-OR of virDomainSaveRestoreFlags + * + * This method will suspend a domain and save its memory contents to + * a file on disk. After the call, if successful, the domain is not + * listed as running anymore (this may be a problem). + * Use virDomainRestore() to restore a domain after saving. + * + * If the hypervisor supports it, @dxml can be used to alter + * host-specific portions of the domain XML that will be used when + * restoring an image. For example, it is possible to alter the + * backing filename that is associated with a disk device, in order to + * prepare for file renaming done as part of backing up the disk + * device while the domain is stopped. + * + * If @flags includes VIR_DOMAIN_SAVE_BYPASS_CACHE, then libvirt will + * attempt to bypass the file system cache while creating the file, or + * fail if it cannot do so for the given system; this can allow less + * pressure on file system cache, but also risks slowing saves to NFS. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virDomainSaveFlags(virDomainPtr domain, const char *to, + const char *dxml, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "to=%s dxml=%s flags=%x", + to, NULLSTR(dxml), flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + conn = domain->conn; + if (to == NULL) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->domainSaveFlags) { + int ret; + char *absolute_to; + + /* We must absolutize the file path as the save is done out of process */ + if (virFileAbsPath(to, &absolute_to) < 0) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, + _("could not build absolute output file path")); + goto error; + } + + ret = conn->driver->domainSaveFlags(domain, absolute_to, dxml, flags); + + VIR_FREE(absolute_to); + + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** * virDomainRestore: * @conn: pointer to the hypervisor connection * @from: path to the input file * * This method will restore a domain saved to disk by virDomainSave(). * + * See virDomainRestoreFlags() for more control. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -2353,6 +2436,80 @@ error: } /** + * virDomainRestoreFlags: + * @conn: pointer to the hypervisor connection + * @from: path to the input file + * @dxml: (optional) XML config for adjusting guest xml used on restore + * @flags: bitwise-OR of virDomainSaveRestoreFlags + * + * This method will restore a domain saved to disk by virDomainSave(). + * + * If the hypervisor supports it, @dxml can be used to alter + * host-specific portions of the domain XML that will be used when + * restoring an image. For example, it is possible to alter the + * backing filename that is associated with a disk device, in order to + * prepare for file renaming done as part of backing up the disk + * device while the domain is stopped. + * + * If @flags includes VIR_DOMAIN_SAVE_BYPASS_CACHE, then libvirt will + * attempt to bypass the file system cache while restoring the file, or + * fail if it cannot do so for the given system; this can allow less + * pressure on file system cache, but also risks slowing saves to NFS. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, from=%s, dxml=%s, flags=%x", + conn, from, NULLSTR(dxml), flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + if (conn->flags & VIR_CONNECT_RO) { + virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + if (from == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->domainRestoreFlags) { + int ret; + char *absolute_from; + + /* We must absolutize the file path as the restore is done out of process */ + if (virFileAbsPath(from, &absolute_from) < 0) { + virLibConnError(VIR_ERR_INTERNAL_ERROR, + _("could not build absolute input file path")); + goto error; + } + + ret = conn->driver->domainRestoreFlags(conn, absolute_from, dxml, + flags); + + VIR_FREE(absolute_from); + + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainCoreDump: * @domain: a domain object * @to: path for the core file @@ -2362,6 +2519,17 @@ error: * Note that for remote Xen Daemon the file path will be interpreted in * the remote host. * + * If @flags includes VIR_DUMP_CRASH, then leave the guest shut off with + * a crashed state after the dump completes. If @flags includes + * VIR_DUMP_LIVE, then make the core dump while continuing to allow + * the guest to run; otherwise, the guest is suspended during the dump. + * The above two flags are mutually exclusive. + * + * Additionally, if @flags includes VIR_DUMP_BYPASS_CACHE, then libvirt + * will attempt to bypass the file system cache while creating the file, + * or fail if it cannot do so for the given system; this can allow less + * pressure on file system cache, but also risks slowing saves to NFS. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -6612,6 +6780,12 @@ error: * libvirtd daemon. Any domains marked for auto destroy will * block attempts at migration or save-to-file * + * If the VIR_DOMAIN_START_BYPASS_CACHE flag is set, and there is a + * managed save file for this domain (created by virDomainManagedSave()), + * then libvirt will attempt to bypass the file system cache while restoring + * the file, or fail if it cannot do so for the given system; this can allow + * less pressure on file system cache, but also risks slowing loads from NFS. + * * Returns 0 in case of success, -1 in case of error */ int @@ -14837,7 +15011,7 @@ error: /** * virDomainManagedSave: * @dom: pointer to the domain - * @flags: optional flags currently unused + * @flags: bitwise-OR of virDomainSaveRestoreFlags * * This method will suspend a domain and save its memory contents to * a file on disk. After the call, if successful, the domain is not @@ -14847,6 +15021,11 @@ error: * restarted (automatically or via an explicit libvirt call). * As a result any running domain is sure to not have a managed saved image. * + * If @flags includes VIR_DOMAIN_SAVE_BYPASS_CACHE, then libvirt will + * attempt to bypass the file system cache while creating the file, or + * fail if it cannot do so for the given system; this can allow less + * pressure on file system cache, but also risks slowing saves to NFS. + * * Returns 0 in case of success or -1 in case of failure */ int virDomainManagedSave(virDomainPtr dom, unsigned int flags) diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 5cc480e..6935140 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -468,6 +468,8 @@ LIBVIRT_0.9.3 { LIBVIRT_0.9.4 { global: + virDomainRestoreFlags; + virDomainSaveFlags; virDomainUndefineFlags; } LIBVIRT_0.9.3; -- 1.7.4.4

* src/remote/remote_driver.c (remote_driver): Add new callbacks. * src/remote/remote_protocol.x (remote_procedure): New RPCs. (remote_domain_save_flags_args, remote_domain_restore_flags_args): New structs. * src/remote_protocol-structs: Update. --- v2: merge 2 and 12 of v1, rebase onto latest libvirt.git src/remote/remote_driver.c | 2 ++ src/remote/remote_protocol.x | 17 ++++++++++++++++- src/remote_protocol-structs | 13 +++++++++++++ 3 files changed, 31 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 5b7a338..692decb 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4165,7 +4165,9 @@ static virDriver remote_driver = { .domainGetState = remoteDomainGetState, /* 0.9.2 */ .domainGetControlInfo = remoteDomainGetControlInfo, /* 0.9.3 */ .domainSave = remoteDomainSave, /* 0.3.0 */ + .domainSaveFlags = remoteDomainSaveFlags, /* 0.9.4 */ .domainRestore = remoteDomainRestore, /* 0.3.0 */ + .domainRestoreFlags = remoteDomainRestoreFlags, /* 0.9.4 */ .domainCoreDump = remoteDomainCoreDump, /* 0.3.0 */ .domainScreenshot = remoteDomainScreenshot, /* 0.9.2 */ .domainSetVcpus = remoteDomainSetVcpus, /* 0.3.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ef9dd10..72d7e0a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -725,10 +725,23 @@ struct remote_domain_save_args { remote_nonnull_string to; }; +struct remote_domain_save_flags_args { + remote_nonnull_domain dom; + remote_nonnull_string to; + remote_string dxml; + unsigned int flags; +}; + struct remote_domain_restore_args { remote_nonnull_string from; }; +struct remote_domain_restore_flags_args { + remote_nonnull_string from; + remote_string dxml; + unsigned int flags; +}; + struct remote_domain_core_dump_args { remote_nonnull_domain dom; remote_nonnull_string to; @@ -2390,7 +2403,9 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_CONTROL_INFO = 229, /* autogen autogen */ REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_UNDEFINE_FLAGS = 231 /* autogen autogen */ + REMOTE_PROC_DOMAIN_UNDEFINE_FLAGS = 231, /* autogen autogen */ + REMOTE_PROC_DOMAIN_SAVE_FLAGS = 232, /* autogen autogen */ + REMOTE_PROC_DOMAIN_RESTORE_FLAGS = 233 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 96e74eb..b17804f 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -429,9 +429,20 @@ struct remote_domain_save_args { remote_nonnull_domain dom; remote_nonnull_string to; }; +struct remote_domain_save_flags_args { + remote_nonnull_domain dom; + remote_nonnull_string to; + remote_string dxml; + u_int flags; +}; struct remote_domain_restore_args { remote_nonnull_string from; }; +struct remote_domain_restore_flags_args { + remote_nonnull_string from; + remote_string dxml; + u_int flags; +}; struct remote_domain_core_dump_args { remote_nonnull_domain dom; remote_nonnull_string to; @@ -1864,4 +1875,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_CONTROL_INFO = 229, REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230, REMOTE_PROC_DOMAIN_UNDEFINE_FLAGS = 231, + REMOTE_PROC_DOMAIN_SAVE_FLAGS = 232, + REMOTE_PROC_DOMAIN_RESTORE_FLAGS = 233, }; -- 1.7.4.4

On Tue, Jul 19, 2011 at 10:20:30PM -0600, Eric Blake wrote: I don't particularly like the idea of using VIR_ERR_INVALID_ARG here since that's used to indicate illegal use of an API. This usage is legal, but unsupported. VIR_ERR_CONFIG_UNSUPPORTED is not quite right and VIR_ERR_NO_SUPPORT is used to indicate complete lack of the driver API impl. So perhaps we should have an VIR_ERR_ARGUMENT_UNSUPPORTED ? ACK to the patch with a different error code used. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Wire up the new flag to several virsh commands. Also, the 'dump' command had undocumented flags. * tools/virsh.c (cmdSave, cmdManagedSave, cmdDump, cmdStart) (cmdRestore): Add new flag. * tools/virsh.pod (save, managedsave, dump, start, restore): Document flags. --- v2: merge 4 and 14 of v1, change --direct to --bypass-cache, improve error handling logic flow tools/virsh.c | 75 ++++++++++++++++++++++++++++++++++++++++-------------- tools/virsh.pod | 27 ++++++++++++++++--- 2 files changed, 77 insertions(+), 25 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ddefb57..ab75677 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1540,6 +1540,8 @@ static const vshCmdOptDef opts_start[] = { #endif {"paused", VSH_OT_BOOL, 0, N_("leave the guest paused after creation")}, {"autodestroy", VSH_OT_BOOL, 0, N_("automatically destroy the guest when virsh disconnects")}, + {"bypass-cache", VSH_OT_BOOL, 0, + N_("avoid file system cache when loading")}, {NULL, 0, 0, NULL} }; @@ -1570,6 +1572,8 @@ cmdStart(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_START_PAUSED; if (vshCommandOptBool(cmd, "autodestroy")) flags |= VIR_DOMAIN_START_AUTODESTROY; + if (vshCommandOptBool(cmd, "bypass-cache")) + flags |= VIR_DOMAIN_START_BYPASS_CACHE; /* Prefer older API unless we have to pass a flag. */ if ((flags ? virDomainCreateWithFlags(dom, flags) @@ -1598,6 +1602,7 @@ static const vshCmdInfo info_save[] = { }; static const vshCmdOptDef opts_save[] = { + {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when saving")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to save the data")}, {NULL, 0, 0, NULL} @@ -1609,7 +1614,8 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; const char *name = NULL; const char *to = NULL; - bool ret = true; + bool ret = false; + int flags = 0; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1617,16 +1623,22 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "file", &to) <= 0) return false; + if (vshCommandOptBool(cmd, "bypass-cache")) + flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainSave(dom, to) == 0) { - vshPrint(ctl, _("Domain %s saved to %s\n"), name, to); - } else { + if ((flags ? virDomainSaveFlags(dom, to, NULL, flags) + : virDomainSave(dom, to)) < 0) { vshError(ctl, _("Failed to save domain %s to %s"), name, to); - ret = false; + goto cleanup; } + vshPrint(ctl, _("Domain %s saved to %s\n"), name, to); + ret = true; + +cleanup: virDomainFree(dom); return ret; } @@ -1644,6 +1656,7 @@ static const vshCmdInfo info_managedsave[] = { }; static const vshCmdOptDef opts_managedsave[] = { + {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when saving")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {NULL, 0, 0, NULL} }; @@ -1653,21 +1666,27 @@ cmdManagedSave(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; const char *name; - bool ret = true; + bool ret = false; + int flags = 0; if (!vshConnectionUsability(ctl, ctl->conn)) return false; + if (vshCommandOptBool(cmd, "bypass-cache")) + flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - if (virDomainManagedSave(dom, 0) == 0) { - vshPrint(ctl, _("Domain %s state saved by libvirt\n"), name); - } else { + if (virDomainManagedSave(dom, flags) < 0) { vshError(ctl, _("Failed to save domain %s state"), name); - ret = false; + goto cleanup; } + vshPrint(ctl, _("Domain %s state saved by libvirt\n"), name); + ret = true; + +cleanup: virDomainFree(dom); return ret; } @@ -1994,6 +2013,8 @@ static const vshCmdInfo info_restore[] = { static const vshCmdOptDef opts_restore[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("the state to restore")}, + {"bypass-cache", VSH_OT_BOOL, 0, + N_("avoid file system cache when restoring")}, {NULL, 0, 0, NULL} }; @@ -2001,7 +2022,8 @@ static bool cmdRestore(vshControl *ctl, const vshCmd *cmd) { const char *from = NULL; - bool ret = true; + bool ret = false; + int flags = 0; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -2009,12 +2031,19 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "file", &from) <= 0) return false; - if (virDomainRestore(ctl->conn, from) == 0) { - vshPrint(ctl, _("Domain restored from %s\n"), from); - } else { + if (vshCommandOptBool(cmd, "bypass-cache")) + flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + + if ((flags ? virDomainRestoreFlags(ctl->conn, from, NULL, flags) + : virDomainRestore(ctl->conn, from)) < 0) { vshError(ctl, _("Failed to restore domain from %s"), from); - ret = false; + goto cleanup; } + + vshPrint(ctl, _("Domain restored from %s\n"), from); + ret = true; + +cleanup: return ret; } @@ -2030,6 +2059,8 @@ static const vshCmdInfo info_dump[] = { static const vshCmdOptDef opts_dump[] = { {"live", VSH_OT_BOOL, 0, N_("perform a live core dump if supported")}, {"crash", VSH_OT_BOOL, 0, N_("crash the domain after core dump")}, + {"bypass-cache", VSH_OT_BOOL, 0, + N_("avoid file system cache when saving")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to dump the core")}, {NULL, 0, 0, NULL} @@ -2041,7 +2072,7 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; const char *name = NULL; const char *to = NULL; - bool ret = true; + bool ret = false; int flags = 0; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -2057,14 +2088,18 @@ cmdDump(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DUMP_LIVE; if (vshCommandOptBool (cmd, "crash")) flags |= VIR_DUMP_CRASH; + if (vshCommandOptBool(cmd, "bypass-cache")) + flags |= VIR_DUMP_BYPASS_CACHE; - if (virDomainCoreDump(dom, to, flags) == 0) { - vshPrint(ctl, _("Domain %s dumped to %s\n"), name, to); - } else { + if (virDomainCoreDump(dom, to, flags) < 0) { vshError(ctl, _("Failed to core dump domain %s to %s"), name, to); - ret = false; + goto cleanup; } + vshPrint(ctl, _("Domain %s dumped to %s\n"), name, to); + ret = true; + +cleanup: virDomainFree(dom); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 9cd2853..5659e32 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -471,9 +471,16 @@ named by I<format> to a domain XML format. Convert the file I<xml> in domain XML format to the native guest configuration format named by I<format>. -=item B<dump> I<domain-id> I<corefilepath> +=item B<dump> I<domain-id> I<corefilepath> [I<--live>] [I<--crash>] +[I<--bypass-cache>] Dumps the core of a domain to a file for analysis. +If I<--live> is specified, the domain continues to run until the core +dump is complete, rather than pausing up front. +If I<--crash> is specified, the domain is halted with a crashed status, +rather than merely left in a paused state. +If I<--bypass-cache> is specified, the save will avoid the file system +cache, although this may slow down the operation. =item B<dumpxml> I<domain-id> [I<--inactive>] [I<--security-info>] [I<--update-cpu>] @@ -508,11 +515,13 @@ except that it does some error checking. The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. -=item B<managedsave> I<domain-id> +=item B<managedsave> I<domain-id> [I<--bypass-cache>] Save and destroy (stop) a running domain, so it can be restarted from the same state at a later time. When the virsh B<start> command is next run for the domain, it will automatically be started from this saved state. +If I<--bypass-cache> is specified, the save will avoid the file system +cache, although this may slow down the operation. The B<dominfo> command can be used to query whether a domain currently has any managed save image. @@ -586,22 +595,27 @@ domain actually reboots. The exact behavior of a domain when it reboots is set by the I<on_reboot> parameter in the domain's XML definition. -=item B<restore> I<state-file> +=item B<restore> I<state-file> [I<--bypass-cache>] Restores a domain from a B<virsh save> state file. See I<save> for more info. +If I<--bypass-cache> is specified, the restore will avoid the file system +cache, although this may slow down the operation. + B<Note>: To avoid corrupting file system contents within the domain, you should not reuse the saved state file for a second B<restore> unless you have also reverted all storage volumes back to the same contents as when the state file was created. -=item B<save> I<domain-id> I<state-file> +=item B<save> I<domain-id> I<state-file> [I<--bypass-cache>] Saves a running domain (RAM, but not disk state) to a state file so that it can be restored later. Once saved, the domain will no longer be running on the system, thus the memory allocated for the domain will be free for other domains to use. B<virsh restore> restores from this state file. +If I<--bypass-cache> is specified, the save will avoid the file system +cache, although this may slow down the operation. This is roughly equivalent to doing a hibernate on a running computer, with all the same limitations. Open network connections may be @@ -787,6 +801,7 @@ The exact behavior of a domain when it shuts down is set by the I<on_shutdown> parameter in the domain's XML definition. =item B<start> I<domain-name> [I<--console>] [I<--paused>] [I<--autodestroy>] +[I<--bypass-cache>] Start a (previously defined) inactive domain, either from the last B<managedsave> state, or via a fresh boot if no managedsave state is @@ -795,7 +810,9 @@ used and supported by the driver; otherwise it will be running. If I<--console> is requested, attach to the console after creation. If I<--autodestroy> is requested, then the guest will be automatically destroyed when virsh closes its connection to libvirt, or otherwise -exits. +exits. If I<--bypass-cache> is specified, and managedsave state exists, +the restore will avoid the file system cache, although this may slow +down the operation. =item B<suspend> I<domain-id> -- 1.7.4.4

Also, migrate was missing documentation for the --xml option added in commit ec5301cb. * tools/virsh.c (cmdSave, cmdRestore): Add xml argument. * tools/virsh.pod (save, restore, migrate): Document it. --- v2: new patch tools/virsh.c | 34 ++++++++++++++++++++++++++++++++-- tools/virsh.pod | 24 ++++++++++++++++++++---- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ab75677..dece917 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1605,6 +1605,8 @@ static const vshCmdOptDef opts_save[] = { {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when saving")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to save the data")}, + {"xml", VSH_OT_STRING, 0, + N_("filename containing updated XML for the target")}, {NULL, 0, 0, NULL} }; @@ -1616,6 +1618,8 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) const char *to = NULL; bool ret = false; int flags = 0; + const char *xmlfile = NULL; + char *xml = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1626,10 +1630,20 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); + return false; + } + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; - if ((flags ? virDomainSaveFlags(dom, to, NULL, flags) + if (xmlfile && + virFileReadAll(xmlfile, 8192, &xml) < 0) + goto cleanup; + + if (((flags || xml) + ? virDomainSaveFlags(dom, to, xml, flags) : virDomainSave(dom, to)) < 0) { vshError(ctl, _("Failed to save domain %s to %s"), name, to); goto cleanup; @@ -1639,6 +1653,7 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(xml); virDomainFree(dom); return ret; } @@ -2015,6 +2030,8 @@ static const vshCmdOptDef opts_restore[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("the state to restore")}, {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when restoring")}, + {"xml", VSH_OT_STRING, 0, + N_("filename containing updated XML for the target")}, {NULL, 0, 0, NULL} }; @@ -2024,6 +2041,8 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; bool ret = false; int flags = 0; + const char *xmlfile = NULL; + char *xml = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -2034,7 +2053,17 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; - if ((flags ? virDomainRestoreFlags(ctl->conn, from, NULL, flags) + if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { + vshError(ctl, "%s", _("malformed xml argument")); + return false; + } + + if (xmlfile && + virFileReadAll(xmlfile, 8192, &xml) < 0) + goto cleanup; + + if (((flags || xml) + ? virDomainRestoreFlags(ctl->conn, from, xml, flags) : virDomainRestore(ctl->conn, from)) < 0) { vshError(ctl, _("Failed to restore domain from %s"), from); goto cleanup; @@ -2044,6 +2073,7 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(xml); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 5659e32..f2fd9ed 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -540,7 +540,7 @@ type attribute for the <domain> element of XML. =item B<migrate> [I<--live>] [I<--direct>] [I<--p2p> [I<--tunnelled>]] [I<--persistent>] [I<--undefinesource>] [I<--suspend>] [I<--copy-storage-all>] [I<--copy-storage-inc>] [I<--verbose>] I<domain-id> I<desturi> [I<migrateuri>] -[I<dname>] [I<--timeout> B<seconds>] +[I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>] Migrate domain to another host. Add I<--live> for live migration; I<--p2p> for peer-2-peer migration; I<--direct> for direct migration; or I<--tunnelled> @@ -555,7 +555,11 @@ I<--verbose> displays the progress of migration. The I<desturi> is the connection URI of the destination host, and I<migrateuri> is the migration URI, which usually can be omitted. I<dname> is used for renaming the domain to new name during migration, which -also usually can be omitted. +also usually can be omitted. Likewise, I<--xml> B<file> is usually +omitted, but can be used to supply an alternative XML file for use on +the destination to supply a larger set of changes to any host-specific +portions of the domain XML, such as accounting for naming differences +between source and destination in accessing underlying storage. I<--timeout> B<seconds> forces guest to suspend when live migration exceeds that many seconds, and @@ -595,19 +599,25 @@ domain actually reboots. The exact behavior of a domain when it reboots is set by the I<on_reboot> parameter in the domain's XML definition. -=item B<restore> I<state-file> [I<--bypass-cache>] +=item B<restore> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] Restores a domain from a B<virsh save> state file. See I<save> for more info. If I<--bypass-cache> is specified, the restore will avoid the file system cache, although this may slow down the operation. +I<--xml> B<file> is usually omitted, but can be used to supply an +alternative XML file for use on the restored guest with changes only +in the host-specific portions of the domain XML. For example, it can +be used to account for file naming differences in underlying storage +due to disk snapshots taken after the guest was saved. + B<Note>: To avoid corrupting file system contents within the domain, you should not reuse the saved state file for a second B<restore> unless you have also reverted all storage volumes back to the same contents as when the state file was created. -=item B<save> I<domain-id> I<state-file> [I<--bypass-cache>] +=item B<save> I<domain-id> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] Saves a running domain (RAM, but not disk state) to a state file so that it can be restored @@ -621,6 +631,12 @@ This is roughly equivalent to doing a hibernate on a running computer, with all the same limitations. Open network connections may be severed upon restore, as TCP timeouts may have expired. +I<--xml> B<file> is usually omitted, but can be used to supply an +alternative XML file for use on the restored guest with changes only +in the host-specific portions of the domain XML. For example, it can +be used to account for file naming differences that are planned to +be made via disk snapshots of underlying storage after the guest is saved. + Domain saved state files assume that disk images will be unchanged between the creation and restore point. For a more complete system restore point, where the disk state is saved alongside the memory -- 1.7.4.4

Rather than making the iohelper subject to a race in reopening the file, it is nicer to pass an already-open fd by inheritance. The old synopsis form must continue to work - if someone updates their libvirt package and installs a new libvirt_iohelper but without restarting the old libvirtd daemon, then the daemon can still make calls using the old syntax but the new iohelper. * src/util/iohelper.c (runIO): Split code for open... (prepare): ...to new function. (usage): Update synopsis. (main): Allow alternate calling form. * src/fdstream.c (virFDStreamOpenFileInternal): Use alternate form. --- v2: unchanged from patch 5 of v1 src/fdstream.c | 34 ++++-------- src/util/iohelper.c | 142 +++++++++++++++++++++++++++++++++++--------------- 2 files changed, 111 insertions(+), 65 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index d25b3f0..2b7812f 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -535,9 +535,17 @@ virFDStreamOpenFileInternal(virStreamPtr st, goto error; } + if (offset && + lseek(fd, offset, SEEK_SET) != offset) { + virReportSystemError(errno, + _("Unable to seek %s to %llu"), + path, offset); + goto error; + } + /* Thanks to the POSIX i/o model, we can't reliably get * non-blocking I/O on block devs/regular files. To - * support those we need to fork a helper process todo + * support those we need to fork a helper process to do * the I/O so we just have a fifo. Or use AIO :-( */ if ((st->flags & VIR_STREAM_NONBLOCK) && @@ -545,14 +553,13 @@ virFDStreamOpenFileInternal(virStreamPtr st, !S_ISFIFO(sb.st_mode))) { int childfd; - if ((oflags & O_RDWR) == O_RDWR) { + if ((oflags & O_ACCMODE) == O_RDWR) { streamsReportError(VIR_ERR_INTERNAL_ERROR, _("%s: Cannot request read and write flags together"), path); goto error; } - VIR_FORCE_CLOSE(fd); if (pipe(fds) < 0) { virReportSystemError(errno, "%s", _("Unable to create pipe")); @@ -562,18 +569,9 @@ virFDStreamOpenFileInternal(virStreamPtr st, cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", path, NULL); - virCommandAddArgFormat(cmd, "%d", oflags); - virCommandAddArgFormat(cmd, "%d", mode); - virCommandAddArgFormat(cmd, "%llu", offset); virCommandAddArgFormat(cmd, "%llu", length); - virCommandAddArgFormat(cmd, "%u", delete); - - /* when running iohelper we don't want to delete file now, - * because a race condition may occur in which we delete it - * before iohelper even opens it. We want iohelper to remove - * the file instead. - */ - delete = false; + virCommandTransferFD(cmd, fd); + virCommandAddArgFormat(cmd, "%d", fd); if (oflags == O_RDONLY) { childfd = fds[1]; @@ -590,14 +588,6 @@ virFDStreamOpenFileInternal(virStreamPtr st, goto error; VIR_FORCE_CLOSE(childfd); - } else { - if (offset && - lseek(fd, offset, SEEK_SET) != offset) { - virReportSystemError(errno, - _("Unable to seek %s to %llu"), - path, offset); - goto error; - } } if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 6a9f355..502bce5 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -42,19 +42,11 @@ #define VIR_FROM_THIS VIR_FROM_STORAGE -static int runIO(const char *path, - int oflags, - int mode, - unsigned long long offset, - unsigned long long length) +static int +prepare(const char *path, int oflags, int mode, + unsigned long long offset) { - char *buf = NULL; - size_t buflen = 1024*1024; - int fd; - int ret = -1; - int fdin, fdout; - const char *fdinname, *fdoutname; - unsigned long long total = 0; + int fd = -1; if (oflags & O_CREAT) { fd = open(path, oflags, mode); @@ -70,10 +62,25 @@ static int runIO(const char *path, if (lseek(fd, offset, SEEK_SET) < 0) { virReportSystemError(errno, _("Unable to seek %s to %llu"), path, offset); + VIR_FORCE_CLOSE(fd); goto cleanup; } } +cleanup: + return fd; +} + +static int +runIO(const char *path, int fd, int oflags, unsigned long long length) +{ + char *buf = NULL; + size_t buflen = 1024*1024; + int ret = -1; + int fdin, fdout; + const char *fdinname, *fdoutname; + unsigned long long total = 0; + if (VIR_ALLOC_N(buf, buflen) < 0) { virReportOOMError(); goto cleanup; @@ -138,61 +145,109 @@ cleanup: return ret; } -int main(int argc, char **argv) +static const char *program_name; + +ATTRIBUTE_NORETURN static void +usage(int status) +{ + if (status) { + fprintf(stderr, _("%s: try --help for more details"), program_name); + } else { + printf(_("Usage: %s FILENAME OFLAGS MODE OFFSET LENGTH DELETE\n" + " or: %s FILENAME LENGTH FD\n"), + program_name, program_name); + } + exit(status); +} + +int +main(int argc, char **argv) { const char *path; virErrorPtr err; unsigned long long offset; unsigned long long length; - int oflags; + int oflags = -1; int mode; - unsigned int delete; + unsigned int delete = 0; + int fd = -1; + int lengthIndex = 0; + + program_name = argv[0]; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || textdomain(PACKAGE) == NULL) { - fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + fprintf(stderr, _("%s: initialization failed\n"), program_name); exit(EXIT_FAILURE); } if (virThreadInitialize() < 0 || virErrorInitialize() < 0 || virRandomInitialize(time(NULL) ^ getpid())) { - fprintf(stderr, _("%s: initialization failed\n"), argv[0]); - exit(EXIT_FAILURE); - } - - if (argc != 7) { - fprintf(stderr, _("%s: syntax FILENAME FLAGS MODE OFFSET LENGTH DELETE\n"), argv[0]); + fprintf(stderr, _("%s: initialization failed\n"), program_name); exit(EXIT_FAILURE); } path = argv[1]; - if (virStrToLong_i(argv[2], NULL, 10, &oflags) < 0) { - fprintf(stderr, _("%s: malformed file flags %s"), argv[0], argv[2]); - exit(EXIT_FAILURE); - } - - if (virStrToLong_i(argv[3], NULL, 10, &mode) < 0) { - fprintf(stderr, _("%s: malformed file mode %s"), argv[0], argv[3]); - exit(EXIT_FAILURE); + if (argc > 1 && STREQ(argv[1], "--help")) + usage(EXIT_SUCCESS); + if (argc == 7) { /* FILENAME OFLAGS MODE OFFSET LENGTH DELETE */ + lengthIndex = 5; + if (virStrToLong_i(argv[2], NULL, 10, &oflags) < 0) { + fprintf(stderr, _("%s: malformed file flags %s"), + program_name, argv[2]); + exit(EXIT_FAILURE); + } + if (virStrToLong_i(argv[3], NULL, 10, &mode) < 0) { + fprintf(stderr, _("%s: malformed file mode %s"), + program_name, argv[3]); + exit(EXIT_FAILURE); + } + if (virStrToLong_ull(argv[4], NULL, 10, &offset) < 0) { + fprintf(stderr, _("%s: malformed file offset %s"), + program_name, argv[4]); + exit(EXIT_FAILURE); + } + if (argc == 7 && virStrToLong_ui(argv[6], NULL, 10, &delete) < 0) { + fprintf(stderr, _("%s: malformed delete flag %s"), + program_name, argv[6]); + exit(EXIT_FAILURE); + } + fd = prepare(path, oflags, mode, offset); + } else if (argc == 4) { /* FILENAME LENGTH FD */ + lengthIndex = 2; + if (virStrToLong_i(argv[3], NULL, 10, &fd) < 0) { + fprintf(stderr, _("%s: malformed fd %s"), + program_name, argv[3]); + exit(EXIT_FAILURE); + } +#ifdef F_GETFL + oflags = fcntl(fd, F_GETFL); +#else + /* Stupid mingw. */ + if (fd == STDIN_FILENO) + oflags = O_RDONLY; + else if (fd == STDOUT_FILENO) + oflags = O_WRONLY; +#endif + if (oflags < 0) { + fprintf(stderr, _("%s: unable to determine access mode of fd %d"), + program_name, fd); + exit(EXIT_FAILURE); + } + } else { /* unknown argc pattern */ + usage(EXIT_FAILURE); } - if (virStrToLong_ull(argv[4], NULL, 10, &offset) < 0) { - fprintf(stderr, _("%s: malformed file offset %s"), argv[0], argv[4]); - exit(EXIT_FAILURE); - } - if (virStrToLong_ull(argv[5], NULL, 10, &length) < 0) { - fprintf(stderr, _("%s: malformed file length %s"), argv[0], argv[5]); - exit(EXIT_FAILURE); - } - if (virStrToLong_ui(argv[6], NULL, 10, &delete) < 0) { - fprintf(stderr, _("%s: malformed delete flag %s"), argv[0],argv[6]); + if (virStrToLong_ull(argv[lengthIndex], NULL, 10, &length) < 0) { + fprintf(stderr, _("%s: malformed file length %s"), + program_name, argv[lengthIndex]); exit(EXIT_FAILURE); } - if (runIO(path, oflags, mode, offset, length) < 0) + if (fd < 0 || runIO(path, fd, oflags, length) < 0) goto error; if (delete) @@ -203,9 +258,10 @@ int main(int argc, char **argv) error: err = virGetLastError(); if (err) { - fprintf(stderr, "%s: %s\n", argv[0], err->message); + fprintf(stderr, "%s: %s\n", program_name, err->message); } else { - fprintf(stderr, _("%s: unknown failure with %s\n"), argv[0], path); + fprintf(stderr, _("%s: unknown failure with %s\n"), + program_name, path); } exit(EXIT_FAILURE); } -- 1.7.4.4

Required for a coming patch where iohelper will operate on O_DIRECT fds. There, the user-space memory must be aligned to file system boundaries (at least 512, but using page-aligned works better, and some file systems prefer 64k). Made tougher by the fact that VIR_ALLOC won't work on void *, but posix_memalign won't work on char * and isn't available everywhere. This patch makes some simplifying assumptions - namely, output to an O_DIRECT fd will only be attempted on an empty seekable file (hence, no need to worry about preserving existing data on a partial block, and ftruncate will work to undo the effects of having to round up the size of the last block written), and input from an O_DIRECT fd will only be attempted on a complete seekable file with the only possible short read at EOF. * configure.ac (AC_CHECK_FUNCS_ONCE): Check for posix_memalign. * src/util/iohelper.c (runIO): Use aligned memory, and handle quirks of O_DIRECT on last write. --- v2: merge patch 6 and 15 of v1. Sorry, I didn't add a testsuite of this yet, but agree that it would be a nice project configure.ac | 6 ++-- src/util/iohelper.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index e9d5be4..9e39f44 100644 --- a/configure.ac +++ b/configure.ac @@ -121,9 +121,9 @@ AC_CHECK_SIZEOF([long]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions -AC_CHECK_FUNCS_ONCE([cfmakeraw regexec sched_getaffinity getuid getgid \ - geteuid initgroups posix_fallocate mmap kill \ - getmntent_r getgrnam_r getpwuid_r]) +AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ + getpwuid_r getuid initgroups kill mmap posix_fallocate posix_memalign \ + regexec sched_getaffinity]) dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 502bce5..9e7bbde 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -74,17 +74,32 @@ cleanup: static int runIO(const char *path, int fd, int oflags, unsigned long long length) { - char *buf = NULL; + void *base = NULL; /* Location to be freed */ + char *buf = NULL; /* Aligned location within base */ size_t buflen = 1024*1024; + intptr_t alignMask = 64*1024 - 1; int ret = -1; int fdin, fdout; const char *fdinname, *fdoutname; unsigned long long total = 0; + bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); + bool shortRead = false; /* true if we hit a short read */ + off_t end = 0; - if (VIR_ALLOC_N(buf, buflen) < 0) { +#if HAVE_POSIX_MEMALIGN + if (posix_memalign(&base, alignMask + 1, buflen)) { virReportOOMError(); goto cleanup; } + buf = base; +#else + if (VIR_ALLOC_N(buf, buflen + alignMask) < 0) { + virReportOOMError(); + goto cleanup; + } + base = buf; + buf = (char *) (((intptr_t) base + alignMask) & alignMask); +#endif switch (oflags & O_ACCMODE) { case O_RDONLY: @@ -92,12 +107,26 @@ runIO(const char *path, int fd, int oflags, unsigned long long length) fdinname = path; fdout = STDOUT_FILENO; fdoutname = "stdout"; + /* To make the implementation simpler, we give up on any + * attempt to use O_DIRECT in a non-trivial manner. */ + if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0 || length)) { + virReportSystemError(end < 0 ? errno : EINVAL, "%s", + _("O_DIRECT read needs entire seekable file")); + goto cleanup; + } break; case O_WRONLY: fdin = STDIN_FILENO; fdinname = "stdin"; fdout = fd; fdoutname = path; + /* To make the implementation simpler, we give up on any + * attempt to use O_DIRECT in a non-trivial manner. */ + if (direct && (end = lseek(fd, 0, SEEK_END)) != 0) { + virReportSystemError(end < 0 ? errno : EINVAL, "%s", + _("O_DIRECT write needs empty seekable file")); + goto cleanup; + } break; case O_RDWR: @@ -124,12 +153,29 @@ runIO(const char *path, int fd, int oflags, unsigned long long length) } if (got == 0) break; /* End of file before end of requested data */ + if (got < buflen || (buflen & alignMask)) { + /* O_DIRECT can handle at most one short read, at end of file */ + if (direct && shortRead) { + virReportSystemError(EINVAL, "%s", + _("Too many short reads for O_DIRECT")); + } + shortRead = true; + } total += got; + if (fdout == fd && direct && shortRead) { + end = total; + memset(buf + got, 0, buflen - got); + got = (got + alignMask) & ~alignMask; + } if (safewrite(fdout, buf, got) < 0) { virReportSystemError(errno, _("Unable to write %s"), fdoutname); goto cleanup; } + if (end && ftruncate(fd, end) < 0) { + virReportSystemError(errno, _("Unable to truncate %s"), fdoutname); + goto cleanup; + } } ret = 0; @@ -141,7 +187,7 @@ cleanup: ret = -1; } - VIR_FREE(buf); + VIR_FREE(base); return ret; } -- 1.7.4.4

O_DIRECT has stringent requirements. Rather than make lots of changes at each site that wants to use O_DIRECT, it is easier to offload the work through a helper process that mirrors the I/O between a pipe and the actual direct fd, so that the other end of the pipe no longer has to worry about constraints. Plus, if the kernel ever gains better posix_fadvise support, then we only have to touch a single file to let all callers benefit from a more efficient way to avoid file system caching. * src/util/virfile.h (virFileDirectFdFlag, virFileDirectFdNew) (virFileDirectFdClose, virFileDirectFdFree): New prototypes. * src/util/virdirect.c: Implement new wrapper object. * src/libvirt_private.syms (virfile.h): Export new symbols. * cfg.mk (useless_free_options): Add to list. * po/POTFILES.in: Add new translations. --- v2: move everything into virfile.c rather than creating a new file, with renames everywhere. Fail rather than silently ignore systems where O_DIRECT is 0. Add comments to take advantage of posix_fadvise rather than O_DIRECT if the Linux kernel ever gets fixed. Add virFileDirectFdFlag as a better probe of which cache-bypass mechanism is actually being used. Better documentation. cfg.mk | 1 + po/POTFILES.in | 1 + src/libvirt_private.syms | 4 + src/util/virfile.c | 175 +++++++++++++++++++++++++++++++++++++++++++++- src/util/virfile.h | 15 ++++ 5 files changed, 195 insertions(+), 1 deletions(-) diff --git a/cfg.mk b/cfg.mk index 0a624f1..773a3df 100644 --- a/cfg.mk +++ b/cfg.mk @@ -119,6 +119,7 @@ useless_free_options = \ --name=virDomainSoundDefFree \ --name=virDomainVideoDefFree \ --name=virDomainWatchdogDefFree \ + --name=virFileDirectFdFree \ --name=virHashFree \ --name=virInterfaceDefFree \ --name=virInterfaceIpDefFree \ diff --git a/po/POTFILES.in b/po/POTFILES.in index 5782cbf..7bb08b0 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -122,6 +122,7 @@ src/util/storage_file.c src/util/sysinfo.c src/util/util.c src/util/viraudit.c +src/util/virfile.c src/util/virterror.c src/util/xml.c src/vbox/vbox_MSCOMGlue.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d42e0a0..0bab64d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1082,6 +1082,10 @@ virAuditSend; # virfile.h virFileClose; +virFileDirectFdClose; +virFileDirectFdFlag; +virFileDirectFdFree; +virFileDirectFdNew; virFileFclose; virFileFdopen; diff --git a/src/util/virfile.c b/src/util/virfile.c index 6576921..8b32518 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -23,10 +23,24 @@ */ #include <config.h> +#include "internal.h" +#include "virfile.h" + +#include <fcntl.h> +#include <sys/stat.h> #include <unistd.h> -#include "virfile.h" +#include "command.h" +#include "configmake.h" +#include "memory.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE +#define virFileError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + int virFileClose(int *fdptr, bool preserve_errno) { @@ -78,3 +92,162 @@ FILE *virFileFdopen(int *fdptr, const char *mode) return file; } + + +/* Opaque type for managing a wrapper around an O_DIRECT fd. For now, + * read-write is not supported, just a single direction. */ +struct _virFileDirectFd { + virCommandPtr cmd; /* Child iohelper process to do the I/O. */ +}; + +/** + * virFileDirectFdFlag: + * + * Returns 0 if the kernel can avoid file system cache pollution + * without any additional flags, O_DIRECT if the original fd must be + * opened in direct mode, or -1 if there is no support for bypassing + * the file system cache. + */ +int +virFileDirectFdFlag(void) +{ + /* XXX For now, Linux posix_fadvise is not powerful enough to + * avoid O_DIRECT. */ + return O_DIRECT ? O_DIRECT : -1; +} + +/** + * virFileDirectFdNew: + * @fd: pointer to fd to wrap + * @name: name of fd, for diagnostics + * + * Update *FD (created with virFileDirectFdFlag() among the flags to + * open()) to ensure that all I/O to that file will bypass the system + * cache. This must be called after open() and optional fchown() or + * fchmod(), but before any seek or I/O, and only on seekable fd. The + * file must be O_RDONLY (to read the entire existing file) or + * O_WRONLY (to write to an empty file). In some cases, *FD is + * changed to a non-seekable pipe; in this case, the caller must not + * do anything further with the original fd. + * + * On success, the new wrapper object is returned, which must be later + * freed with virFileDirectFdFree(). On failure, *FD is unchanged, an + * error message is output, and NULL is returned. + */ +virFileDirectFdPtr +virFileDirectFdNew(int *fd, const char *name) +{ + virFileDirectFdPtr ret = NULL; + bool output = false; + int pipefd[2] = { -1, -1 }; + int mode = -1; + + /* XXX support posix_fadvise rather than spawning a child, if the + * kernel support for that is decent enough. */ + + if (!O_DIRECT) { + virFileError(VIR_ERR_INTERNAL_ERROR, "%s", + _("O_DIRECT unsupported on this platform")); + return NULL; + } + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(); + return NULL; + } + +#ifdef F_GETFL + /* Mingw lacks F_GETFL, but it also lacks O_DIRECT so didn't get + * here in the first place. All other platforms reach this + * line. */ + mode = fcntl(*fd, F_GETFL); +#endif + + if (mode < 0) { + virFileError(VIR_ERR_INTERNAL_ERROR, _("invalid fd %d for %s"), + *fd, name); + goto error; + } else if ((mode & O_ACCMODE) == O_WRONLY) { + output = true; + } else if ((mode & O_ACCMODE) != O_RDONLY) { + virFileError(VIR_ERR_INTERNAL_ERROR, _("unexpected mode %x for %s"), + mode & O_ACCMODE, name); + goto error; + } + + if (pipe2(pipefd, O_CLOEXEC) < 0) { + virFileError(VIR_ERR_INTERNAL_ERROR, + _("unable to create pipe for %s"), name); + goto error; + } + + ret->cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", + name, "0", NULL); + if (output) { + virCommandSetInputFD(ret->cmd, pipefd[0]); + virCommandSetOutputFD(ret->cmd, fd); + virCommandAddArg(ret->cmd, "1"); + } else { + virCommandSetInputFD(ret->cmd, *fd); + virCommandSetOutputFD(ret->cmd, &pipefd[1]); + virCommandAddArg(ret->cmd, "0"); + } + + if (virCommandRunAsync(ret->cmd, NULL) < 0) + goto error; + + if (VIR_CLOSE(pipefd[!output]) < 0) { + virFileError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to close pipe")); + goto error; + } + + VIR_FORCE_CLOSE(*fd); + *fd = pipefd[output]; + return ret; + +error: + VIR_FORCE_CLOSE(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[1]); + virFileDirectFdFree(ret); + return NULL; +} + +/** + * virFileDirectFdClose: + * @dfd: direct fd wrapper, or NULL + * + * If DFD is valid, then ensure that I/O has completed, which may + * include reaping a child process. Return 0 if all data for the + * wrapped fd is complete, or -1 on failure with an error emitted. + * This function intentionally returns 0 when DFD is NULL, so that + * callers can conditionally create a virFileDirectFd wrapper but + * unconditionally call the cleanup code. To avoid deadlock, only + * call this after closing the fd resulting from virFileDirectFdNew(). + */ +int +virFileDirectFdClose(virFileDirectFdPtr dfd) +{ + if (!dfd) + return 0; + + return virCommandWait(dfd->cmd, NULL); +} + +/** + * virFileDirectFdFree: + * @dfd: direct fd wrapper, or NULL + * + * Free all remaining resources associated with DFD. If + * virFileDirectFdClose() was not previously called, then this may + * discard some previous I/O. To avoid deadlock, only call this after + * closing the fd resulting from virFileDirectFdNew(). + */ +void +virFileDirectFdFree(virFileDirectFdPtr dfd) +{ + if (!dfd) + return; + + virCommandFree(dfd->cmd); + VIR_FREE(dfd); +} diff --git a/src/util/virfile.h b/src/util/virfile.h index d11f902..0906568 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -50,4 +50,19 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; # define VIR_FORCE_CLOSE(FD) ignore_value(virFileClose(&(FD), true)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true)) +/* Opaque type for managing a wrapper around an O_DIRECT fd. */ +struct _virFileDirectFd; + +typedef struct _virFileDirectFd virFileDirectFd; +typedef virFileDirectFd *virFileDirectFdPtr; + +int virFileDirectFdFlag(void); + +virFileDirectFdPtr virFileDirectFdNew(int *fd, const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int virFileDirectFdClose(virFileDirectFdPtr dfd); + +void virFileDirectFdFree(virFileDirectFdPtr dfd); + #endif /* __VIR_FILES_H */ -- 1.7.4.4

Wire together the previous patches to support file system cache bypass during API save/restore requests in qemu. * src/qemu/qemu_driver.c (qemuDomainSaveInternal, doCoreDump) (qemudDomainObjStart, qemuDomainSaveImageOpen, qemuDomainObjRestore) (qemuDomainObjStart): Add parameter. (qemuDomainSaveFlags, qemuDomainManagedSave, qemudDomainCoreDump) (processWatchdogEvent, qemudDomainStartWithFlags, qemuAutostartDomain) (qemuDomainRestoreFlags): Update callers. --- v2: merge 8 and 16 of v1, and rebase to renamed flags and new virFileDirectFdFlag call done earlier in this series src/qemu/qemu_driver.c | 161 ++++++++++++++++++++++++++++++++++-------------- 1 files changed, 115 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 95f30c3..f6166c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -116,11 +116,12 @@ static void processWatchdogEvent(void *data, void *opaque); static int qemudShutdown(void); -static int qemudDomainObjStart(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - bool start_paused, - bool autodestroy); +static int qemuDomainObjStart(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_paused, + bool autodestroy, + bool bypass_cache); static int qemudDomainGetMaxVcpus(virDomainPtr dom); @@ -148,9 +149,11 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq vm->def->name, err ? err->message : _("unknown error")); } else { + /* XXX need to wire bypass-cache autostart into qemu.conf */ if (vm->autostart && !virDomainObjIsActive(vm) && - qemudDomainObjStart(data->conn, data->driver, vm, false, false) < 0) { + qemuDomainObjStart(data->conn, data->driver, vm, + false, false, false) < 0) { err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, @@ -2110,7 +2113,7 @@ qemuCompressProgramName(int compress) static int qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainObjPtr vm, const char *path, - int compressed) + int compressed, bool bypass_cache) { char *xml = NULL; struct qemud_save_header header; @@ -2125,6 +2128,8 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, int fd = -1; uid_t uid = getuid(); gid_t gid = getgid(); + int directFlag = 0; + virFileDirectFdPtr directFd = NULL; memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -2207,15 +2212,23 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, /* Obtain the file handle. */ /* First try creating the file as root */ + if (bypass_cache) { + directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + goto endjob; + } + } if (!is_reg) { - fd = open(path, O_WRONLY | O_TRUNC); + fd = open(path, O_WRONLY | O_TRUNC | directFlag); if (fd < 0) { virReportSystemError(errno, _("unable to open %s"), path); goto endjob; } } else { - if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY, - S_IRUSR|S_IWUSR, + int oflags = O_CREAT | O_TRUNC | O_WRONLY | directFlag; + if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid, 0)) < 0) { /* If we failed as root, and the error was permission-denied (EACCES or EPERM), assume it's on a network-connected share @@ -2261,7 +2274,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, /* Retry creating the file as driver->user */ - if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY, + if ((fd = virFileOpenAs(path, oflags, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, driver->user, driver->group, VIR_FILE_OPEN_AS_UID)) < 0) { @@ -2279,6 +2292,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } } + if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) + goto endjob; + /* Write header to file, followed by XML */ if (qemuDomainSaveHeader(fd, path, xml, &header) < 0) { VIR_FORCE_CLOSE(fd); @@ -2294,6 +2310,8 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virReportSystemError(errno, _("unable to close %s"), path); goto endjob; } + if (virFileDirectFdClose(directFd) < 0) + goto endjob; ret = 0; @@ -2326,6 +2344,7 @@ endjob: cleanup: VIR_FORCE_CLOSE(fd); + virFileDirectFdFree(directFd); VIR_FREE(xml); if (ret != 0 && is_reg) unlink(path); @@ -2361,7 +2380,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, int ret = -1; virDomainObjPtr vm = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); if (dxml) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("xml modification unsupported")); @@ -2403,7 +2422,8 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, goto cleanup; } - ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed); + ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); vm = NULL; cleanup: @@ -2441,7 +2461,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) int ret = -1; int compressed; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2466,7 +2486,8 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_INFO("Saving state to %s", name); compressed = QEMUD_SAVE_FORMAT_RAW; - ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed); + ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); vm = NULL; cleanup: @@ -2550,18 +2571,33 @@ static int doCoreDump(struct qemud_driver *driver, virDomainObjPtr vm, const char *path, - enum qemud_save_formats compress) + enum qemud_save_formats compress, + bool bypass_cache) { int fd = -1; int ret = -1; + virFileDirectFdPtr directFd = NULL; + int directFlag = 0; /* Create an empty file with appropriate ownership. */ - if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { + if (bypass_cache) { + directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + goto cleanup; + } + } + if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | directFlag, + S_IRUSR | S_IWUSR)) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, _("failed to create '%s'"), path); goto cleanup; } + if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) + goto cleanup; + if (qemuMigrationToFile(driver, vm, fd, 0, path, qemuCompressProgramName(compress), true, false) < 0) goto cleanup; @@ -2572,11 +2608,14 @@ doCoreDump(struct qemud_driver *driver, path); goto cleanup; } + if (virFileDirectFdClose(directFd) < 0) + goto cleanup; ret = 0; cleanup: VIR_FORCE_CLOSE(fd); + virFileDirectFdClose(directFd); if (ret != 0) unlink(path); return ret; @@ -2620,7 +2659,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, int ret = -1; virDomainEventPtr event = NULL; - virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH, -1); + virCheckFlags(VIR_DUMP_LIVE | VIR_DUMP_CRASH | VIR_DUMP_BYPASS_CACHE, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2661,7 +2700,8 @@ static int qemudDomainCoreDump(virDomainPtr dom, } } - ret = doCoreDump(driver, vm, path, getCompressionType(driver)); + ret = doCoreDump(driver, vm, path, getCompressionType(driver), + (flags & VIR_DUMP_BYPASS_CACHE) != 0); if (ret < 0) goto endjob; @@ -2832,10 +2872,9 @@ static void processWatchdogEvent(void *data, void *opaque) goto endjob; } - ret = doCoreDump(driver, - wdEvent->vm, - dumpfile, - getCompressionType(driver)); + /* XXX wire up qemu.conf to support bypass-cache dumps */ + ret = doCoreDump(driver, wdEvent->vm, dumpfile, + getCompressionType(driver), false); if (ret < 0) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Dump failed")); @@ -3572,18 +3611,29 @@ cleanup: return ret; } -static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) +static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, - struct qemud_save_header *ret_header) + struct qemud_save_header *ret_header, + bool bypass_cache, virFileDirectFdPtr *directFd) { int fd; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; + int directFlag = 0; - if ((fd = virFileOpenAs(path, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) { + if (bypass_cache) { + directFlag = virFileDirectFdFlag(); + if (directFlag < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("bypass cache unsupported by this system")); + goto error; + } + } + if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + getuid(), getgid(), 0)) < 0) { if ((fd != -EACCES && fd != -EPERM) || driver->user == getuid()) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3593,7 +3643,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, /* Opening as root failed, but qemu runs as a different user * that might have better luck. */ - if ((fd = virFileOpenAs(path, O_RDONLY, 0, + if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, driver->user, driver->group, VIR_FILE_OPEN_AS_UID)) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3601,6 +3651,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, goto error; } } + if (bypass_cache && (*directFd = virFileDirectFdNew(&fd, path)) == NULL) + goto error; if (saferead(fd, &header, sizeof(header)) != sizeof(header)) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3779,8 +3831,9 @@ qemuDomainRestoreFlags(virConnectPtr conn, int fd = -1; int ret = -1; struct qemud_save_header header; + virFileDirectFdPtr directFd = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); if (dxml) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("xml modification unsupported")); @@ -3789,7 +3842,9 @@ qemuDomainRestoreFlags(virConnectPtr conn, qemuDriverLock(driver); - fd = qemuDomainSaveImageOpen(driver, path, &def, &header); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + &directFd); if (fd < 0) goto cleanup; @@ -3808,6 +3863,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, goto cleanup; ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + if (virFileDirectFdClose(directFd) < 0) + VIR_WARN("Failed to close %s", path); if (qemuDomainObjEndJob(driver, vm) == 0) vm = NULL; @@ -3819,6 +3876,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); + virFileDirectFdFree(directFd); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -3836,14 +3894,17 @@ static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - const char *path) + const char *path, + bool bypass_cache) { virDomainDefPtr def = NULL; int fd = -1; int ret = -1; struct qemud_save_header header; + virFileDirectFdPtr directFd = NULL; - fd = qemuDomainSaveImageOpen(driver, path, &def, &header); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, + bypass_cache, &directFd); if (fd < 0) goto cleanup; @@ -3865,10 +3926,13 @@ qemuDomainObjRestore(virConnectPtr conn, def = NULL; ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + if (virFileDirectFdClose(directFd) < 0) + VIR_WARN("Failed to close %s", path); cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); + virFileDirectFdFree(directFd); return ret; } @@ -4082,11 +4146,13 @@ static int qemudNumDefinedDomains(virConnectPtr conn) { } -static int qemudDomainObjStart(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - bool start_paused, - bool autodestroy) +static int +qemuDomainObjStart(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_paused, + bool autodestroy, + bool bypass_cache) { int ret = -1; char *managed_save; @@ -4101,7 +4167,8 @@ static int qemudDomainObjStart(virConnectPtr conn, goto cleanup; if (virFileExists(managed_save)) { - ret = qemuDomainObjRestore(conn, driver, vm, managed_save); + ret = qemuDomainObjRestore(conn, driver, vm, managed_save, + bypass_cache); if ((ret == 0) && (unlink(managed_save) < 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); @@ -4127,14 +4194,15 @@ cleanup: } static int -qemudDomainStartWithFlags(virDomainPtr dom, unsigned int flags) +qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; virCheckFlags(VIR_DOMAIN_START_PAUSED | - VIR_DOMAIN_START_AUTODESTROY, -1); + VIR_DOMAIN_START_AUTODESTROY | + VIR_DOMAIN_START_BYPASS_CACHE, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4156,9 +4224,10 @@ qemudDomainStartWithFlags(virDomainPtr dom, unsigned int flags) goto endjob; } - if (qemudDomainObjStart(dom->conn, driver, vm, - (flags & VIR_DOMAIN_START_PAUSED) != 0, - (flags & VIR_DOMAIN_START_AUTODESTROY) != 0) < 0) + if (qemuDomainObjStart(dom->conn, driver, vm, + (flags & VIR_DOMAIN_START_PAUSED) != 0, + (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, + (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0) < 0) goto endjob; ret = 0; @@ -4175,9 +4244,9 @@ cleanup: } static int -qemudDomainStart(virDomainPtr dom) +qemuDomainStart(virDomainPtr dom) { - return qemudDomainStartWithFlags(dom, 0); + return qemuDomainStartWithFlags(dom, 0); } static int @@ -8616,8 +8685,8 @@ static virDriver qemuDriver = { .domainXMLToNative = qemuDomainXMLToNative, /* 0.6.4 */ .listDefinedDomains = qemudListDefinedDomains, /* 0.2.0 */ .numOfDefinedDomains = qemudNumDefinedDomains, /* 0.2.0 */ - .domainCreate = qemudDomainStart, /* 0.2.0 */ - .domainCreateWithFlags = qemudDomainStartWithFlags, /* 0.8.2 */ + .domainCreate = qemuDomainStart, /* 0.2.0 */ + .domainCreateWithFlags = qemuDomainStartWithFlags, /* 0.8.2 */ .domainDefineXML = qemudDomainDefine, /* 0.2.0 */ .domainUndefine = qemudDomainUndefine, /* 0.2.0 */ .domainUndefineFlags = qemuDomainUndefineFlags, /* 0.9.4 */ -- 1.7.4.4

When auto-dumping a domain on crash events, or autostarting a domain with managed save state, let the user configure whether to imply the bypass cache flag. * src/qemu/qemu.conf (auto_dump_bypass_cache, auto_start_bypass_cache): Document new variables. * src/qemu/libvirtd_qemu.aug (vnc_entry): Let augeas parse them. * src/qemu/qemu_conf.h (qemud_driver): Store new preferences. * src/qemu/qemu_conf.c (qemudLoadDriverConfig): Parse them. * src/qemu/qemu_driver.c (processWatchdogEvent, qemuAutostartDomain): Honor them. --- v2: merge 9 and 16 of v1, rename direct to bypass_cache src/qemu/libvirtd_qemu.aug | 2 ++ src/qemu/qemu.conf | 16 ++++++++++++++++ src/qemu/qemu_conf.c | 10 +++++++++- src/qemu/qemu_conf.h | 5 ++++- src/qemu/qemu_driver.c | 8 ++++---- 5 files changed, 35 insertions(+), 6 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 66858ae..d018ac2 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -41,6 +41,8 @@ module Libvirtd_qemu = | str_entry "save_image_format" | str_entry "dump_image_format" | str_entry "auto_dump_path" + | bool_entry "auto_dump_bypass_cache" + | bool_entry "auto_start_bypass_cache" | str_entry "hugetlbfs_mount" | bool_entry "relaxed_acs_check" | bool_entry "vnc_allow_host_audio" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 934f99b..145062c 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -208,6 +208,22 @@ # # auto_dump_path = "/var/lib/libvirt/qemu/dump" +# When a domain is configured to be auto-dumped, enabling this flag +# has the same effect as using the VIR_DUMP_BYPASS_CACHE flag with the +# virDomainCoreDump API. That is, the system will avoid using the +# file system cache while writing the dump file, but may cause +# slower operation. +# +# auto_dump_bypass_cache = 0 + +# When a domain is configured to be auto-started, enabling this flag +# has the same effect as using the VIR_DOMAIN_START_BYPASS_CACHE flag +# with the virDomainCreateWithFlags API. That is, the system will +# avoid using the file system cache when restoring any managed state +# file, but may cause slower operation. +# +# auto_start_bypass_cache = 0 + # If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is # unspecified here, determination of a host mount point in /proc/mounts diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 4a17a55..6efca6b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1,7 +1,7 @@ /* * qemu_conf.c: QEMU configuration management * - * Copyright (C) 2006, 2007, 2008, 2009, 2010 Red Hat, Inc. + * Copyright (C) 2006-2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -378,6 +378,14 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } } + p = virConfGetValue (conf, "auto_dump_bypass_cache"); + CHECK_TYPE ("auto_dump_bypass_cache", VIR_CONF_LONG); + if (p) driver->autoDumpBypassCache = true; + + p = virConfGetValue (conf, "auto_start_bypass_cache"); + CHECK_TYPE ("auto_start_bypass_cache", VIR_CONF_LONG); + if (p) driver->autoStartBypassCache = true; + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index fa4c607..0a60d32 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -1,7 +1,7 @@ /* * qemu_conf.h: QEMU configuration management * - * Copyright (C) 2006-2007, 2009-2010 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -119,6 +119,9 @@ struct qemud_driver { char *dumpImageFormat; char *autoDumpPath; + bool autoDumpBypassCache; + + bool autoStartBypassCache; pciDeviceList *activePciHostdevs; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f6166c2..73d2938 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -149,11 +149,11 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq vm->def->name, err ? err->message : _("unknown error")); } else { - /* XXX need to wire bypass-cache autostart into qemu.conf */ if (vm->autostart && !virDomainObjIsActive(vm) && qemuDomainObjStart(data->conn, data->driver, vm, - false, false, false) < 0) { + false, false, + data->driver->autoStartBypassCache) < 0) { err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, @@ -2872,9 +2872,9 @@ static void processWatchdogEvent(void *data, void *opaque) goto endjob; } - /* XXX wire up qemu.conf to support bypass-cache dumps */ ret = doCoreDump(driver, wdEvent->vm, dumpfile, - getCompressionType(driver), false); + getCompressionType(driver), + driver->autoDumpBypassCache); if (ret < 0) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Dump failed")); -- 1.7.4.4

libvirt-guests is a perfect use case for bypassing the file system cache - lots of filesystem traffic done at system shutdown, where caching is pointless, and startup, where reading large files only once just gets in the way. Make this a configurable option in the init script, but defaulting to existing behavior. * tools/libvirt-guests.sysconf (BYPASS_CACHE): New variable. * tools/libvirt-guests.init.sh (start, suspend_guest): Use it. --- merge 10 and 18 of v1, rename SAVE_DIRECT to BYPASS_CACHE tools/libvirt-guests.init.sh | 10 ++++++++-- tools/libvirt-guests.sysconf | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh index 30f957a..367177e 100644 --- a/tools/libvirt-guests.init.sh +++ b/tools/libvirt-guests.init.sh @@ -43,6 +43,7 @@ ON_BOOT=start ON_SHUTDOWN=suspend SHUTDOWN_TIMEOUT=0 START_DELAY=0 +BYPASS_CACHE=0 test -f "$sysconfdir"/sysconfig/libvirt-guests && . "$sysconfdir"/sysconfig/libvirt-guests @@ -143,6 +144,8 @@ start() { fi isfirst=true + bypass= + test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache while read uri list; do configured=false set -f @@ -172,7 +175,8 @@ start() { else sleep $START_DELAY fi - retval run_virsh "$uri" start "$name" >/dev/null && \ + retval run_virsh "$uri" start $bypass "$name" \ + >/dev/null && \ gettext "done"; echo fi fi @@ -190,8 +194,10 @@ suspend_guest() name=$(guest_name "$uri" "$guest") label=$(eval_gettext "Suspending \$name: ") + bypass= + test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache printf %s "$label" - run_virsh "$uri" managedsave "$guest" >/dev/null & + run_virsh "$uri" managedsave $bypass "$guest" >/dev/null & virsh_pid=$! while true; do sleep 1 diff --git a/tools/libvirt-guests.sysconf b/tools/libvirt-guests.sysconf index 37b258e..9b8b64f 100644 --- a/tools/libvirt-guests.sysconf +++ b/tools/libvirt-guests.sysconf @@ -25,3 +25,8 @@ # number of seconds we're willing to wait for a guest to shut down #SHUTDOWN_TIMEOUT=0 + +# If non-zero, try to bypass the file system cache when saving and +# restoring guests, even though this may give slower operation for +# some file systems. +#BYPASS_CACHE=0 -- 1.7.4.4

With this, it is possible to update the path to a disk backing image on either the save or restore action, without having to binary edit the XML embedded in the state file. * src/qemu/qemu_driver.c (qemuDomainSaveInternal) (qemuDomainSaveImageOpen): Add parameter. (qemuDomainSaveFlags, qemuDomainManagedSave) (qemuDomainRestoreFlags, qemuDomainObjRestore): Update callers. --- v2: new patch Tested via: virsh start dom virsh dumpxml dom > dom.xml ln /path/to/disk /path/to/disk2 ln /path/to/disk /path/to/disk3 edit dom.xml to change /path/to/disk to /path/to/disk2 virsh save dom dom.save --xml dom.xml check that xml embedded in binary dom.save did change cp dom.xml dom.xml2 edit dom.xml to change /path/to/disk2 to /path/to/disk3 edit dom.xml2 to break abi, such as removing entire cdrom <disk> element virsh restore dom.save --xml dom.xml2 => properly failed virsh restore dom.save --xml dom.xml virsh dumpxml dom - validate that /path/to/disk3 is in use Which means it is now possible to do: virsh dumpxml dom > dom.xml virsh save dom dom.save use qemu-img to create new qcow2 files with backing file of the original images alter dom.xml to reflect the new file names virsh restore dom.save --xml dom.xml to externally manage disk snapshots causing changed file names. Not quite where I wanted to be with snapshot support, but certainly a step in the right direction! src/qemu/qemu_driver.c | 56 +++++++++++++++++++++++++++++++++-------------- 1 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 73d2938..d1fa06f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2113,7 +2113,7 @@ qemuCompressProgramName(int compress) static int qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainObjPtr vm, const char *path, - int compressed, bool bypass_cache) + int compressed, bool bypass_cache, const char *xmlin) { char *xml = NULL; struct qemud_save_header header; @@ -2160,7 +2160,22 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } /* Get XML for the domain */ - xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE); + if (xmlin) { + virDomainDefPtr def = NULL; + + if (!(def = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) { + goto endjob; + } + if (!virDomainDefCheckABIStability(vm->def, def)) { + virDomainDefFree(def); + goto endjob; + } + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE); + } else { + xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE); + } if (!xml) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to get domain xml")); @@ -2381,11 +2396,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, virDomainObjPtr vm = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); - if (dxml) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("xml modification unsupported")); - return -1; - } qemuDriverLock(driver); @@ -2423,7 +2433,8 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, } ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + dxml); vm = NULL; cleanup: @@ -2487,7 +2498,8 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) compressed = QEMUD_SAVE_FORMAT_RAW; ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + NULL); vm = NULL; cleanup: @@ -3616,7 +3628,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, - bool bypass_cache, virFileDirectFdPtr *directFd) + bool bypass_cache, virFileDirectFdPtr *directFd, + const char *xmlin) { int fd; struct qemud_save_header header; @@ -3700,6 +3713,20 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, QEMU_EXPECTED_VIRT_TYPES, VIR_DOMAIN_XML_INACTIVE))) goto error; + if (xmlin) { + virDomainDefPtr def2 = NULL; + + if (!(def2 = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto error; + if (!virDomainDefCheckABIStability(def, def2)) { + virDomainDefFree(def2); + goto error; + } + virDomainDefFree(def); + def = def2; + } VIR_FREE(xml); @@ -3834,17 +3861,12 @@ qemuDomainRestoreFlags(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); - if (dxml) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("xml modification unsupported")); - return -1; - } qemuDriverLock(driver); fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd); + &directFd, dxml); if (fd < 0) goto cleanup; @@ -3904,7 +3926,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd); + bypass_cache, &directFd, NULL); if (fd < 0) goto cleanup; -- 1.7.4.4

* src/qemu/qemu_driver.c (qemuDomainSaveImageGetXMLDesc) (qemuDomainSaveImageDefineXML): New functions. (qemuDomainSaveImageOpen): Add parameter. (qemuDomainRestoreFlags, qemuDomainObjRestore): Adjust clients. (qemuDomainSaveInternal): Simplify array expansion. --- Tested via: virsh save dom dom.save virsh save-image-dumpxml dom.save > dom.xml edit dom.xml virsh save-image-define dom.save dom.xml virsh save-image-edit dom.save virsh restore dom.save and the edits in dom.xml showed up in my editor, as well as on the restore. src/qemu/qemu_driver.c | 111 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 100 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d1fa06f..148770f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2214,14 +2214,14 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, unsigned long long pad = QEMU_MONITOR_MIGRATE_TO_FILE_BS - (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS); + size_t len = header.xml_len; - if (VIR_REALLOC_N(xml, header.xml_len + pad) < 0) { + if (VIR_EXPAND_N(xml, len, pad) < 0) { virReportOOMError(); goto endjob; } - memset(xml + header.xml_len, 0, pad); offset += pad; - header.xml_len += pad; + header.xml_len = len; } /* Obtain the file handle. */ @@ -3623,29 +3623,30 @@ cleanup: return ret; } -static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) +static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, bool bypass_cache, virFileDirectFdPtr *directFd, - const char *xmlin) + const char *xmlin, bool edit) { int fd; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; - int directFlag = 0; + int oflags = edit ? O_RDWR : O_RDONLY; if (bypass_cache) { - directFlag = virFileDirectFdFlag(); + int directFlag = virFileDirectFdFlag(); if (directFlag < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("bypass cache unsupported by this system")); goto error; } + oflags |= directFlag; } - if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + if ((fd = virFileOpenAs(path, oflags, 0, getuid(), getgid(), 0)) < 0) { if ((fd != -EACCES && fd != -EPERM) || driver->user == getuid()) { @@ -3656,7 +3657,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, /* Opening as root failed, but qemu runs as a different user * that might have better luck. */ - if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + if ((fd = virFileOpenAs(path, oflags, 0, driver->user, driver->group, VIR_FILE_OPEN_AS_UID)) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3866,7 +3867,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd, dxml); + &directFd, dxml, false); if (fd < 0) goto cleanup; @@ -3912,6 +3913,92 @@ qemuDomainRestore(virConnectPtr conn, return qemuDomainRestoreFlags(conn, path, NULL, 0); } +static char * +qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, + unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + char *ret = NULL; + virDomainDefPtr def = NULL; + int fd = -1; + struct qemud_save_header header; + + /* We only take subset of virDomainDefFormat flags. */ + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + + qemuDriverLock(driver); + + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, + NULL, false); + + if (fd < 0) + goto cleanup; + + ret = qemuDomainDefFormatXML(driver, def, flags); + +cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, + const char *dxml, unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + int ret = -1; + virDomainDefPtr def = NULL; + int fd = -1; + struct qemud_save_header header; + char *xml = NULL; + size_t len; + + virCheckFlags(0, -1); + + qemuDriverLock(driver); + + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, + dxml, true); + + if (fd < 0) + goto cleanup; + + xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_SECURE); + if (!xml) + goto cleanup; + len = strlen(xml) + 1; + + if (len > header.xml_len) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("new xml too large to fit in file")); + } + if (VIR_EXPAND_N(xml, len, header.xml_len - len) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (lseek(fd, sizeof(header), SEEK_SET) != sizeof(header)) { + virReportSystemError(errno, _("cannot seek in '%s'"), path); + goto cleanup; + } + if (safewrite(fd, xml, len) != len || + VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("failed to write xml to '%s'"), path); + goto cleanup; + } + + ret = 0; + +cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + VIR_FREE(xml); + qemuDriverUnlock(driver); + return ret; +} + static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, @@ -3926,7 +4013,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL); + bypass_cache, &directFd, NULL, false); if (fd < 0) goto cleanup; @@ -8690,6 +8777,8 @@ static virDriver qemuDriver = { .domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */ .domainRestore = qemuDomainRestore, /* 0.2.0 */ .domainRestoreFlags = qemuDomainRestoreFlags, /* 0.9.4 */ + .domainSaveImageGetXMLDesc = qemuDomainSaveImageGetXMLDesc, /* 0.9.4 */ + .domainSaveImageDefineXML = qemuDomainSaveImageDefineXML, /* 0.9.4 */ .domainCoreDump = qemudDomainCoreDump, /* 0.7.0 */ .domainScreenshot = qemuDomainScreenshot, /* 0.9.2 */ .domainSetVcpus = qemuDomainSetVcpus, /* 0.4.4 */ -- 1.7.4.4