Outgoing migration still uses a Unix socket and or exec netcat until the next patch. * src/qemu/qemu_migration.c (qemuMigrationPrepareTunnel): Replace Unix socket with simpler pipe. Suggested by Paolo Bonzini. --- src/qemu/qemu_migration.c | 45 +++++++++++++-------------------------------- 1 files changed, 13 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8ce6e17..f450130 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -239,11 +239,10 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, { virDomainDefPtr def = NULL; virDomainObjPtr vm = NULL; - char *migrateFrom; virDomainEventPtr event = NULL; int ret = -1; int internalret; - char *unixfile = NULL; + int dataFD[2] = { -1, -1 }; virBitmapPtr qemuCaps = NULL; qemuDomainObjPrivatePtr priv = NULL; struct timeval now; @@ -289,12 +288,12 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, /* Domain starts inactive, even if the domain XML had an id field. */ vm->def->id = -1; - if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.dest.%s", - driver->libDir, vm->def->name) < 0) { - virReportOOMError(); + if (pipe(dataFD) < 0 || + virSetCloseExec(dataFD[0]) < 0) { + virReportSystemError(errno, "%s", + _("cannot create pipe for tunnelled migration")); goto endjob; } - unlink(unixfile); /* check that this qemu version supports the interactive exec */ if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch, @@ -304,25 +303,11 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, vm->def->emulator); goto endjob; } - if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) - internalret = virAsprintf(&migrateFrom, "unix:%s", unixfile); - else if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) - internalret = virAsprintf(&migrateFrom, "exec:nc -U -l %s", unixfile); - else { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Destination qemu is too old to support tunnelled migration")); - goto endjob; - } - if (internalret < 0) { - virReportOOMError(); - goto endjob; - } /* Start the QEMU daemon, with the same command-line arguments plus - * -incoming unix:/path/to/file or exec:nc -U /path/to/file + * -incoming stdin (which qemu_command might convert to exec:cat or fd:n) */ - internalret = qemuProcessStart(dconn, driver, vm, migrateFrom, true, - -1, NULL, VIR_VM_OP_MIGRATE_IN_START); - VIR_FREE(migrateFrom); + internalret = qemuProcessStart(dconn, driver, vm, "stdin", true, dataFD[1], + NULL, VIR_VM_OP_MIGRATE_IN_START); if (internalret < 0) { qemuAuditDomainStart(vm, "migrated", false); /* Note that we don't set an error here because qemuProcessStart @@ -335,9 +320,7 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, goto endjob; } - if (virFDStreamConnectUNIX(st, - unixfile, - false) < 0) { + if (virFDStreamOpen(st, dataFD[0]) < 0) { qemuAuditDomainStart(vm, "migrated", false); qemuProcessStop(driver, vm, 0); if (!vm->persistent) { @@ -345,9 +328,8 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } - virReportSystemError(errno, - _("cannot open unix socket '%s' for tunnelled migration"), - unixfile); + virReportSystemError(errno, "%s", + _("cannot pass pipe for tunnelled migration")); goto endjob; } @@ -378,9 +360,8 @@ endjob: cleanup: qemuCapsFree(qemuCaps); virDomainDefFree(def); - if (unixfile) - unlink(unixfile); - VIR_FREE(unixfile); + VIR_FORCE_CLOSE(dataFD[0]); + VIR_FORCE_CLOSE(dataFD[1]); if (vm) virDomainObjUnlock(vm); if (event) -- 1.7.4

* src/qemu/qemu_monitor_text.h (qemuMonitorTextMigrate): Declare in place of individual monitor commands. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONMigrate): Likewise. * src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrateToHost) (qemuMonitorTextMigrateToCommand, qemuMonitorTextMigrateToFile) (qemuMonitorTextMigrateToUnix): Delete. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONMigrateToHost) (qemuMonitorJSONMigrateToCommand, qemuMonitorJSONMigrateToFile) (qemuMonitorJSONMigrateToUnix): Delete. * src/qemu/qemu_monitor.c (qemuMonitorMigrateToHost) (qemuMonitorMigrateToCommand, qemuMonitorMigrateToFile) (qemuMonitorMigrateToUnix): Consolidate shared code. --- src/qemu/qemu_monitor.c | 92 +++++++++++++++++++++++++++---- src/qemu/qemu_monitor_json.c | 125 +---------------------------------------- src/qemu/qemu_monitor_json.h | 23 +------ src/qemu/qemu_monitor_text.c | 123 +---------------------------------------- src/qemu/qemu_monitor_text.h | 23 +------ 5 files changed, 97 insertions(+), 289 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dfb1aad..eb6f8d4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1390,6 +1390,7 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon, int port) { int ret; + char *uri = NULL; VIR_DEBUG("mon=%p hostname=%s port=%d flags=%u", mon, hostname, port, flags); @@ -1399,10 +1400,18 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon, return -1; } + + if (virAsprintf(&uri, "tcp:%s:%d", hostname, port) < 0) { + virReportOOMError(); + return -1; + } + if (mon->json) - ret = qemuMonitorJSONMigrateToHost(mon, flags, hostname, port); + ret = qemuMonitorJSONMigrate(mon, flags, uri); else - ret = qemuMonitorTextMigrateToHost(mon, flags, hostname, port); + ret = qemuMonitorTextMigrate(mon, flags, uri); + + VIR_FREE(uri); return ret; } @@ -1411,7 +1420,9 @@ int qemuMonitorMigrateToCommand(qemuMonitorPtr mon, unsigned int flags, const char * const *argv) { - int ret; + char *argstr; + char *dest = NULL; + int ret = -1; VIR_DEBUG("mon=%p argv=%p flags=%u", mon, argv, flags); @@ -1421,10 +1432,25 @@ int qemuMonitorMigrateToCommand(qemuMonitorPtr mon, return -1; } + argstr = virArgvToString(argv); + if (!argstr) { + virReportOOMError(); + goto cleanup; + } + + if (virAsprintf(&dest, "exec:%s", argstr) < 0) { + virReportOOMError(); + goto cleanup; + } + if (mon->json) - ret = qemuMonitorJSONMigrateToCommand(mon, flags, argv); + ret = qemuMonitorJSONMigrate(mon, flags, dest); else - ret = qemuMonitorTextMigrateToCommand(mon, flags, argv); + ret = qemuMonitorTextMigrate(mon, flags, dest); + +cleanup: + VIR_FREE(argstr); + VIR_FREE(dest); return ret; } @@ -1434,7 +1460,10 @@ int qemuMonitorMigrateToFile(qemuMonitorPtr mon, const char *target, unsigned long long offset) { - int ret; + char *argstr; + char *dest = NULL; + int ret = -1; + char *safe_target = NULL; VIR_DEBUG("mon=%p argv=%p target=%s offset=%llu flags=%u", mon, argv, target, offset, flags); @@ -1451,10 +1480,43 @@ int qemuMonitorMigrateToFile(qemuMonitorPtr mon, return -1; } + argstr = virArgvToString(argv); + if (!argstr) { + virReportOOMError(); + goto cleanup; + } + + /* Migrate to file */ + safe_target = qemuMonitorEscapeShell(target); + if (!safe_target) { + virReportOOMError(); + goto cleanup; + } + + /* Two dd processes, sharing the same stdout, are necessary to + * allow starting at an alignment of 512, but without wasting + * padding to get to the larger alignment useful for speed. Use + * <> redirection to avoid truncating a regular file. */ + if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s | " + "{ dd bs=%llu seek=%llu if=/dev/null && " + "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX, + argstr, QEMU_MONITOR_MIGRATE_TO_FILE_BS, + offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS, + QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE, + safe_target) < 0) { + virReportOOMError(); + goto cleanup; + } + if (mon->json) - ret = qemuMonitorJSONMigrateToFile(mon, flags, argv, target, offset); + ret = qemuMonitorJSONMigrate(mon, flags, dest); else - ret = qemuMonitorTextMigrateToFile(mon, flags, argv, target, offset); + ret = qemuMonitorTextMigrate(mon, flags, dest); + +cleanup: + VIR_FREE(safe_target); + VIR_FREE(argstr); + VIR_FREE(dest); return ret; } @@ -1462,7 +1524,8 @@ int qemuMonitorMigrateToUnix(qemuMonitorPtr mon, unsigned int flags, const char *unixfile) { - int ret; + char *dest = NULL; + int ret = -1; VIR_DEBUG("mon=%p, unixfile=%s flags=%u", mon, unixfile, flags); @@ -1472,10 +1535,17 @@ int qemuMonitorMigrateToUnix(qemuMonitorPtr mon, return -1; } + if (virAsprintf(&dest, "unix:%s", unixfile) < 0) { + virReportOOMError(); + return -1; + } + if (mon->json) - ret = qemuMonitorJSONMigrateToUnix(mon, flags, unixfile); + ret = qemuMonitorJSONMigrate(mon, flags, dest); else - ret = qemuMonitorTextMigrateToUnix(mon, flags, unixfile); + ret = qemuMonitorTextMigrate(mon, flags, dest); + + VIR_FREE(dest); return ret; } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e6903a1..5cf35b9 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1,7 +1,7 @@ /* * qemu_monitor_json.c: interaction with QEMU monitor console * - * Copyright (C) 2006-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 @@ -1669,9 +1669,9 @@ int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon, } -static int qemuMonitorJSONMigrate(qemuMonitorPtr mon, - unsigned int flags, - const char *uri) +int qemuMonitorJSONMigrate(qemuMonitorPtr mon, + unsigned int flags, + const char *uri) { int ret; virJSONValuePtr cmd = @@ -1696,123 +1696,6 @@ static int qemuMonitorJSONMigrate(qemuMonitorPtr mon, return ret; } - -int qemuMonitorJSONMigrateToHost(qemuMonitorPtr mon, - unsigned int flags, - const char *hostname, - int port) -{ - char *uri = NULL; - int ret; - - if (virAsprintf(&uri, "tcp:%s:%d", hostname, port) < 0) { - virReportOOMError(); - return -1; - } - - ret = qemuMonitorJSONMigrate(mon, flags, uri); - - VIR_FREE(uri); - - return ret; -} - - -int qemuMonitorJSONMigrateToCommand(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv) -{ - char *argstr; - char *dest = NULL; - int ret = -1; - - argstr = virArgvToString(argv); - if (!argstr) { - virReportOOMError(); - goto cleanup; - } - - if (virAsprintf(&dest, "exec:%s", argstr) < 0) { - virReportOOMError(); - goto cleanup; - } - - ret = qemuMonitorJSONMigrate(mon, flags, dest); - -cleanup: - VIR_FREE(argstr); - VIR_FREE(dest); - return ret; -} - -int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv, - const char *target, - unsigned long long offset) -{ - char *argstr; - char *dest = NULL; - int ret = -1; - char *safe_target = NULL; - - argstr = virArgvToString(argv); - if (!argstr) { - virReportOOMError(); - goto cleanup; - } - - /* Migrate to file */ - safe_target = qemuMonitorEscapeShell(target); - if (!safe_target) { - virReportOOMError(); - goto cleanup; - } - - /* Two dd processes, sharing the same stdout, are necessary to - * allow starting at an alignment of 512, but without wasting - * padding to get to the larger alignment useful for speed. Use - * <> redirection to avoid truncating a regular file. */ - if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s | " - "{ dd bs=%llu seek=%llu if=/dev/null && " - "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX, - argstr, QEMU_MONITOR_MIGRATE_TO_FILE_BS, - offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS, - QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE, - safe_target) < 0) { - virReportOOMError(); - goto cleanup; - } - - ret = qemuMonitorJSONMigrate(mon, flags, dest); - -cleanup: - VIR_FREE(safe_target); - VIR_FREE(argstr); - VIR_FREE(dest); - return ret; -} - -int qemuMonitorJSONMigrateToUnix(qemuMonitorPtr mon, - unsigned int flags, - const char *unixfile) -{ - char *dest = NULL; - int ret = -1; - - if (virAsprintf(&dest, "unix:%s", unixfile) < 0) { - virReportOOMError(); - return -1; - } - - ret = qemuMonitorJSONMigrate(mon, flags, dest); - - VIR_FREE(dest); - - return ret; -} - - int qemuMonitorJSONMigrateCancel(qemuMonitorPtr mon) { int ret; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4ae472a..bca7070 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -1,7 +1,7 @@ /* * qemu_monitor_json.h: interaction with QEMU monitor console * - * Copyright (C) 2006-2009 Red Hat, Inc. + * Copyright (C) 2006-2009, 2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -104,24 +104,9 @@ int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon, unsigned long long *remaining, unsigned long long *total); -int qemuMonitorJSONMigrateToHost(qemuMonitorPtr mon, - unsigned int flags, - const char *hostname, - int port); - -int qemuMonitorJSONMigrateToCommand(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv); - -int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv, - const char *target, - unsigned long long offset); - -int qemuMonitorJSONMigrateToUnix(qemuMonitorPtr mon, - unsigned int flags, - const char *unixfile); +int qemuMonitorJSONMigrate(qemuMonitorPtr mon, + unsigned int flags, + const char *uri); int qemuMonitorJSONMigrateCancel(qemuMonitorPtr mon); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 0aed690..9b93e1c 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1,7 +1,7 @@ /* * qemu_monitor_text.c: interaction with QEMU monitor console * - * Copyright (C) 2006-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 @@ -1205,9 +1205,9 @@ cleanup: } -static int qemuMonitorTextMigrate(qemuMonitorPtr mon, - unsigned int flags, - const char *dest) +int qemuMonitorTextMigrate(qemuMonitorPtr mon, + unsigned int flags, + const char *dest) { char *cmd = NULL; char *info = NULL; @@ -1271,121 +1271,6 @@ cleanup: return ret; } -int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon, - unsigned int flags, - const char *hostname, - int port) -{ - char *uri = NULL; - int ret; - - if (virAsprintf(&uri, "tcp:%s:%d", hostname, port) < 0) { - virReportOOMError(); - return -1; - } - - ret = qemuMonitorTextMigrate(mon, flags, uri); - - VIR_FREE(uri); - - return ret; -} - - -int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv) -{ - char *argstr; - char *dest = NULL; - int ret = -1; - - argstr = virArgvToString(argv); - if (!argstr) { - virReportOOMError(); - goto cleanup; - } - - if (virAsprintf(&dest, "exec:%s", argstr) < 0) { - virReportOOMError(); - goto cleanup; - } - - ret = qemuMonitorTextMigrate(mon, flags, dest); - -cleanup: - VIR_FREE(argstr); - VIR_FREE(dest); - return ret; -} - -int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv, - const char *target, - unsigned long long offset) -{ - char *argstr; - char *dest = NULL; - int ret = -1; - char *safe_target = NULL; - - argstr = virArgvToString(argv); - if (!argstr) { - virReportOOMError(); - goto cleanup; - } - - /* Migrate to file */ - safe_target = qemuMonitorEscapeShell(target); - if (!safe_target) { - virReportOOMError(); - goto cleanup; - } - - /* Two dd processes, sharing the same stdout, are necessary to - * allow starting at an alignment of 512, but without wasting - * padding to get to the larger alignment useful for speed. Use - * <> redirection to avoid truncating a regular file. */ - if (virAsprintf(&dest, "exec:" VIR_WRAPPER_SHELL_PREFIX "%s | " - "{ dd bs=%llu seek=%llu if=/dev/null && " - "dd bs=%llu; } 1<>%s" VIR_WRAPPER_SHELL_SUFFIX, - argstr, QEMU_MONITOR_MIGRATE_TO_FILE_BS, - offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS, - QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE, - safe_target) < 0) { - virReportOOMError(); - goto cleanup; - } - - ret = qemuMonitorTextMigrate(mon, flags, dest); - -cleanup: - VIR_FREE(safe_target); - VIR_FREE(argstr); - VIR_FREE(dest); - return ret; -} - -int qemuMonitorTextMigrateToUnix(qemuMonitorPtr mon, - unsigned int flags, - const char *unixfile) -{ - char *dest = NULL; - int ret = -1; - - if (virAsprintf(&dest, "unix:%s", unixfile) < 0) { - virReportOOMError(); - return -1; - } - - ret = qemuMonitorTextMigrate(mon, flags, dest); - - VIR_FREE(dest); - - return ret; -} - int qemuMonitorTextMigrateCancel(qemuMonitorPtr mon) { char *info = NULL; diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index b29dbcc..c2a85b7 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -1,7 +1,7 @@ /* * qemu_monitor_text.h: interaction with QEMU monitor console * - * Copyright (C) 2006-2009 Red Hat, Inc. + * Copyright (C) 2006-2009, 2011 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -102,24 +102,9 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon, unsigned long long *remaining, unsigned long long *total); -int qemuMonitorTextMigrateToHost(qemuMonitorPtr mon, - unsigned int flags, - const char *hostname, - int port); - -int qemuMonitorTextMigrateToCommand(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv); - -int qemuMonitorTextMigrateToFile(qemuMonitorPtr mon, - unsigned int flags, - const char * const *argv, - const char *target, - unsigned long long offset); - -int qemuMonitorTextMigrateToUnix(qemuMonitorPtr mon, - unsigned int flags, - const char *unixfile); +int qemuMonitorTextMigrate(qemuMonitorPtr mon, + unsigned int flags, + const char *uri); int qemuMonitorTextMigrateCancel(qemuMonitorPtr mon); -- 1.7.4

* src/qemu/qemu_monitor.h (qemuMonitorMigrateToFd): New prototype. * src/qemu/qemu_monitor.c (qemuMonitorMigrateToFd): New function. --- src/qemu/qemu_monitor.c | 31 +++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ 2 files changed, 35 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 765a1ae..1e79523 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1384,6 +1384,37 @@ int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon, } +int qemuMonitorMigrateToFd(qemuMonitorPtr mon, + unsigned int flags, + int fd) +{ + int ret; + VIR_DEBUG("mon=%p fd=%d flags=%u", + mon, fd, flags); + + if (!mon) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (qemuMonitorSendFileHandle(mon, "migrate", fd) < 0) + return -1; + + if (mon->json) + ret = qemuMonitorJSONMigrate(mon, flags, "fd:migrate"); + else + ret = qemuMonitorTextMigrate(mon, flags, "fd:migrate"); + + if (ret < 0) { + if (qemuMonitorCloseFileHandle(mon, "migrate") < 0) + VIR_WARN0("failed to close migration handle"); + } + + return ret; +} + + int qemuMonitorMigrateToHost(qemuMonitorPtr mon, unsigned int flags, const char *hostname, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 0ea1330..1a64ac0 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -260,6 +260,10 @@ typedef enum { QEMU_MONITOR_MIGRATION_FLAGS_LAST } QEMU_MONITOR_MIGRATE; +int qemuMonitorMigrateToFd(qemuMonitorPtr mon, + unsigned int flags, + int fd); + int qemuMonitorMigrateToHost(qemuMonitorPtr mon, unsigned int flags, const char *hostname, -- 1.7.4

* Eric Blake (eblake(a)redhat.com) wrote: Should that be in common sendfd code?

Currently, the hook function in virFileOperation is extremely limited: it must be async-signal-safe, and cannot modify any memory in the parent process. It is much handier to return a valid fd and operate on it in the parent than to deal with hook restrictions. * src/util/util.h (VIR_FILE_OP_RETURN_FD): New flag. * src/util/util.c (virFileOperationNoFork, virFileOperation): Honor new flag. --- src/util/util.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++------ src/util/util.h | 4 +- 2 files changed, 126 insertions(+), 17 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index f41e117..6bf3219 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1392,14 +1392,15 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { goto error; } + if (flags & VIR_FILE_OP_RETURN_FD) + return fd; if (VIR_CLOSE(fd) < 0) { ret = -errno; virReportSystemError(errno, _("failed to close new file '%s'"), path); - fd = -1; goto error; } - fd = -1; + error: VIR_FORCE_CLOSE(fd); return ret; @@ -1444,7 +1445,32 @@ error: return ret; } -/* return -errno on failure, or 0 on success */ +/** + * virFileOperation: + * @path: file to open or create + * @openflags: flags to pass to open + * @mode: mode to use on creation or when forcing permissions + * @uid: uid that should own file on creation + * @gid: gid that should own file + * @hook: callback to run once file is open; see below for restrictions + * @hookdata: opaque data for @hook + * @flags: bit-wise or of VIR_FILE_OP_* flags + * + * Open @path, and execute an optional callback @hook on the open file + * description. @hook must return 0 on success, or -errno on failure. + * If @flags includes VIR_FILE_OP_AS_UID, then open the file while the + * effective user id is @uid (by using a child process); this allows + * one to bypass root-squashing NFS issues. If @flags includes + * VIR_FILE_OP_FORCE_PERMS, then ensure that @path has those + * permissions before the callback, even if the file already existed + * with different permissions. If @flags includes + * VIR_FILE_OP_RETURN_FD, then use SCM_RIGHTS to guarantee that any + * @hook is run in the parent, and the return value (if non-negative) + * is the file descriptor, left open. Otherwise, @hook might be run + * in a child process, so it must be async-signal-safe (ie. no + * malloc() or anything else that depends on modifying locks held in + * the parent), no file descriptor remains open, and 0 is returned on + * success. Return -errno on failure. */ int virFileOperation(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, virFileOperationHook hook, void *hookdata, @@ -1452,7 +1478,14 @@ int virFileOperation(const char *path, int openflags, mode_t mode, struct stat st; pid_t pid; int waitret, status, ret = 0; - int fd; + int fd = -1; + int pair[2] = { -1, -1 }; + struct msghdr msg; + struct cmsghdr *cmsg; + char buf[CMSG_SPACE(sizeof(fd))]; + char dummy = 0; + struct iovec iov; + int forkRet; if ((!(flags & VIR_FILE_OP_AS_UID)) || (getuid() != 0) @@ -1466,7 +1499,29 @@ int virFileOperation(const char *path, int openflags, mode_t mode, * following dance avoids problems caused by root-squashing * NFS servers. */ - int forkRet = virFork(&pid); + if (flags & VIR_FILE_OP_RETURN_FD) { + if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pair) < 0) { + ret = -errno; + virReportSystemError(errno, + _("failed to create socket needed for '%s'"), + path); + return ret; + } + + memset(&msg, 0, sizeof(msg)); + iov.iov_base = &dummy; + iov.iov_len = 1; + msg.msg_iov = &iov; + msg.msg_iovlen = 1; + msg.msg_control = buf; + msg.msg_controllen = sizeof(buf); + cmsg = CMSG_FIRSTHDR(&msg); + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); + } + + forkRet = virFork(&pid); if (pid < 0) { ret = -errno; @@ -1474,6 +1529,31 @@ int virFileOperation(const char *path, int openflags, mode_t mode, } if (pid) { /* parent */ + if (flags & VIR_FILE_OP_RETURN_FD) { + VIR_FORCE_CLOSE(pair[1]); + + do { + ret = recvmsg(pair[0], &msg, 0); + } while (ret < 0 && errno == EINTR); + + if (ret < 0) { + ret = -errno; + VIR_FORCE_CLOSE(pair[0]); + while ((waitret = waitpid(pid, NULL, 0) == -1) + && (errno == EINTR)); + goto parenterror; + } + VIR_FORCE_CLOSE(pair[0]); + + /* See if fd was transferred. */ + cmsg = CMSG_FIRSTHDR(&msg); + if (cmsg && cmsg->cmsg_len == CMSG_LEN(sizeof(fd)) && + cmsg->cmsg_level == SOL_SOCKET && + cmsg->cmsg_type == SCM_RIGHTS) { + memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd)); + } + } + /* wait for child to complete, and retrieve its exit code */ while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); @@ -1485,12 +1565,20 @@ int virFileOperation(const char *path, int openflags, mode_t mode, goto parenterror; } ret = -WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == -EACCES)) { + if (!WIFEXITED(status) || (ret == -EACCES) || + ((flags & VIR_FILE_OP_RETURN_FD) && fd == -1)) { /* fall back to the simpler method, which works better in * some cases */ return virFileOperationNoFork(path, openflags, mode, uid, gid, hook, hookdata, flags); } + if (!ret && (flags & VIR_FILE_OP_RETURN_FD)) { + if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { + VIR_FORCE_CLOSE(fd); + goto parenterror; + } + ret = fd; + } parenterror: return ret; } @@ -1500,6 +1588,7 @@ parenterror: if (forkRet < 0) { /* error encountered and logged in virFork() after the fork. */ + ret = -errno; goto childerror; } @@ -1549,15 +1638,33 @@ parenterror: path, mode); goto childerror; } - if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { - goto childerror; - } - if (VIR_CLOSE(fd) < 0) { - ret = -errno; - virReportSystemError(errno, _("child failed to close new file '%s'"), - path); - goto childerror; + if (flags & VIR_FILE_OP_RETURN_FD) { + VIR_FORCE_CLOSE(pair[0]); + memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd)); + + do { + ret = sendmsg(pair[1], &msg, 0); + } while (ret < 0 && errno == EINTR); + + if (ret < 0) { + ret = -errno; + VIR_FORCE_CLOSE(pair[1]); + goto childerror; + } + ret = 0; + } else { + if ((hook) && ((ret = hook(fd, hookdata)) != 0)) + goto childerror; + if (VIR_CLOSE(fd) < 0) { + ret = -errno; + virReportSystemError(errno, + _("child failed to close new file '%s'"), + path); + goto childerror; + } } + + ret = 0; childerror: /* ret tracks -errno on failure, but exit value must be positive. * If the child exits with EACCES, then the parent tries again. */ @@ -1688,7 +1795,7 @@ int virFileOperation(const char *path ATTRIBUTE_UNUSED, virUtilError(VIR_ERR_INTERNAL_ERROR, "%s", _("virFileOperation is not implemented for WIN32")); - return -1; + return -ENOSYS; } int virDirCreate(const char *path ATTRIBUTE_UNUSED, @@ -1700,7 +1807,7 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED, virUtilError(VIR_ERR_INTERNAL_ERROR, "%s", _("virDirCreate is not implemented for WIN32")); - return -1; + return -ENOSYS; } #endif /* WIN32 */ diff --git a/src/util/util.h b/src/util/util.h index 5f6473c..3970d58 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -128,12 +128,14 @@ enum { VIR_FILE_OP_NONE = 0, VIR_FILE_OP_AS_UID = (1 << 0), VIR_FILE_OP_FORCE_PERMS = (1 << 1), + VIR_FILE_OP_RETURN_FD = (1 << 2), }; typedef int (*virFileOperationHook)(int fd, void *data); int virFileOperation(const char *path, int openflags, mode_t mode, uid_t uid, gid_t gid, virFileOperationHook hook, void *hookdata, - unsigned int flags) ATTRIBUTE_RETURN_CHECK; + unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; enum { VIR_DIR_CREATE_NONE = 0, -- 1.7.4

