[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Fix domain restore for files on root-squash NFS.

Thanks for the quick response!

On 02/23/2010 03:32 PM, Eric Blake wrote:
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.

Ah, yes. Those terrible typos that never seem worth the effort to fix after they're already checked in. Thanks for nipping it in the bud!

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

Mostly because I was following the example of util.c:__virExec(), which does the same thing. Following examples seems to get me into nearly as much trouble as ignoring them ;-)

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

Interesting point. If we can't rely on that, then __virExec() should be changed too. Of course this makes maintenance a bit more error prone, since we would have to make sure that *every* jump to the cleanup that happened prior to creating the pipe would set the pipe fds to -1 (or just leave the initialization in, and re-set them to -1 when pipe failed; that's probably the safest thing to do).

+        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).


I think I just heard someone volunteering! ;-)

+    /* 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?

What's the proper emoticon for blushing in embarrassment? Bonehead typo ;-) Thanks for pointing it out.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]