On Wed, Mar 09, 2011 at 06:45:44PM -0700, Eric Blake wrote:

* 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(-)

ACK

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; +}

It is a nice idea todo the SendFileHAndle/CloseFileHandle calls from here, rather than in the qemu_driver code itself. It makes the latter much clearer - we should think about whether it makes sense to rewrite the NIC hotplug code to work this way too. 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 :|

This is also a bug fix - on the error path, qemu_hotplug would leave the configfd file leaked into qemu. At least the next attempt to hotplug a PCI device would reuse the same fdname, and when the qemu getfd monitor command gets a new fd by the same name as an earlier one, it closes the earlier one, so there is no risk of qemu running out of fds. * src/qemu/qemu_monitor.h (qemuMonitorAddDeviceWithFd): New prototype. * src/qemu/qemu_monitor.c (qemuMonitorAddDevice): Move guts... (qemuMonitorAddDeviceWithFd): ...to new function, and add support for fd passing. * src/qemu/qemu_hotplug.c (qemuDomainAttachHostPciDevice): Use it to simplify code. ---

...

It is a nice idea todo the SendFileHAndle/CloseFileHandle calls from here, rather than in the qemu_driver code itself. It makes the latter much clearer - we should think about whether it makes sense to rewrite the NIC hotplug code to work this way too.

Agreed; I'll submit a patch for that shortly.

Thanks for requesting this - I found and plugged a resource leak in the process. NIC hotplug cleanup coming next. src/qemu/qemu_hotplug.c | 19 +++++++++---------- src/qemu/qemu_monitor.c | 24 +++++++++++++++++++++--- src/qemu/qemu_monitor.h | 5 +++++ 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e4ba526..40e4159 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -829,21 +829,19 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &hostdev->info) < 0) goto error; if (qemuCapsGet(qemuCaps, QEMU_CAPS_PCI_CONFIGFD)) { - configfd = qemuOpenPCIConfig(hostdev); + if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("pci configfd file cannot be attached: " + "qemu is not using a unix socket monitor")); + } else { + configfd = qemuOpenPCIConfig(hostdev); + } if (configfd >= 0) { if (virAsprintf(&configfd_name, "fd-%s", hostdev->info.alias) < 0) { virReportOOMError(); goto error; } - - qemuDomainObjEnterMonitorWithDriver(driver, vm); - if (qemuMonitorSendFileHandle(priv->mon, configfd_name, - configfd) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto error; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); } } @@ -858,7 +856,8 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, goto error; qemuDomainObjEnterMonitorWithDriver(driver, vm); - ret = qemuMonitorAddDevice(priv->mon, devstr); + ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, + configfd, configfd_name); qemuDomainObjExitMonitorWithDriver(driver, vm); } else { virDomainDevicePCIAddress guestAddr; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index da38096..f2b1721 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2016,10 +2016,13 @@ int qemuMonitorDelDevice(qemuMonitorPtr mon, } -int qemuMonitorAddDevice(qemuMonitorPtr mon, - const char *devicestr) +int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, + const char *devicestr, + int fd, + const char *fdname) { - VIR_DEBUG("mon=%p device=%s", mon, devicestr); + VIR_DEBUG("mon=%p device=%s fd=%d fdname=%s", mon, devicestr, fd, + NULLSTR(fdname)); int ret; if (!mon) { @@ -2028,13 +2031,28 @@ int qemuMonitorAddDevice(qemuMonitorPtr mon, return -1; } + if (fd >= 0 && qemuMonitorSendFileHandle(mon, fdname, fd) < 0) + return -1; + if (mon->json) ret = qemuMonitorJSONAddDevice(mon, devicestr); else ret = qemuMonitorTextAddDevice(mon, devicestr); + + if (ret < 0 && fd >= 0) { + if (qemuMonitorCloseFileHandle(mon, fdname) < 0) + VIR_WARN("failed to close device handle '%s'", fdname); + } + return ret; } +int qemuMonitorAddDevice(qemuMonitorPtr mon, + const char *devicestr) +{ + return qemuMonitorAddDeviceWithFd(mon, devicestr, -1, NULL); +} + int qemuMonitorAddDrive(qemuMonitorPtr mon, const char *drivestr) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7bea083..a20ff8e 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -390,6 +390,11 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, int qemuMonitorAddDevice(qemuMonitorPtr mon, const char *devicestr); +int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, + const char *devicestr, + int fd, + const char *fdname); + int qemuMonitorDelDevice(qemuMonitorPtr mon, const char *devalias); -- 1.7.4

