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

[Cole Robinson]

On 06/21/2011 04:49 PM, Eric Blake wrote:
> 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?
> 

Documentation purposes. Like you say, LXC can't even be run
non-privileged. However for the next version of the patch I just chose
to track the 'privileged' value in lxc_driver_t, similar to how is done
with qemu, even though it's not useful yet for LXC. That way we can pass
it around like usual, and if we ever support lxc:///session less sites
will need to be changed. v2 coming.

Thanks,
Cole

>> +
>> +    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.
>