[libvirt] [PATCH] snapshot: allow block devices past cgroup

[Eric Blake]

On 05/08/2012 12:42 PM, Laine Stump wrote:
> On 05/07/2012 07:00 PM, Eric Blake wrote:
>> It turns out that when cgroups are enabled, the use of a block device
>> for a snapshot target was failing with EPERM due to libvirt failing
>> to add the block device to the cgroup whitelist.  See also
>> https://bugzilla.redhat.com/show_bug.cgi?id=810200
>>
>> * src/qemu/qemu_driver.c
>> (qemuDomainSnapshotCreateSingleDiskActive)
>> (qemuDomainSnapshotUndoSingleDiskActive): Account for cgroup.
>> (qemuDomainSnapshotCreateDiskActive): Update caller.
>> ---
>> @@ -9948,8 +9949,15 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver,
>>
>>      if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0)
>>          goto cleanup;
>> +    if (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) {
> 
> As far as I can see, you can't get to here if cgroup is NULL.

cgroup can be NULL; see below...

> Technically there's nothing wrong with checking it, but isn't it
> unnecessary?

No, it's necessary (unless we fix qemuSetupDiskCgroup to be a no-op when
cgroup is NULL).


>> @@ -10084,6 +10095,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
>>      int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */
>>      bool atomic = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
>>      bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
>> +    virCgroupPtr cgroup = NULL;
>>
>>      if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
>>          return -1;
>> @@ -10094,6 +10106,14 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
>>          goto endjob;
>>      }
>>
>> +    if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES) &&
>> +        virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0)) {
>> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
>> +                        _("Unable to find cgroup for %s"),
>> +                        vm->def->name);
>> +        goto endjob;
>> +    }
>> +
> 
> 
> By the time you get here, you're guaranteed the cgroup is non-NULL, right?

No.  If qemuCgroupControllerActive() returns false (possible if your
qemu.conf left "devices" out of cgroup_controllers, or if you don't have
cgroups mounted in the kernel at all), then we never call
virCgroupForDomain to populate cgroup with a non-NULL value.

> 
> 
> Also, looking beyond the context provided by git - virCGroupFree() is
> called in cleanup, which is higher up in the function than end_job, but
> there are 2 occurrences of goto end_job past where you get the cgroup -
> if you encountered an error and took either of those goto's, you would
> end up leaking cgroup.

Eek; I'll fix that and provide a v2.

>> @@ -10156,7 +10176,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
>>              }
>>          }
>>
>> -        ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm,
>> +        ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, cgroup,
> 
> So back to the other topic - this is the only place this function is
> called, and you're guaranteed that cgroup will be non-NULL, so I don't
> think qemuDomainSnapshotCreateSingleDiskActive() actually needs to check
> for NULL cgroup.

Calling with NULL cgroup is permissible.

>> @@ -10214,6 +10234,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
>>      }
>>
>>  cleanup:
>> +    if (cgroup)
>> +        virCgroupFree(&cgroup);
> 
> I think this either needs to be moved down to end_job, or the to goto
> end_job's I mentioned above need to goto cleanup (I think I prefer the
> former).

cleanup falls through to endjob - we have to use cleanup if we pause the
cpus or if we modify state that needs saving.  But since we do indeed
have a 'goto endjob' that occurs after cgroup might be set, we do indeed
need to move the virCgroupFree() into the endjob label.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120508/e7944379/attachment-0001.sig>