On 05/07/2010 09:24 AM, Daniel P. Berrange wrote:
> The cgroups ACL code was only allowing the primary disk image.
> It is possible to chain images together, so we need to search
> for backing stores and add them to the ACL too. Since the ACL
> only handles block devices, we ignore the EINVAL we get from
> plain files. In addition it was missing code to teardown the
> cgroup when hot-unplugging a disk
>
> * src/qemu/qemu_driver.c: Allow backing stores in cgroup ACLs
> and add missing teardown code in unplug path
> +static int qemuSetupDiskCgroup(virCgroupPtr cgroup,
> + virDomainObjPtr vm,
> + virDomainDiskDefPtr disk)
> +{
> + char *path = disk->src;
> + int ret = -1;
> +
> + while (path != NULL) {
> + virStorageFileMetadata meta;
> + int rc;
> +
> + VIR_DEBUG("Process path %s for disk", path);
> + rc = virCgroupAllowDevicePath(cgroup, path);
Except for this line...
> + if (rc != 0) {
> + /* Get this for non-block devices */
> + if (rc == -EINVAL) {
> + VIR_DEBUG("Ignoring EINVAL for %s", path);
> + } else {
> + virReportSystemError(-rc,
> + _("Unable to allow device %s for %s"),
...and this string...
> + path, vm->def->name);
> + if (path != disk->src)
> + VIR_FREE(path);
> + goto cleanup;
> + }
> + }
> +
> + memset(&meta, 0, sizeof(meta));
> +
> + rc = virStorageFileGetMetadata(path, &meta);
> +
> + if (path != disk->src)
> + VIR_FREE(path);
> + path = NULL;
> +
> + if (rc < 0)
> + goto cleanup;
> +
> + path = meta.backingStore;
> + } while (path != NULL);
> +
> + ret = 0;
> +
> +cleanup:
> + return ret;
> +}
> +
> +
> +static int qemuTeardownDiskCgroup(virCgroupPtr cgroup,
> + virDomainObjPtr vm,
> + virDomainDiskDefPtr disk)
these two functions are identical. Is it worth consolidating them into
a helper function that takes a bool parameter to decide whether to
allow/deny, so that if we end up having to make any logic fixes in the
future, we only have to touch a single place?
Other than that question, I didn't see anything blatantly wrong in this
patch, so ACK.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
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