[libvirt] [PATCH 2/2] lxc: Fail connection attempt if cgroups not mounted

[Eric Blake]

On 06/21/2011 10:47 AM, Cole Robinson wrote:
> Currently a user can connect to lxc:/// if cgroups aren't mounted, but
> they can't do a whole lot: starting and even stopping guests doesn't work
> (the latter only if cgroups were unmounted behind libvirts back). To make

s/libvirts/libvirt's/

> matters worse, even after mounting cgroups, libvirt must be restarted
> to actually notice.
> 
> This is frustrating for users who may make it all the way to the end of
> the virt-manager 'New VM' wizard only to receive an error that requires
> a libvirtd restart.
> 
> Fix this by checking for cgroup mounts at lxc:/// connect time, and
> if none are found, fail the connection.
> 
> I'm not sure if there are any negative consequences to putting this
> logic in lxcOpen...

I'm not thinking of any; at any rate, this seems like a reasonable
change.  But there are some issues that probably require a v2:

> +++ b/src/lxc/lxc_driver.c
> @@ -107,6 +107,22 @@ static void lxcDomainEventFlush(int timer, void *opaque);
>  static void lxcDomainEventQueue(lxc_driver_t *driver,
>                                  virDomainEventPtr event);
>  
> +static int lxcGetCgroup(lxc_driver_t *driver)
> +{
> +    int privileged = 1;

Why create this variable, if it never changes from the constant of 1?
Is it even possible to have a non-privileged lxc driver instance, and if
so, shouldn't we be getting this value from driver?

> +
> +    if (driver->cgroup)
> +        return 0;
> +
> +    int rc = virCgroupForDriver("lxc", &driver->cgroup, privileged, 1);
> +    if (rc < 0) {
> +        char buf[1024];
> +        VIR_DEBUG("Unable to create cgroup for LXC driver: %s",
> +                  virStrerror(-rc, buf, sizeof(buf)));

Unrelated to your patch, but I can't help but wonder if we should change
virStrerror into taking one argument and always returning a thread-local
string, rather than making callers pass a stack-allocated buffer.

> @@ -2113,11 +2136,7 @@ static int lxcStartup(int privileged)
>      lxc_driver->log_libvirtd = 0; /* by default log to container logfile */
>      lxc_driver->have_netns = lxcCheckNetNsSupport();
>  
> -    rc = virCgroupForDriver("lxc", &lxc_driver->cgroup, privileged, 1);
> -    if (rc < 0) {
> -        char buf[1024];
> -        VIR_DEBUG("Unable to create cgroup for LXC driver: %s",
> -                  virStrerror(-rc, buf, sizeof(buf)));
> +    if (lxcGetCgroup(lxc_driver) < 0) {

Hmm, this makes it look like lxcGetCgroup should take a second
parameter, privileged.

-- 
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/20110621/35aef7c3/attachment-0001.sig>