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

[Laine Stump]

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.