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.