[libvirt] [PATCH] Fix handling of disk backing stores with cgroups

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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