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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list