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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list