On 03/15/2011 06:19 PM, Chris Wright wrote:

* Eric Blake (eblake@redhat.com) wrote:

...

@@ -829,21 +829,19 @@ int qemuDomainAttachHostPciDevice(struct qemud_driver *driver, if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &hostdev->info) < 0) goto error; if (qemuCapsGet(qemuCaps, QEMU_CAPS_PCI_CONFIGFD)) { - configfd = qemuOpenPCIConfig(hostdev); + if (priv->monConfig->type != VIR_DOMAIN_CHR_TYPE_UNIX) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("pci configfd file cannot be attached: " + "qemu is not using a unix socket monitor"));

Should that be in common sendfd code?

Yeah, factoring that into a common location _would_ be nicer. I'll whip up a patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 03/09/2011 08:45 PM, Eric Blake wrote:

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) {

As mentioned in other mail, if you change SOCK_DGRAM to SOCK_STREAM, the parent will no longer hang on recvmsg() if the child neglects to call sendmsg().

+ 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]);

Why not just always close pair[1]? (right now you're depending on _exit() to close it)

+ 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,

ACK with the above two nits fixed.

On Wed, Mar 09, 2011 at 06:45:46PM -0700, Eric Blake wrote:

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);

Ok, so this is the bit which causes a regression on NFS rootsquash, if the path isn't readable by root.

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);

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 :|

On 03/09/2011 08:45 PM, Eric Blake wrote:

This patch intentionally doesn't change indentation, in order to make it easier to review the real changes.

