Re: [libvirt] [PATCH 4/7] Place every QEMU guest in a private cgroup

[Daniel Veillard <veillard redhat com>]

On Fri, Jul 17, 2009 at 09:04:26AM -0400, Daniel P. Berrange wrote:
> * src/qemu_driver.c: Place guest in cgroup upon startup. Remove
>   cgroup upon shutdown
> ---
>  src/qemu_driver.c |  105 +++++++++++++++++++++++++++++++++++++++++++++++++----
[...]

> +        virReportSystemError(conn, -rc,
> +                             _("Unable to create cgroup for %s"), vm->def->name);

  hum, there is a missing line split here, I think I saw one similar in
  patch 3 but it was less obvious


> +static int qemuAddToCgroup(virDomainDefPtr def)
> +{
> +    virCgroupPtr cgroup = NULL;
> +    int rc;
> +
> +    if (virCgroupHaveSupport() != 0)
> +        return 0; /* Not supported, so claim success */
> +
> +    rc = virCgroupForDomain(def, "qemu", &cgroup);
> +    if (rc != 0) {
> +        virReportSystemError(NULL, -rc, _("unable to find cgroup for domain %s"), def->name);
> +        return -1;
> +    }
> +
> +    rc = virCgroupAddTask(cgroup, getpid());
> +    if (rc != 0) {
> +        virReportSystemError(NULL, -rc, _("unable to add domain %s task %d to cgroup"), def->name, getpid());
> +        virCgroupFree(&cgroup);
> +        return -1;
> +    }

  A couple more, and I would define itn ret = 0; and factorize the
  cleanup on exit

> +
> +    virCgroupFree(&cgroup);
> +    return 0;
> +}
> +
> +
[...]
>  
> -struct gemudHookData {
> -        virConnectPtr conn;
> -        virDomainObjPtr vm;
> -        struct qemud_driver *driver;
> +struct qemudHookData {
> +    virConnectPtr conn;
> +    virDomainObjPtr vm;
> +    struct qemud_driver *driver;
>  };

  took me a while to spot that gemu turned in qemu


  Again not a cgroup expert but this looks okay to me, ACK

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/