On 03/02/2013 01:30 PM, Doug Goldstein wrote: > On Sat, Mar 2, 2013 at 8:30 AM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: >> Hey, >> >> On Wed, Feb 27, 2013 at 07:20:56PM +0800, Daniel Veillard wrote: >>> I have just tagged git and pushed the tarball. The rpms for F17 >>> are at the usual place too: >>> ftp://libvirt.org/libvirt/ >>> >>> I gave a try to the set of rpms and this seems to work fine for >>> relatively simple tests. >>> Please give it more testing and let's keep change to git to >>> bug fixes to minimize risks for 1.0.3. >>> If everything goes well I will push 1.0.3 on Friday, >> I've found a pretty bad issue with rc2/git head and Boxes, creating new >> boxes fails with >> 2013-03-02 14:04:22.028+0000: 15681: error : >> qemuDomainCheckDiskPresence:1837 : cannot access file >> '/home/teuf/isos/msdn/win7/fr_windows_7_home_premium_n_with_sp1_x64_dvd_u_676833.iso': >> Opération non permise >> >> Looking more into it, this seems to be caused by >> >> commit f506a4c115c44003455cb956861836a46425f97b >> Date: Thu Jan 31 13:18:45 2013 -0500 >> >> util: make virSetUIDGID a NOP only when uid or gid is -1 >> >> qemuDomainCheckDiskPresence calls virFileAccessibleAs with (user, group) >> being (0, 0) as Boxes is using qemu:///session (these are set to 0 by >> virQEMUDriverConfigNew in the session case). >> >> virFileAccessibleAs calls virSetUIDGID(0, 0) which used to be a noop before >> the commit above, but is now trying to call setreuid/setregid, which fails. >> >> I tried reverting this patch, but then probing qemu binaries during >> libvirtd startup fails so not a good workaround :) >> >> The patch below works for me, but it needs careful review as this is code I >> don't know at all and I've only lightly tested it. This issue is a 1.0.3 >> blocker as far as Boxes is concerned. >> >> Thanks, >> >> Christophe >> >> >> From 6474eb9dc04dcaa29450116ddfb76aefdaffd4f6 Mon Sep 17 00:00:00 2001 >> From: Christophe Fergeau <cfergeau@xxxxxxxxxx> >> Date: Sat, 2 Mar 2013 15:19:47 +0100 >> Subject: [PATCH] qemu: Use -1 as unpriviledged uid/gid >> >> Commit f506a4c1 changed virSetUIDGID() to be a noop >> when uid/gid are -1, while it used to be a noop when >> they are <= 0. >> >> The changes in this commit broke creating new VMs in GNOME Boxes >> as qemuDomainCheckDiskPresence gets called during domain creation/startup, >> which in turn calls virFileAccessibleAs which fails after calling >> virSetUIDGID(0, 0) (Boxes uses session libvirtd). >> >> This commit changes virQEMUDriverConfigNew to use -1 as the unpriviledged >> uid/gid, and adjusts one code path that expected 0 in this case. I've also >> looked at the various places where cfg->user is used, and they all >> seem to handle -1 correctly. >> --- >> src/qemu/qemu_conf.c | 4 ++-- >> src/qemu/qemu_domain.c | 4 ++-- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c >> index f3b09cf..3ef3499 100644 >> --- a/src/qemu/qemu_conf.c >> +++ b/src/qemu/qemu_conf.c >> @@ -129,8 +129,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) >> if (virGetGroupID(QEMU_GROUP, &cfg->group) < 0) >> goto error; >> } else { >> - cfg->user = 0; >> - cfg->group = 0; >> + cfg->user = (uid_t)-1; >> + cfg->group = (gid_t)-1; >> } >> cfg->dynamicOwnership = privileged; >> > I'll agree this change should fix it from a code inspection. So ACK > this hunk. Really starting to think we need some tests for this. Given > the late phase in the 1.0.3 release cycle (post freeze), do you have a > specific test case I can use to verify this? > >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 0e56596..1ecc8fa 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -1301,8 +1301,8 @@ void qemuDomainObjCheckTaint(virQEMUDriverPtr driver, >> >> if (cfg->privileged && >> (!cfg->clearEmulatorCapabilities || >> - cfg->user == 0 || >> - cfg->group == 0)) >> + cfg->user == (uid_t)-1 || >> + cfg->group == (gid_t)-1)) >> qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_HIGH_PRIVILEGES, logFD); >> >> if (obj->def->namespaceData) { >> -- >> 1.8.1.2 > You self NACK'd this part. But I'll agree with the NACK here. This > will actually break the checks here for the taint. In fact this code > chunk shows that using 0 for user & group to mean don't care was > wrong. As the author of the patch that uncovered/caused the breakage, I'll second the ACK on the first hunk and NACK on the second. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list