* src/util/util.h (VIR_FILE_OP_RETURN_FD, virFileOperationHook): Delete. (virFileOperation): Rename... (virFileOpenAs): ...and reduce parameters. * src/util/util.c (virFileOperationNoFork, virFileOperation): Rename and simplify. * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Adjust caller. * src/storage/storage_backend.c (virStorageBackendCreateRaw): Likewise. * src/libvirt_private.syms: Reflect rename. --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 20 +++----- src/storage/storage_backend.c | 12 ++-- src/util/util.c | 103 +++++++++++++---------------------------- src/util/util.h | 15 ++---- 5 files changed, 53 insertions(+), 99 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c0da78e..a9f7e39 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -876,8 +876,8 @@ virFileIsExecutable; virFileLinkPointsTo; virFileMakePath; virFileMatchesNameSuffix; +virFileOpenAs; virFileOpenTty; -virFileOperation; virFilePid; virFileReadAll; virFileReadLimFD; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a73c5b9..06bc969 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1880,11 +1880,9 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, goto endjob; } } else { - if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY, - S_IRUSR|S_IWUSR, - getuid(), getgid(), - NULL, NULL, - VIR_FILE_OP_RETURN_FD))< 0) { + if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY, + S_IRUSR|S_IWUSR, + getuid(), getgid(), 0))< 0) { /* If we failed as root, and the error was permission-denied (EACCES or EPERM), assume it's on a network-connected share where root access is restricted (eg, root-squashed NFS). If the @@ -1918,7 +1916,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom,

case 0: default: - /* local file - log the error returned by virFileOperation */ + /* local file - log the error returned by virFileOpenAs */ virReportSystemError(-rc, _("Failed to create domain save file '%s'"), path); @@ -1929,12 +1927,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom,

/* Retry creating the file as driver->user */

- if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY, - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, - driver->user, driver->group, - NULL, NULL, - (VIR_FILE_OP_AS_UID | - VIR_FILE_OP_RETURN_FD)))< 0) { + if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, + driver->user, driver->group, + VIR_FILE_OPEN_AS_UID))< 0) { virReportSystemError(-fd, _("Error from child process creating '%s'"), path); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c7c5ccd..7a3a2b8 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -364,14 +364,14 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,

uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid; gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid; - operation_flags = VIR_FILE_OP_FORCE_PERMS | VIR_FILE_OP_RETURN_FD; + operation_flags = VIR_FILE_OPEN_FORCE_PERMS; if (pool->def->type == VIR_STORAGE_POOL_NETFS) - operation_flags |= VIR_FILE_OP_AS_UID; + operation_flags |= VIR_FILE_OPEN_AS_UID;

- if ((fd = virFileOperation(vol->target.path, - O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode, uid, gid, - NULL, NULL, operation_flags))< 0) { + if ((fd = virFileOpenAs(vol->target.path, + O_RDWR | O_CREAT | O_EXCL, + vol->target.perms.mode, uid, gid, + operation_flags))< 0) { virReportSystemError(-fd, _("cannot create path '%s'"), vol->target.path); diff --git a/src/util/util.c b/src/util/util.c index 6bf3219..137cdb9 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1355,10 +1355,10 @@ virFileIsExecutable(const char *file)

#ifndef WIN32 /* return -errno on failure, or 0 on success */ -static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, - uid_t uid, gid_t gid, - virFileOperationHook hook, void *hookdata, - unsigned int flags) { +static int +virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) +{ int fd = -1; int ret = 0; struct stat st; @@ -1381,7 +1381,7 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, path, (unsigned int) uid, (unsigned int) gid); goto error; } - if ((flags& VIR_FILE_OP_FORCE_PERMS) + if ((flags& VIR_FILE_OPEN_FORCE_PERMS) && (fchmod(fd, mode)< 0)) { ret = -errno; virReportSystemError(errno, @@ -1389,17 +1389,7 @@ static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, path, mode); goto error; } - 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); - goto error; - } + return fd;

error: VIR_FORCE_CLOSE(fd); @@ -1446,35 +1436,27 @@ error: }

/** - * virFileOperation: + * virFileOpenAs: * @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 + * @flags: bit-wise or of VIR_FILE_OPEN_* 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 + * If @flags includes VIR_FILE_OPEN_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 + * VIR_FILE_OPEN_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, - unsigned int flags) { + * with different permissions. The return value (if non-negative) + * is the file descriptor, left open. Return -errno on failure. */ +int +virFileOpenAs(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) +{ struct stat st; pid_t pid; int waitret, status, ret = 0; @@ -1487,11 +1469,10 @@ int virFileOperation(const char *path, int openflags, mode_t mode, struct iovec iov; int forkRet;

- if ((!(flags& VIR_FILE_OP_AS_UID)) + if ((!(flags& VIR_FILE_OPEN_AS_UID)) || (getuid() != 0) || ((uid == 0)&& (gid == 0))) { - return virFileOperationNoFork(path, openflags, mode, uid, gid, - hook, hookdata, flags); + return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); }

/* parent is running as root, but caller requested that the @@ -1499,7 +1480,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode, * following dance avoids problems caused by root-squashing * NFS servers. */

- if (flags& VIR_FILE_OP_RETURN_FD) { + { if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pair)< 0) { ret = -errno; virReportSystemError(errno, @@ -1529,7 +1510,7 @@ 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 { @@ -1565,20 +1546,13 @@ int virFileOperation(const char *path, int openflags, mode_t mode, goto parenterror; } ret = -WEXITSTATUS(status); - if (!WIFEXITED(status) || (ret == -EACCES) || - ((flags& VIR_FILE_OP_RETURN_FD)&& fd == -1)) { + if (!WIFEXITED(status) || (ret == -EACCES) || fd == -1) { /* fall back to the simpler method, which works better in * some cases */ - return virFileOperationNoFork(path, openflags, mode, uid, gid, - hook, hookdata, flags); + return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags); } - if (!ret&& (flags& VIR_FILE_OP_RETURN_FD)) { - if ((hook)&& ((ret = hook(fd, hookdata)) != 0)) { - VIR_FORCE_CLOSE(fd); - goto parenterror; - } + if (!ret) ret = fd; - } parenterror: return ret; } @@ -1630,7 +1604,7 @@ parenterror: path, (unsigned int) uid, (unsigned int) gid); goto childerror; } - if ((flags& VIR_FILE_OP_FORCE_PERMS) + if ((flags& VIR_FILE_OPEN_FORCE_PERMS) && (fchmod(fd, mode)< 0)) { ret = -errno; virReportSystemError(errno, @@ -1638,7 +1612,7 @@ parenterror: path, mode); goto childerror; } - if (flags& VIR_FILE_OP_RETURN_FD) { + { VIR_FORCE_CLOSE(pair[0]);

This VIR_FORCE_CLOSE(pair[0]) should be moved up to the very start of the child code - otherwise any error prior to this point will leave it dangling for _exit() to clean up.

memcpy(CMSG_DATA(cmsg),&fd, sizeof(fd));

@@ -1651,17 +1625,6 @@ parenterror: VIR_FORCE_CLOSE(pair[1]);

A leftover from 05/15 - I think thie VIR_FORCE_CLOSE should happen at/after childerror:. That way it will always get cleaned up, whether or not there is an error at any time during child execution.

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; @@ -1783,17 +1746,15 @@ childerror: #else /* WIN32 */

/* return -errno on failure, or 0 on success */ -int virFileOperation(const char *path ATTRIBUTE_UNUSED, - int openflags ATTRIBUTE_UNUSED, - mode_t mode ATTRIBUTE_UNUSED, - uid_t uid ATTRIBUTE_UNUSED, - gid_t gid ATTRIBUTE_UNUSED, - virFileOperationHook hook ATTRIBUTE_UNUSED, - void *hookdata ATTRIBUTE_UNUSED, - unsigned int flags ATTRIBUTE_UNUSED) +int virFileOpenAs(const char *path ATTRIBUTE_UNUSED, + int openflags ATTRIBUTE_UNUSED, + mode_t mode ATTRIBUTE_UNUSED, + uid_t uid ATTRIBUTE_UNUSED, + gid_t gid ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) { virUtilError(VIR_ERR_INTERNAL_ERROR, - "%s", _("virFileOperation is not implemented for WIN32")); + "%s", _("virFileOpenAs is not implemented for WIN32"));

return -ENOSYS; } diff --git a/src/util/util.h b/src/util/util.h index 3970d58..cecd348 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -125,16 +125,13 @@ bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1); char *virFileSanitizePath(const char *path);

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), + VIR_FILE_OPEN_NONE = 0, + VIR_FILE_OPEN_AS_UID = (1<< 0), + VIR_FILE_OPEN_FORCE_PERMS = (1<< 1), }; -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) +int virFileOpenAs(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;

enum {

Aside from the two problems with the locations of VIR_FORCE_CLOSE(pair[n]), this all looks fine, and hasn't broken either backing store or save images on root-squash NFS, so ACK with those two changes.

On Wed, Mar 09, 2011 at 07:18:31PM -0700, Eric Blake wrote:

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; +}

I think it could be worth moving this method into qemu_migrate.h/.c now it is decoupled from the public API objects/methods. ACK to the refactoring regardless. 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 :|

SELinux labeling and cgroup ACLs aren't required if we hand a pre-opened fd to qemu. All the more reason to love fd: migration. * src/qemu/qemu_driver.c (qemuDomainMigrateToFile): Skip steps that are irrelevant in fd migration. --- src/qemu/qemu_driver.c | 65 +++++++++++++++++++++++++++++------------------ 1 files changed, 40 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2422482..a679b2c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1779,35 +1779,52 @@ qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, 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; + if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX && + compressed == QEMUD_SAVE_FORMAT_RAW) { + /* 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. */ + is_reg = true; + bypassSecurityDriver = true; + } else { + /* Phooey - we have to fall back on exec migration, where qemu + * has to popen() the file by name. We might also stumble on + * the qemu race where it does a wait() that botches pclose(). + * FIXME: can we reliably detect and ignore that particular + * failure where the migration completes but the reported + * status is wrong? */ + 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; + } } - 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); + + if ((!bypassSecurityDriver) && + virSecurityManagerSetSavedStateLabel(driver->securityManager, + vm, path) < 0) goto cleanup; - } } - if ((!bypassSecurityDriver) && - virSecurityManagerSetSavedStateLabel(driver->securityManager, - vm, path) < 0) - goto cleanup; - + qemuDomainObjEnterMonitorWithDriver(driver, vm); 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, @@ -1818,7 +1835,6 @@ qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, offset); } - qemuDomainObjExitMonitorWithDriver(driver, vm); } else { const char *prog = qemudSaveCompressionTypeToString(compressed); const char *args[] = { @@ -1826,12 +1842,11 @@ qemuDomainMigrateToFile(struct qemud_driver *driver, virDomainObjPtr vm, "-c", NULL }; - qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorMigrateToFile(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args, path, offset); - qemuDomainObjExitMonitorWithDriver(driver, vm); } + qemuDomainObjExitMonitorWithDriver(driver, vm); if (rc < 0) goto cleanup; -- 1.7.4