Re: [libvirt] [PATCH] Fix domain restore for files on root-squash NFS.
According to Laine Stump on 2/23/2010 12:54 PM:
Minor nits (I have not validated the overall patch in context; this is
just from a quick high-level review)
If qemudDomainRestore fails to open the domain save file, create a
pipe, then fork a process that does setuid(qemu_user) and opens the
file, then reads this file and stuffs it into the pipe. the parnet
s/. the parnet/. The parent/
libvirtd process will use the other end of the pipe as its fd, then
reap the child process after its done reading.
s/its/it's/
This makes domain restore work on a root-squash NFS share that is only
visible to the qemu user.
---
src/qemu/qemu_driver.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 155 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ac6ef6a..f909a59 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4659,6 +4659,135 @@ 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, pid_t *childpid) {
+ int pipefd[2] = {-1, -1};
Why the explicit initialization...
+ int fd = -1;
+
+ *childpid = -1;
+
+ if (pipe(pipefd) < 0) {
...since I see nothing in POSIX that guarantees that pipe() will preserve
your initialized values across failure. I think it's better to explicitly
set things to -1 on the failure path, rather than initializing that way.
+ virReportSystemError(errno,
+ _("failed to create pipe to read '%s'"),
+ path);
+ goto parentcleanup;
+ }
+
+ int forkRet = virFork(childpid);
Unrelated to your patch, but we should probably start using pipe2 and
atomically marking pipes as CLOEXEC, so that use of libvirt in a
multithreaded environment does not have a race window where unrelated
threads can leak libvirt's fds across an exec (well, for new enough
kernels anyway; at least gnulib has a pipe2 fallback for older kernels).
...
+ /* 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;
+ int bufsize = 1024 * 1024;
+ int bytesread;
+
+ /* child doesn't need the read side of the pipe */
+ close(pipefd[0]);
+
+ if (forkRet < 0) {
+ exit_code = errno;
+ virReportSystemError(errno,
+ _("failed in child after forking to read
'%s'"),
+ path);
+ goto childcleanup;
+ }
+
+ if (setuid(uid) != 0) {
+ exit_code = errno;
+ virReportSystemError(errno,
+ _("cannot setuid(%d) to read '%s'"),
+ getuid(), path);
Is that the right uid to be reporting in the error message?
Nothing else jumped out at me as suspicious.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 320 bytes)