Re: [libvirt] [PATCH 2/2] lxc: Fail connection attempt if cgroups not mounted
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.