This allows direct saves (no compression, no root-squash NFS) to use the more efficient fd: migration, which in turn avoids a race where qemu exec: migration can sometimes fail because qemu does a generic waitpid() that conflicts with the pclose() used by exec:. Further patches will solve compression and root-squash NFS. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Use new function when there is no compression. --- src/qemu/qemu_driver.c | 26 +++++++++++++++++++++++--- 1 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2d11da8..c4d3c85 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1790,6 +1790,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, int is_reg = 0; unsigned long long offset; virCgroupPtr cgroup = NULL; + virBitmapPtr qemuCaps = NULL; memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -1820,6 +1821,11 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, } } + if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch, + NULL, + &qemuCaps) < 0) + goto endjob; + /* Get XML for the domain */ xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE); if (!xml) { @@ -1980,10 +1986,23 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (header.compressed == QEMUD_SAVE_FORMAT_RAW) { const char *args[] = { "cat", NULL }; + /* XXX gross - why don't we reuse the fd already opened earlier */ + int fd = -1; + + if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) + fd = open(path, O_WRONLY); qemuDomainObjEnterMonitorWithDriver(driver, vm); - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); + if (fd >= 0 && lseek(fd, offset, SEEK_SET) == offset) { + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + fd); + } else { + rc = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, offset); + } + VIR_FORCE_CLOSE(fd); qemuDomainObjExitMonitorWithDriver(driver, vm); } else { const char *prog = qemudSaveCompressionTypeToString(header.compressed); @@ -2065,6 +2084,7 @@ endjob: } cleanup: + qemuCapsFree(qemuCaps); VIR_FREE(xml); if (ret != 0 && is_reg) unlink(path); -- 1.7.4

