[libvirt] Release candidate 2 of 1.0.3 is available

[Laine Stump]

On 03/02/2013 01:30 PM, Doug Goldstein wrote:
> On Sat, Mar 2, 2013 at 8:30 AM, Christophe Fergeau <cfergeau at redhat.com> 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 at redhat.com>
>> 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.