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,

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(a)redhat.com&gt; 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;

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

On Sat, Mar 2, 2013 at 8:30 AM, Christophe Fergeau <cfergeau(a)redhat.com&gt; 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(a)redhat.com&gt; > 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.