[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.
s/its/it's/


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]