12:09 p.m.
FYI, I just verified that the restore failures I was seeing after
applying this patch were actually happening *without* the patch as well,
and are unrelated to domain save (it's a race condition in domain
restore that needs to be dealt with separately), so this patch is okay
to put in
I verified I've been testing with an unmodified form of this patch,
*EXCEPT* that I hadn't done make syntax-check on it (since I didn't
really think that it was working code at the time ;-)), and there is one
occurence of white-space at the end of a line.
Should I resend with that change? Or do you want to just fix it up?
Also, notice that this patch saves the domain file with 0660 permission
(umask will normally turn it into 0640) as we had thought that would be
part of the way to allow restore from a root-squashed NFS server (just
make sure that the reader had group read permissions). Now it seems we
will be using the trick of running the restore code setuid instead, so
the 0660 mode will no longer necessary. Should I revise this patch to
create the file as 0600, or just do that as part of the upcoming domain
restore patch?
(a couple more comments are inline)
On 02/22/2010 11:57 AM, Daniel Veillard wrote:
On Fri, Feb 19, 2010 at 02:30:08AM -0500, Laine Stump wrote:
> Move *all* file operations related to creation and writing of libvirt
> header to the domain save file into a hook function that is called by
> virFileOperation. First try to call virFileOperation as root. If that
> fails with EACCESS, and (in the case of Linux) statfs says that we're
> trying to save the file on an NFS share, rerun virFileOperation,
> telling it to fork a child process and setuid to the qemu user. This
> is the only way we can successfully create a file on a root-squashed
> NFS server.
>
> This patch (along with setting dynamic_ownership=0 in qemu.conf)
> makes qemudDomainSave work on root-squashed NFS.
>
[...]
> +#ifdef __linux__
> + /* On Linux we can also verify the FS-type of the directory. */
> + struct statfs st;
> + char *dirpath, *p;
>
[...]
> +
> + if ((p = strrchr(dirpath, '/')) == NULL) {
> + qemuReportError(VIR_ERR_INVALID_ARG,
> + _("Invalid path '%s' for domain save
file"),
> + path);
> + VIR_FREE(dirpath);
> + goto endjob;
> + }
> +
> + if (p == dirpath)
> + *(p+1) = '\0';
> + else
> + *p = '\0';
>
I wasn't sure why that was needed but since the man page states
"path is the pathname of any file within the mounted file system"
so okay we have to get back to the directory which is an existing file.
Heh. Yeah, I tried to do it with the filename first, and that didn't
work, so we're stuck with the silly char* calisthenics.
> + if (statfs(dirpath,&st) == -1) {
> + virReportSystemError(rc,
> + _("Failed to create domain save file
'%s'"
> + " statfs of '%s' failed
(%d)"),
> + path, errno);
> + VIR_FREE(dirpath);
> + goto endjob;
> + }
> +
> + VIR_FREE(dirpath);
> +
> + if (st.f_type != NFS_SUPER_MAGIC) {
> + virReportSystemError(rc,
> + _("Failed to create domain save file
'%s' (fstype 0x%X"),
> + path, st.f_type);
> + goto endjob;
> + }
>
Okay, we really need to error out if not on NFS (I can't think of
another kind of filesystem which would exhibit the same behaviour)
There is a bugzilla report that talks about similar behavior on an smbfs
share (they were having problems with storage volumes, but it seemed to
smell the same) but I haven't tried it out. If it turns out to work
similarly, we can add a case for SMBFS_SUPER_MAGIC.
My problem with this piece of code is that it's Linux-only. Other
systems either don't have statfs, or it is implemented differently and
doesn't have the f_type property. It would be nice if there was a method
that worked the same on all platforms...
> +#endif
> +
> + /* Retry creating the file as driver->user */
> +
> + if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
> + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
> + driver->user, driver->group,
> + qemudDomainSaveFileOpHook,&hdata,
> + VIR_FILE_OP_AS_UID)) != 0) {
> + virReportSystemError(rc, _("Error from child process creating
'%s'"),
> + path);
> + goto endjob;
> + }
> +
> + /* Since we had to setuid to create the file, and the fstype
> + is NFS, we assume it's a root-squashing NFS share, and that
> + the security driver stuff would have failed anyway */
> +
> + bypassSecurityDriver = 1;
> }
> - fd = -1;
>
> - if (driver->securityDriver&&
> +
> + if ((!bypassSecurityDriver)&&
> + driver->securityDriver&&
> driver->securityDriver->domainSetSavedStateLabel&&
> driver->securityDriver->domainSetSavedStateLabel(vm, path) == -1)
> goto endjob;
> @@ -4081,7 +4181,8 @@ static int qemudDomainSave(virDomainPtr dom,
> if (rc< 0)
> goto endjob;
>
> - if (driver->securityDriver&&
> + if ((!bypassSecurityDriver)&&
> + driver->securityDriver&&
> driver->securityDriver->domainRestoreSavedStateLabel&&
> driver->securityDriver->domainRestoreSavedStateLabel(vm, path) ==
-1)
> goto endjob;
> @@ -4106,8 +4207,6 @@ endjob:
> vm = NULL;
>
> cleanup:
> - if (fd != -1)
> - close(fd);
> VIR_FREE(xml);
> if (ret != 0)
> unlink(path);
>
Okay, ACK,
Daniel