Direct access to an open file is so much simpler than passing everything through a pipe! * src/qemu/qemu_driver.c (qemudOpenAsUID) (qemudDomainSaveImageClose): Delete. (qemudDomainSaveImageOpen): Rename... (qemuDomainSaveImageOpen): ...and drop read_pid argument. Use virFileOpenAs instead of qemudOpenAsUID. (qemudDomainSaveImageStartVM, qemudDomainRestore) (qemudDomainObjRestore): Rename... (qemuDomainSaveImageStartVM, qemuDomainRestore) (qemDomainObjRestore): ...and simplify accordingly. (qemudDomainObjStart, qemuDriver): Update callers. --- src/qemu/qemu_driver.c | 265 ++++++++--------------------------------------- 1 files changed, 45 insertions(+), 220 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 11ba910..9de19ea 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3012,183 +3012,32 @@ cleanup: return ret; } -/* qemudOpenAsUID() - pipe/fork/setuid/open a file, and return the - pipe fd to caller, so that it can read from the file. Also return - the pid of the child process, so the caller can wait for it to exit - after it's finished reading (to avoid a zombie, if nothing - else). */ - -static int -qemudOpenAsUID(const char *path, uid_t uid, gid_t gid, pid_t *child_pid) -{ - int pipefd[2]; - int fd = -1; - - *child_pid = -1; - - if (pipe(pipefd) < 0) { - virReportSystemError(errno, - _("failed to create pipe to read '%s'"), - path); - pipefd[0] = pipefd[1] = -1; - goto parent_cleanup; - } - - int forkRet = virFork(child_pid); - - if (*child_pid < 0) { - virReportSystemError(errno, - _("failed to fork child to read '%s'"), - path); - goto parent_cleanup; - } - - if (*child_pid > 0) { - - /* parent */ - - /* parent doesn't need the write side of the pipe */ - VIR_FORCE_CLOSE(pipefd[1]); - - if (forkRet < 0) { - virReportSystemError(errno, - _("failed in parent after forking child to read '%s'"), - path); - goto parent_cleanup; - } - /* caller gets the read side of the pipe */ - fd = pipefd[0]; - pipefd[0] = -1; -parent_cleanup: - VIR_FORCE_CLOSE(pipefd[0]); - VIR_FORCE_CLOSE(pipefd[1]); - if ((fd < 0) && (*child_pid > 0)) { - /* a child process was started and subsequently an error - occurred in the parent, so we need to wait for it to - exit, but its status is inconsequential. */ - while ((waitpid(*child_pid, NULL, 0) == -1) - && (errno == EINTR)) { - /* empty */ - } - *child_pid = -1; - } - return fd; - } - - /* child */ - - /* setuid to the qemu user, then open the file, read it, - and stuff it into the pipe for the parent process to - read */ - int exit_code; - char *buf = NULL; - size_t bufsize = 1024 * 1024; - int bytesread; - - /* child doesn't need the read side of the pipe */ - VIR_FORCE_CLOSE(pipefd[0]); - - if (forkRet < 0) { - exit_code = errno; - virReportSystemError(errno, - _("failed in child after forking to read '%s'"), - path); - goto child_cleanup; - } - - if (virSetUIDGID(uid, gid) < 0) { - exit_code = errno; - goto child_cleanup; - } - - if ((fd = open(path, O_RDONLY)) < 0) { - exit_code = errno; - virReportSystemError(errno, - _("cannot open '%s' as uid %d"), - path, uid); - goto child_cleanup; - } - - if (VIR_ALLOC_N(buf, bufsize) < 0) { - exit_code = ENOMEM; - virReportOOMError(); - goto child_cleanup; - } - - /* read from fd and write to pipefd[1] until EOF */ - do { - if ((bytesread = saferead(fd, buf, bufsize)) < 0) { - exit_code = errno; - virReportSystemError(errno, - _("child failed reading from '%s'"), - path); - goto child_cleanup; - } - if (safewrite(pipefd[1], buf, bytesread) != bytesread) { - exit_code = errno; - virReportSystemError(errno, "%s", - _("child failed writing to pipe")); - goto child_cleanup; - } - } while (bytesread > 0); - exit_code = 0; - -child_cleanup: - VIR_FREE(buf); - VIR_FORCE_CLOSE(fd); - VIR_FORCE_CLOSE(pipefd[1]); - _exit(exit_code); -} - -static int qemudDomainSaveImageClose(int fd, pid_t read_pid, int *status) -{ - int ret = 0; - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, "%s", - _("cannot close file")); - } - - if (read_pid != -1) { - /* reap the process that read the file */ - while ((ret = waitpid(read_pid, status, 0)) == -1 - && errno == EINTR) { - /* empty */ - } - } else if (status) { - *status = 0; - } - - return ret; -} - -static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) -qemudDomainSaveImageOpen(struct qemud_driver *driver, - const char *path, - virDomainDefPtr *ret_def, - struct qemud_save_header *ret_header, - pid_t *ret_read_pid) +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) { int fd; - pid_t read_pid = -1; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; - if ((fd = open(path, O_RDONLY)) < 0) { - if ((driver->user == 0) || (getuid() != 0)) { + if ((fd = virFileOpenAs(path, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) { + if ((fd != -EACCES && fd != -EPERM) || + driver->user == getuid()) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot read domain image")); goto error; } /* Opening as root failed, but qemu runs as a different user - that might have better luck. Create a pipe, then fork a - child process to run as the qemu user, which will hopefully - have the necessary authority to read the file. */ - if ((fd = qemudOpenAsUID(path, - driver->user, driver->group, &read_pid)) < 0) { - /* error already reported */ + * that might have better luck. */ + if ((fd = virFileOpenAs(path, O_RDONLY, 0, + driver->user, driver->group, + VIR_FILE_OPEN_AS_UID)) < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("cannot read domain image")); goto error; } } @@ -3238,34 +3087,30 @@ qemudDomainSaveImageOpen(struct qemud_driver *driver, *ret_def = def; *ret_header = header; - *ret_read_pid = read_pid; return fd; error: virDomainDefFree(def); VIR_FREE(xml); - qemudDomainSaveImageClose(fd, read_pid, NULL); + VIR_FORCE_CLOSE(fd); return -1; } -static int ATTRIBUTE_NONNULL(6) -qemudDomainSaveImageStartVM(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - int *fd, - pid_t *read_pid, - const struct qemud_save_header *header, - const char *path) +static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) +qemuDomainSaveImageStartVM(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + int *fd, + const struct qemud_save_header *header, + const char *path) { int ret = -1; virDomainEventPtr event; int intermediatefd = -1; pid_t intermediate_pid = -1; int childstat; - int wait_ret; - int status; if (header->version == 2) { const char *intermediate_argv[3] = { NULL, "-dc", NULL }; @@ -3298,8 +3143,9 @@ qemudDomainSaveImageStartVM(virConnectPtr conn, if (intermediate_pid != -1) { if (ret < 0) { - /* if there was an error setting up qemu, the intermediate process will - * wait forever to write to stdout, so we must manually kill it. + /* if there was an error setting up qemu, the intermediate + * process will wait forever to write to stdout, so we + * must manually kill it. */ VIR_FORCE_CLOSE(intermediatefd); VIR_FORCE_CLOSE(*fd); @@ -3314,30 +3160,10 @@ qemudDomainSaveImageStartVM(virConnectPtr conn, } VIR_FORCE_CLOSE(intermediatefd); - wait_ret = qemudDomainSaveImageClose(*fd, *read_pid, &status); - *fd = -1; - if (*read_pid != -1) { - if (wait_ret == -1) { - virReportSystemError(errno, - _("failed to wait for process reading '%s'"), - path); - ret = -1; - } else if (!WIFEXITED(status)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("child process exited abnormally reading '%s'"), - path); - ret = -1; - } else { - int exit_status = WEXITSTATUS(status); - if (exit_status != 0) { - virReportSystemError(exit_status, - _("child process returned error reading '%s'"), - path); - ret = -1; - } - } + if (VIR_CLOSE(*fd) < 0) { + virReportSystemError(errno, _("cannot close file: %s"), path); + ret = -1; } - *read_pid = -1; if (ret < 0) { qemuAuditDomainStart(vm, "restored", false); @@ -3376,19 +3202,20 @@ out: return ret; } -static int qemudDomainRestore(virConnectPtr conn, - const char *path) { +static int +qemuDomainRestore(virConnectPtr conn, + const char *path) +{ struct qemud_driver *driver = conn->privateData; virDomainDefPtr def = NULL; virDomainObjPtr vm = NULL; int fd = -1; - pid_t read_pid = -1; int ret = -1; struct qemud_save_header header; qemuDriverLock(driver); - fd = qemudDomainSaveImageOpen(driver, path, &def, &header, &read_pid); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header); if (fd < 0) goto cleanup; @@ -3406,8 +3233,7 @@ static int qemudDomainRestore(virConnectPtr conn, if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) goto cleanup; - ret = qemudDomainSaveImageStartVM(conn, driver, vm, &fd, - &read_pid, &header, path); + ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); if (qemuDomainObjEndJob(vm) == 0) vm = NULL; @@ -3418,25 +3244,25 @@ static int qemudDomainRestore(virConnectPtr conn, cleanup: virDomainDefFree(def); - qemudDomainSaveImageClose(fd, read_pid, NULL); + VIR_FORCE_CLOSE(fd); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } -static int qemudDomainObjRestore(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - const char *path) +static int +qemuDomainObjRestore(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + const char *path) { virDomainDefPtr def = NULL; int fd = -1; - pid_t read_pid = -1; int ret = -1; struct qemud_save_header header; - fd = qemudDomainSaveImageOpen(driver, path, &def, &header, &read_pid); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header); if (fd < 0) goto cleanup; @@ -3457,12 +3283,11 @@ static int qemudDomainObjRestore(virConnectPtr conn, virDomainObjAssignDef(vm, def, true); def = NULL; - ret = qemudDomainSaveImageStartVM(conn, driver, vm, &fd, - &read_pid, &header, path); + ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); cleanup: virDomainDefFree(def); - qemudDomainSaveImageClose(fd, read_pid, NULL); + VIR_FORCE_CLOSE(fd); return ret; } @@ -3673,7 +3498,7 @@ static int qemudDomainObjStart(virConnectPtr conn, */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { - ret = qemudDomainObjRestore(conn, driver, vm, managed_save); + ret = qemuDomainObjRestore(conn, driver, vm, managed_save); if (unlink(managed_save) < 0) { VIR_WARN("Failed to remove the managed state %s", managed_save); @@ -6810,7 +6635,7 @@ static virDriver qemuDriver = { qemudDomainSetMemory, /* domainSetMemory */ qemudDomainGetInfo, /* domainGetInfo */ qemudDomainSave, /* domainSave */ - qemudDomainRestore, /* domainRestore */ + qemuDomainRestore, /* domainRestore */ qemudDomainCoreDump, /* domainCoreDump */ qemudDomainSetVcpus, /* domainSetVcpus */ qemudDomainSetVcpusFlags, /* domainSetVcpusFlags */ -- 1.7.4

This points out that core dumps (still) don't work for root-squash NFS, since the fd is not opened correctly. This patch should not introduce any functionality change, it is just a refactoring to avoid duplicated code. * src/qemu/qemu_driver.c (qemuDomainMigrateToFile): New function. (qemudDomainSaveFlag, doCoreDump): Use it. --- src/qemu/qemu_driver.c | 249 +++++++++++++++++++++--------------------------- 1 files changed, 110 insertions(+), 139 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9de19ea..2422482 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1766,6 +1766,101 @@ endjob: return ret; } +/* Internal function called while driver lock is held and vm is active. */ +static int +qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, + virBitmapPtr qemuCaps, + int fd, off_t offset, const char *path, + int compressed, + bool is_reg, bool bypassSecurityDriver) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virCgroupPtr cgroup = NULL; + int ret = -1; + int rc; + + if (!is_reg && + qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { + if (virCgroupForDomain(driver->cgroup, vm->def->name, + &cgroup, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find cgroup for %s"), + vm->def->name); + goto cleanup; + } + rc = virCgroupAllowDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RW); + qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc); + if (rc < 0) { + virReportSystemError(-rc, + _("Unable to allow device %s for %s"), + path, vm->def->name); + goto cleanup; + } + } + + if ((!bypassSecurityDriver) && + virSecurityManagerSetSavedStateLabel(driver->securityManager, + vm, path) < 0) + goto cleanup; + + if (compressed == QEMUD_SAVE_FORMAT_RAW) { + const char *args[] = { "cat", NULL }; + + qemuDomainObjEnterMonitorWithDriver(driver, vm); + if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) { + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + fd); + } else { + rc = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, offset); + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + } else { + const char *prog = qemudSaveCompressionTypeToString(compressed); + const char *args[] = { + prog, + "-c", + NULL + }; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + rc = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, offset); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + + if (rc < 0) + goto cleanup; + + rc = qemuMigrationWaitForCompletion(driver, vm); + + if (rc < 0) + goto cleanup; + + ret = 0; + +cleanup: + if ((!bypassSecurityDriver) && + virSecurityManagerRestoreSavedStateLabel(driver->securityManager, + vm, path) < 0) + VIR_WARN("failed to restore save state label on %s", path); + + if (cgroup != NULL) { + rc = virCgroupDenyDevicePath(cgroup, path, + VIR_CGROUP_DEVICE_RWM); + qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc); + if (rc < 0) + VIR_WARN("Unable to deny device %s for %s %d", + path, vm->def->name, rc); + virCgroupFree(&cgroup); + } + return ret; +} + /* This internal function expects the driver lock to already be held on * entry and the vm must be active. */ @@ -1775,15 +1870,14 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, { char *xml = NULL; struct qemud_save_header header; - int bypassSecurityDriver = 0; + bool bypassSecurityDriver = false; int ret = -1; int rc; virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; struct stat sb; - int is_reg = 0; + bool is_reg = false; unsigned long long offset; - virCgroupPtr cgroup = NULL; virBitmapPtr qemuCaps = NULL; int fd = -1; @@ -1838,9 +1932,9 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, * that with NFS we can't actually stat() the file. * The subsequent codepaths will still raise an error * if a truely fatal problem is hit */ - is_reg = 1; + is_reg = true; } else { - is_reg = S_ISREG(sb.st_mode); + is_reg = !!S_ISREG(sb.st_mode); } offset = sizeof(header) + header.xml_len; @@ -1935,7 +2029,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, is NFS, we assume it's a root-squashing NFS share, and that the security driver stuff would have failed anyway */ - bypassSecurityDriver = 1; + bypassSecurityDriver = true; } } @@ -1945,86 +2039,13 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, goto endjob; } - /* Allow qemu to access file */ - - if (!is_reg && - qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { - if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find cgroup for %s"), - vm->def->name); - goto endjob; - } - rc = virCgroupAllowDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RW); - qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc); - if (rc < 0) { - virReportSystemError(-rc, - _("Unable to allow device %s for %s"), - path, vm->def->name); - goto endjob; - } - } - - if ((!bypassSecurityDriver) && - virSecurityManagerSetSavedStateLabel(driver->securityManager, - vm, path) < 0) - goto endjob; - - if (header.compressed == QEMUD_SAVE_FORMAT_RAW) { - const char *args[] = { "cat", NULL }; - - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && - priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) { - rc = qemuMonitorMigrateToFd(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - fd); - } else { - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - } else { - const char *prog = qemudSaveCompressionTypeToString(header.compressed); - const char *args[] = { - prog, - "-c", - NULL - }; - qemuDomainObjEnterMonitorWithDriver(driver, vm); - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); - qemuDomainObjExitMonitorWithDriver(driver, vm); - } - - if (rc < 0) - goto endjob; - + /* Perform the migration */ + if (qemuDomainMigrateToFile(driver, vm, qemuCaps, fd, offset, path, + compressed, is_reg, bypassSecurityDriver) < 0) + goto cleanup; if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); - goto endjob; - } - - rc = qemuMigrationWaitForCompletion(driver, vm); - - if (rc < 0) - goto endjob; - - if ((!bypassSecurityDriver) && - virSecurityManagerRestoreSavedStateLabel(driver->securityManager, - vm, path) < 0) - VIR_WARN("failed to restore save state label on %s", path); - - if (cgroup != NULL) { - rc = virCgroupDenyDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RWM); - qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc); - if (rc < 0) - VIR_WARN("Unable to deny device %s for %s %d", - path, vm->def->name, rc); + goto cleanup; } ret = 0; @@ -2050,22 +2071,7 @@ endjob: if (rc < 0) VIR_WARN0("Unable to resume guest CPUs after save failure"); } - - if (cgroup != NULL) { - rc = virCgroupDenyDevicePath(cgroup, path, - VIR_CGROUP_DEVICE_RWM); - qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc); - if (rc < 0) - VIR_WARN("Unable to deny device %s for %s: %d", - path, vm->def->name, rc); - } - - if ((!bypassSecurityDriver) && - virSecurityManagerRestoreSavedStateLabel(driver->securityManager, - vm, path) < 0) - VIR_WARN("failed to restore save state label on %s", path); } - if (qemuDomainObjEndJob(vm) == 0) vm = NULL; } @@ -2078,7 +2084,6 @@ cleanup: unlink(path); if (event) qemuDomainEventQueue(driver, event); - virCgroupFree(&cgroup); return ret; } @@ -2282,9 +2287,6 @@ static int doCoreDump(struct qemud_driver *driver, { int fd = -1; int ret = -1; - qemuDomainObjPrivatePtr priv; - - priv = vm->privateData; /* Create an empty file with appropriate ownership. */ if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) { @@ -2293,6 +2295,10 @@ static int doCoreDump(struct qemud_driver *driver, goto cleanup; } + if (qemuDomainMigrateToFile(driver, vm, NULL, fd, 0, path, + compress, true, false) < 0) + goto cleanup; + if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to save file %s"), @@ -2300,42 +2306,7 @@ static int doCoreDump(struct qemud_driver *driver, goto cleanup; } - if (virSecurityManagerSetSavedStateLabel(driver->securityManager, - vm, path) < 0) - goto cleanup; - - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (compress == QEMUD_SAVE_FORMAT_RAW) { - const char *args[] = { - "cat", - NULL, - }; - ret = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, 0); - } else { - const char *prog = qemudSaveCompressionTypeToString(compress); - const char *args[] = { - prog, - "-c", - NULL, - }; - ret = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, 0); - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret < 0) - goto cleanup; - - ret = qemuMigrationWaitForCompletion(driver, vm); - - if (ret < 0) - goto cleanup; - - if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, - vm, path) < 0) - goto cleanup; + ret = 0; cleanup: if (ret != 0) -- 1.7.4

Spawn the compressor ourselves, instead of requiring the shell. * src/qemu/qemu_driver.c (qemuDomainMigrateToFile): Spawn compression helper process when needed. --- src/qemu/qemu_driver.c | 37 ++++++++++++++++++++++++++++++++----- 1 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a679b2c..02c3c48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1778,10 +1778,12 @@ qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, virCgroupPtr cgroup = NULL; int ret = -1; int rc; + virCommandPtr cmd = NULL; + int pipeFD[2] = { -1, -1 }; if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX && - compressed == QEMUD_SAVE_FORMAT_RAW) { + (compressed == QEMUD_SAVE_FORMAT_RAW || pipe(pipeFD) == 0)) { /* All right! We can use fd migration, which means that qemu * doesn't have to open() the file, so we don't have to futz * around with granting access or revoking it later. */ @@ -1842,9 +1844,26 @@ qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, "-c", NULL }; - rc = qemuMonitorMigrateToFile(priv->mon, - QEMU_MONITOR_MIGRATE_BACKGROUND, - args, path, offset); + if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) { + cmd = virCommandNewArgs(args); + virCommandSetInputFD(cmd, pipeFD[0]); + virCommandSetOutputFD(cmd, &fd); + if (virCommandRunAsync(cmd, NULL) < 0) { + qemuDomainObjExitMonitorWithDriver(driver, vm); + goto cleanup; + } + rc = qemuMonitorMigrateToFd(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + pipeFD[1]); + if (VIR_CLOSE(pipeFD[0]) < 0 || + VIR_CLOSE(pipeFD[1]) < 0) + VIR_WARN0("failed to close intermediate pipe"); + } else { + rc = qemuMonitorMigrateToFile(priv->mon, + QEMU_MONITOR_MIGRATE_BACKGROUND, + args, path, offset); + } } qemuDomainObjExitMonitorWithDriver(driver, vm); @@ -1852,13 +1871,21 @@ qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, goto cleanup; rc = qemuMigrationWaitForCompletion(driver, vm); - if (rc < 0) goto cleanup; + if (cmd && virCommandWait(cmd, NULL) < 0) + goto cleanup; + ret = 0; cleanup: + VIR_FORCE_CLOSE(pipeFD[0]); + VIR_FORCE_CLOSE(pipeFD[1]); + /* FIXME - virCommandWait can overwrite errors; need to add + * virCommandKill that does the job silently */ + ignore_value(virCommandWait(cmd, NULL)); + virCommandFree(cmd); if ((!bypassSecurityDriver) && virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path) < 0) -- 1.7.4