[libvirt] [PATCH 00/15] Major refactor of LXC controller code

The LXC controller code (libvirt_lxc binary) has grown rather large and complicated over time. One of the particularly bad problems is that there is no single store of global state. The results is that methods are passing around huge lists of parameters and it becomes unclear who is responsible for cleaning up state on failure. Although they are harmless in this context, there are many FDs which are leaked in the normal shutdown path. This series refactors everything so that a virLXCControllerPtr object holds all global state. The final patch also switches over to using the virNetServer APIs for the client/server monitor socket handling. There should be no functional change in this series, but it prepares the way for the next fun bit which is to introduce a full RPC protocol between libvirtd & libvirt_lxc. This will enable us to provide a bunch of interesting new APIs that are desired for LXC. Don't be put off by the large patch series - each change is fairly small & self-contained, with primarily just renaming of variables and funtions daemon/libvirtd.c | 3 daemon/remote.c | 3 daemon/remote.h | 3 src/Makefile.am | 41 - src/libvirt_private.syms | 1 src/lxc/lxc_container.c | 10 src/lxc/lxc_container.h | 2 src/lxc/lxc_controller.c | 1250 +++++++++++++++++++++-------------------------- src/rpc/virnetserver.c | 120 ++-- src/rpc/virnetserver.h | 6 src/util/virfile.c | 138 +++++ src/util/virfile.h | 3 12 files changed, 819 insertions(+), 761 deletions(-)

From: "Daniel P. Berrange" <berrange@redhat.com> The callback that is invoked when a new RPC client is initialized does not have any opaque parameter. Add one so that custom data can be passed into the callback Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd.c | 3 ++- daemon/remote.c | 3 ++- daemon/remote.h | 3 ++- src/rpc/virnetserver.c | 7 +++++-- src/rpc/virnetserver.h | 6 ++++-- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9c06344..8c434a0 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1176,7 +1176,8 @@ int main(int argc, char **argv) { config->keepalive_count, !!config->keepalive_required, config->mdns_adv ? config->mdns_name : NULL, - remoteClientInitHook))) { + remoteClientInitHook, + NULL))) { ret = VIR_DAEMON_ERR_INIT; goto cleanup; } diff --git a/daemon/remote.c b/daemon/remote.c index b8c2aab..095d854 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -640,7 +640,8 @@ static void remoteClientCloseFunc(virNetServerClientPtr client) int remoteClientInitHook(virNetServerPtr srv ATTRIBUTE_UNUSED, - virNetServerClientPtr client) + virNetServerClientPtr client, + void *opaque ATTRIBUTE_UNUSED) { struct daemonClientPrivate *priv; int i; diff --git a/daemon/remote.h b/daemon/remote.h index 5444e47..d3e1b2d 100644 --- a/daemon/remote.h +++ b/daemon/remote.h @@ -36,6 +36,7 @@ extern virNetServerProgramProc qemuProcs[]; extern size_t qemuNProcs; int remoteClientInitHook(virNetServerPtr srv, - virNetServerClientPtr client); + virNetServerClientPtr client, + void *opaque); #endif /* __LIBVIRTD_REMOTE_H__ */ diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 247ddd7..358666d 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -112,6 +112,7 @@ struct _virNetServer { void *autoShutdownOpaque; virNetServerClientInitHook clientInitHook; + void *clientInitOpaque; }; @@ -248,7 +249,7 @@ static int virNetServerDispatchNewClient(virNetServerServicePtr svc ATTRIBUTE_UN goto error; if (srv->clientInitHook && - srv->clientInitHook(srv, client) < 0) + srv->clientInitHook(srv, client, srv->clientInitOpaque) < 0) goto error; if (VIR_EXPAND_N(srv->clients, srv->nclients, 1) < 0) { @@ -310,7 +311,8 @@ virNetServerPtr virNetServerNew(size_t min_workers, unsigned int keepaliveCount, bool keepaliveRequired, const char *mdnsGroupName, - virNetServerClientInitHook clientInitHook) + virNetServerClientInitHook clientInitHook, + void *opaque) { virNetServerPtr srv; struct sigaction sig_action; @@ -334,6 +336,7 @@ virNetServerPtr virNetServerNew(size_t min_workers, srv->keepaliveRequired = keepaliveRequired; srv->sigwrite = srv->sigread = -1; srv->clientInitHook = clientInitHook; + srv->clientInitOpaque = opaque; srv->privileged = geteuid() == 0 ? true : false; if (mdnsGroupName && diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 0ebe00e..438f524 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -32,7 +32,8 @@ # include "virnetserverservice.h" typedef int (*virNetServerClientInitHook)(virNetServerPtr srv, - virNetServerClientPtr client); + virNetServerClientPtr client, + void *opaque); virNetServerPtr virNetServerNew(size_t min_workers, size_t max_workers, @@ -42,7 +43,8 @@ virNetServerPtr virNetServerNew(size_t min_workers, unsigned int keepaliveCount, bool keepaliveRequired, const char *mdnsGroupName, - virNetServerClientInitHook clientInitHook); + virNetServerClientInitHook clientInitHook, + void *opaque); typedef int (*virNetServerAutoShutdownFunc)(virNetServerPtr srv, void *opaque); -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:40 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The callback that is invoked when a new RPC client is initialized does not have any opaque parameter. Add one so that custom data can be passed into the callback
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd.c | 3 ++- daemon/remote.c | 3 ++- daemon/remote.h | 3 ++- src/rpc/virnetserver.c | 7 +++++-- src/rpc/virnetserver.h | 6 ++++-- 5 files changed, 15 insertions(+), 7 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Refactor the RPC server dispatcher code so that if 'max_workers==0' the entire server will run single threaded. This is useful for use cases where there will only ever be 1 client connected which serializes its requests Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/rpc/virnetserver.c | 113 +++++++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 45 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 358666d..4a02aab 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -127,49 +127,64 @@ static void virNetServerUnlock(virNetServerPtr srv) } -static void virNetServerHandleJob(void *jobOpaque, void *opaque) +static int virNetServerProcessMsg(virNetServerPtr srv, + virNetServerClientPtr client, + virNetServerProgramPtr prog, + virNetMessagePtr msg) { - virNetServerPtr srv = opaque; - virNetServerJobPtr job = jobOpaque; - - VIR_DEBUG("server=%p client=%p message=%p prog=%p", - srv, job->client, job->msg, job->prog); - - if (!job->prog) { + int ret = -1; + if (!prog) { /* Only send back an error for type == CALL. Other * message types are not expecting replies, so we * must just log it & drop them */ - if (job->msg->header.type == VIR_NET_CALL || - job->msg->header.type == VIR_NET_CALL_WITH_FDS) { - if (virNetServerProgramUnknownError(job->client, - job->msg, - &job->msg->header) < 0) - goto error; + if (msg->header.type == VIR_NET_CALL || + msg->header.type == VIR_NET_CALL_WITH_FDS) { + if (virNetServerProgramUnknownError(client, + msg, + &msg->header) < 0) + goto cleanup; } else { VIR_INFO("Dropping client mesage, unknown program %d version %d type %d proc %d", - job->msg->header.prog, job->msg->header.vers, - job->msg->header.type, job->msg->header.proc); + msg->header.prog, msg->header.vers, + msg->header.type, msg->header.proc); /* Send a dummy reply to free up 'msg' & unblock client rx */ - virNetMessageClear(job->msg); - job->msg->header.type = VIR_NET_REPLY; - if (virNetServerClientSendMessage(job->client, job->msg) < 0) - goto error; + virNetMessageClear(msg); + msg->header.type = VIR_NET_REPLY; + if (virNetServerClientSendMessage(client, msg) < 0) + goto cleanup; } - goto cleanup; + goto done; } - if (virNetServerProgramDispatch(job->prog, + if (virNetServerProgramDispatch(prog, srv, - job->client, - job->msg) < 0) + client, + msg) < 0) + goto cleanup; + +done: + ret = 0; + +cleanup: + return ret; +} + +static void virNetServerHandleJob(void *jobOpaque, void *opaque) +{ + virNetServerPtr srv = opaque; + virNetServerJobPtr job = jobOpaque; + + VIR_DEBUG("server=%p client=%p message=%p prog=%p", + srv, job->client, job->msg, job->prog); + + if (virNetServerProcessMsg(srv, job->client, job->prog, job->msg) < 0) goto error; virNetServerLock(srv); virNetServerProgramFree(job->prog); virNetServerUnlock(srv); -cleanup: virNetServerClientFree(job->client); VIR_FREE(job); return; @@ -187,7 +202,6 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client, void *opaque) { virNetServerPtr srv = opaque; - virNetServerJobPtr job; virNetServerProgramPtr prog = NULL; unsigned int priority = 0; size_t i; @@ -196,34 +210,42 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client, VIR_DEBUG("server=%p client=%p message=%p", srv, client, msg); - if (VIR_ALLOC(job) < 0) { - virReportOOMError(); - return -1; - } - - job->client = client; - job->msg = msg; - virNetServerLock(srv); for (i = 0 ; i < srv->nprograms ; i++) { - if (virNetServerProgramMatches(srv->programs[i], job->msg)) { + if (virNetServerProgramMatches(srv->programs[i], msg)) { prog = srv->programs[i]; break; } } - if (prog) { - virNetServerProgramRef(prog); - job->prog = prog; - priority = virNetServerProgramGetPriority(prog, msg->header.proc); - } + if (srv->workers) { + virNetServerJobPtr job; + + if (VIR_ALLOC(job) < 0) { + virReportOOMError(); + goto cleanup; + } - ret = virThreadPoolSendJob(srv->workers, priority, job); + job->client = client; + job->msg = msg; + + if (prog) { + virNetServerProgramRef(prog); + job->prog = prog; + priority = virNetServerProgramGetPriority(prog, msg->header.proc); + } - if (ret < 0) { - VIR_FREE(job); - virNetServerProgramFree(prog); + ret = virThreadPoolSendJob(srv->workers, priority, job); + + if (ret < 0) { + VIR_FREE(job); + virNetServerProgramFree(prog); + } + } else { + ret = virNetServerProcessMsg(srv, client, prog, msg); } + +cleanup: virNetServerUnlock(srv); return ret; @@ -324,7 +346,8 @@ virNetServerPtr virNetServerNew(size_t min_workers, srv->refs = 1; - if (!(srv->workers = virThreadPoolNew(min_workers, max_workers, + if (max_workers && + !(srv->workers = virThreadPoolNew(min_workers, max_workers, priority_workers, virNetServerHandleJob, srv))) -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:41 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Refactor the RPC server dispatcher code so that if 'max_workers==0' the entire server will run single threaded. This is useful for use cases where there will only ever be 1 client connected which serializes its requests
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/rpc/virnetserver.c | 113 +++++++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 45 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Currently the build of libvirt_lxc will cause recompilation of all sources under src/util, src/conf, src/security and more. Switch the libvirt_lxc process to link against the libtool convenience libraries that are already built as part of the main libvirt.os & libvirtd build process Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 39 ++++++++------------------------------- 1 file changed, 8 insertions(+), 31 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 2309984..eeeda1c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -598,7 +598,7 @@ libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ $(DBUS_CFLAGS) libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ - $(RT_LIBS) $(DBUS_LIBS) $(MSCOM_LIBS) + $(RT_LIBS) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) noinst_LTLIBRARIES += libvirt_conf.la @@ -783,14 +783,12 @@ endif libvirt_driver_qemu_impl_la_CFLAGS = $(NUMACTL_CFLAGS) \ $(GNUTLS_CFLAGS) \ - $(LIBXML_CFLAGS) \ $(LIBNL_CFLAGS) \ -I$(top_srcdir)/src/conf $(AM_CFLAGS) libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_qemu_impl_la_LIBADD = $(NUMACTL_LIBS) \ $(CAPNG_LIBS) \ $(GNUTLS_LIBS) \ - $(LIBXML_LIBS) \ $(LIBNL_LIBS) libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES) @@ -1290,7 +1288,7 @@ libvirt_la_LDFLAGS = $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_SYMBOL_FILE) \ $(LIBVIRT_NODELETE) $(AM_LDFLAGS) \ $(CYGWIN_EXTRA_LDFLAGS) $(MINGW_EXTRA_LDFLAGS) libvirt_la_BUILT_LIBADD += ../gnulib/lib/libgnu.la -libvirt_la_LIBADD += $(LIBXML_LIBS) \ +libvirt_la_LIBADD += \ $(DRIVER_MODULE_LIBS) \ $(CYGWIN_EXTRA_LIBADD) libvirt_la_CFLAGS = -DIN_LIBVIRT $(AM_CFLAGS) @@ -1521,26 +1519,13 @@ libexec_PROGRAMS += libvirt_lxc libvirt_lxc_SOURCES = \ $(LXC_CONTROLLER_SOURCES) \ - $(UTIL_SOURCES) \ - $(NODE_INFO_SOURCES) \ - $(ENCRYPTION_CONF_SOURCES) \ - $(NETDEV_CONF_SOURCES) \ - $(DOMAIN_CONF_SOURCES) \ - $(SECRET_CONF_SOURCES) \ - $(CPU_CONF_SOURCES) \ - $(SECURITY_DRIVER_SOURCES) \ - $(NWFILTER_PARAM_CONF_SOURCES) -if WITH_SECDRIVER_SELINUX -libvirt_lxc_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) -endif -if WITH_SECDRIVER_APPARMOR -libvirt_lxc_SOURCES += $(SECURITY_DRIVER_APPARMOR_SOURCES) -endif + $(NODE_INFO_SOURCES) libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(AM_LDFLAGS) -libvirt_lxc_LDADD = $(CAPNG_LIBS) $(YAJL_LIBS) \ - $(LIBXML_LIBS) $(NUMACTL_LIBS) $(THREAD_LIBS) \ - $(LIBNL_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ - $(RT_LIBS) $(DBUS_LIBS) \ +libvirt_lxc_LDADD = \ + $(NUMACTL_LIBS) \ + libvirt_driver_security.la \ + libvirt_conf.la \ + libvirt_util.la \ ../gnulib/lib/libgnu.la if WITH_DTRACE_PROBES libvirt_lxc_LDADD += libvirt_probes.lo @@ -1552,13 +1537,6 @@ if WITH_SECDRIVER_APPARMOR libvirt_lxc_LDADD += $(APPARMOR_LIBS) endif libvirt_lxc_CFLAGS = \ - $(LIBPARTED_CFLAGS) \ - $(NUMACTL_CFLAGS) \ - $(CAPNG_CFLAGS) \ - $(YAJL_CFLAGS) \ - $(AUDIT_CFLAGS) \ - $(DBUS_CFLAGS) \ - $(LIBNL_CFLAGS) \ -I$(top_srcdir)/src/conf \ $(AM_CFLAGS) if HAVE_LIBBLKID @@ -1583,7 +1561,6 @@ virt_aa_helper_SOURCES = $(SECURITY_DRIVER_APPARMOR_HELPER_SOURCES) virt_aa_helper_LDFLAGS = $(WARN_LDFLAGS) $(AM_LDFLAGS) virt_aa_helper_LDADD = \ - $(LIBXML_LIBS) \ libvirt_conf.la \ libvirt_util.la \ ../gnulib/lib/libgnu.la -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:42 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the build of libvirt_lxc will cause recompilation of all sources under src/util, src/conf, src/security and more. Switch the libvirt_lxc process to link against the libtool convenience libraries that are already built as part of the main libvirt.os & libvirtd build process
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 39 ++++++++------------------------------- 1 file changed, 8 insertions(+), 31 deletions(-)
It also moves LIBXML_LIBS down to libvirt_util, which is a good thing too. ACK Jirka

On 07/03/2012 09:58 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the build of libvirt_lxc will cause recompilation of all sources under src/util, src/conf, src/security and more. Switch the libvirt_lxc process to link against the libtool convenience libraries that are already built as part of the main libvirt.os & libvirtd build process
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 39 ++++++++------------------------------- 1 file changed, 8 insertions(+), 31 deletions(-)
Does this mean we can clean up the $(DOMAIN_LIST_SOURCES) hack from commit 2c68080, where we had a link failure on libvirt_lxc when trying to pull in src/conf/virdomainlist.c? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Wed, Jul 04, 2012 at 06:39:46AM -0600, Eric Blake wrote:
On 07/03/2012 09:58 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the build of libvirt_lxc will cause recompilation of all sources under src/util, src/conf, src/security and more. Switch the libvirt_lxc process to link against the libtool convenience libraries that are already built as part of the main libvirt.os & libvirtd build process
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 39 ++++++++------------------------------- 1 file changed, 8 insertions(+), 31 deletions(-)
Does this mean we can clean up the $(DOMAIN_LIST_SOURCES) hack from commit 2c68080, where we had a link failure on libvirt_lxc when trying to pull in src/conf/virdomainlist.c?
I don't recall what the problem was, but it looks like that bit of the makefile could be cleaned up. I'd expect anything that's in the conf/ directory to be listed under the same _SOURCES variable. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/04/12 17:00, Daniel P. Berrange wrote:
On Wed, Jul 04, 2012 at 06:39:46AM -0600, Eric Blake wrote:
On 07/03/2012 09:58 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Currently the build of libvirt_lxc will cause recompilation of all sources under src/util, src/conf, src/security and more. Switch the libvirt_lxc process to link against the libtool convenience libraries that are already built as part of the main libvirt.os & libvirtd build process
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 39 ++++++++------------------------------- 1 file changed, 8 insertions(+), 31 deletions(-)
Does this mean we can clean up the $(DOMAIN_LIST_SOURCES) hack from commit 2c68080, where we had a link failure on libvirt_lxc when trying to pull in src/conf/virdomainlist.c?
I don't recall what the problem was, but it looks like that bit of the makefile could be cleaned up. I'd expect anything that's in the conf/ directory to be listed under the same _SOURCES variable.
The problem is that when libvirt_lxc binary is built it includes and links helpers from src/conf/ but does not link everything required for all the helpers. Some of the helpers (notably: virconsole and virdomainlist) require some other functions to be linked although they are not used in libvirt_lxc resulting in a linking failure. I've worked this around with a separate _SOURCES variable in the makefile that is not included into libvirt_lxc.
Daniel

From: "Daniel P. Berrange" <berrange@redhat.com> The LXC controller code is having to pass around an ever increasing number of parameters between methods. To make the code more managable introduce a virLXCControllerPtr to hold all this state, starting with the container name and virDomainDefPtr object Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 133 +++++++++++++++++++++++++++++++--------------- 1 file changed, 91 insertions(+), 42 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index b0e9f46..7447f6c 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -80,6 +80,64 @@ struct cgroup_device_policy { }; +typedef struct _virLXCController virLXCController; +typedef virLXCController *virLXCControllerPtr; +struct _virLXCController { + char *name; + virDomainDefPtr def; +}; + +static void virLXCControllerFree(virLXCControllerPtr ctrl); + +static virLXCControllerPtr virLXCControllerNew(const char *name) +{ + virLXCControllerPtr ctrl = NULL; + virCapsPtr caps = NULL; + char *configFile = NULL; + + if (VIR_ALLOC(ctrl) < 0) + goto no_memory; + + if (!(ctrl->name = strdup(name))) + goto no_memory; + + if ((caps = lxcCapsInit(NULL)) == NULL) + goto cleanup; + + if ((configFile = virDomainConfigFile(LXC_STATE_DIR, + ctrl->name)) == NULL) + goto cleanup; + + if ((ctrl->def = virDomainDefParseFile(caps, + configFile, + 1 << VIR_DOMAIN_VIRT_LXC, + 0)) == NULL) + goto cleanup; + +cleanup: + VIR_FREE(configFile); + virCapabilitiesFree(caps); + return ctrl; + +no_memory: + virReportOOMError(); +//error: + virLXCControllerFree(ctrl); + ctrl = NULL; + goto cleanup; +} + +static void virLXCControllerFree(virLXCControllerPtr ctrl) +{ + if (!ctrl) + return; + + virDomainDefFree(ctrl->def); + VIR_FREE(ctrl->name); + + VIR_FREE(ctrl); +} + static int lxcGetLoopFD(char **dev_name) { int fd = -1; @@ -625,12 +683,12 @@ cleanup: return rc; } -static char*lxcMonitorPath(virDomainDefPtr def) +static char*lxcMonitorPath(virLXCControllerPtr ctrl) { char *sockpath; if (virAsprintf(&sockpath, "%s/%s.sock", - LXC_STATE_DIR, def->name) < 0) + LXC_STATE_DIR, ctrl->def->name) < 0) virReportOOMError(); return sockpath; } @@ -1362,15 +1420,15 @@ cleanup: } static int -lxcControllerRun(virDomainDefPtr def, - virSecurityManagerPtr securityDriver, - unsigned int nveths, - char **veths, - int monitor, - int client, - int *ttyFDs, - size_t nttyFDs, - int handshakefd) +virLXCControllerRun(virLXCControllerPtr ctrl, + virSecurityManagerPtr securityDriver, + unsigned int nveths, + char **veths, + int monitor, + int client, + int *ttyFDs, + size_t nttyFDs, + int handshakefd) { int rc = -1; int control[2] = { -1, -1}; @@ -1407,12 +1465,12 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } - if (lxcSetupLoopDevices(def, &nloopDevs, &loopDevs) < 0) + if (lxcSetupLoopDevices(ctrl->def, &nloopDevs, &loopDevs) < 0) goto cleanup; - root = virDomainGetRootFilesystem(def); + root = virDomainGetRootFilesystem(ctrl->def); - if (lxcSetContainerResources(def) < 0) + if (lxcSetContainerResources(ctrl->def) < 0) goto cleanup; /* @@ -1436,7 +1494,7 @@ lxcControllerRun(virDomainDefPtr def, * marked as shared */ if (root) { - mount_options = virSecurityManagerGetMountOptions(securityDriver, def); + mount_options = virSecurityManagerGetMountOptions(securityDriver, ctrl->def); char *opts; VIR_DEBUG("Setting up private /dev/pts"); @@ -1525,10 +1583,10 @@ lxcControllerRun(virDomainDefPtr def, } } - if (lxcSetPersonality(def) < 0) + if (lxcSetPersonality(ctrl->def) < 0) goto cleanup; - if ((container = lxcContainerStart(def, + if ((container = lxcContainerStart(ctrl->def, securityDriver, nveths, veths, @@ -1618,6 +1676,8 @@ cleanup: } + + int main(int argc, char *argv[]) { pid_t pid; @@ -1629,9 +1689,6 @@ int main(int argc, char *argv[]) int monitor = -1; int handshakefd = -1; int bg = 0; - virCapsPtr caps = NULL; - virDomainDefPtr def = NULL; - char *configFile = NULL; char *sockpath = NULL; const struct option options[] = { { "background", 0, NULL, 'b' }, @@ -1646,6 +1703,7 @@ int main(int argc, char *argv[]) int *ttyFDs = NULL; size_t nttyFDs = 0; virSecurityManagerPtr securityDriver = NULL; + virLXCControllerPtr ctrl = NULL; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || @@ -1766,31 +1824,22 @@ int main(int argc, char *argv[]) virEventRegisterDefaultImpl(); - if ((caps = lxcCapsInit(NULL)) == NULL) - goto cleanup; - - if ((configFile = virDomainConfigFile(LXC_STATE_DIR, - name)) == NULL) - goto cleanup; - - if ((def = virDomainDefParseFile(caps, configFile, - 1 << VIR_DOMAIN_VIRT_LXC, - 0)) == NULL) + if (!(ctrl = virLXCControllerNew(name))) goto cleanup; VIR_DEBUG("Security model %s type %s label %s imagelabel %s", - NULLSTR(def->seclabel.model), - virDomainSeclabelTypeToString(def->seclabel.type), - NULLSTR(def->seclabel.label), - NULLSTR(def->seclabel.imagelabel)); + NULLSTR(ctrl->def->seclabel.model), + virDomainSeclabelTypeToString(ctrl->def->seclabel.type), + NULLSTR(ctrl->def->seclabel.label), + NULLSTR(ctrl->def->seclabel.imagelabel)); - if (def->nnets != nveths) { + if (ctrl->def->nnets != nveths) { fprintf(stderr, "%s: expecting %d veths, but got %d\n", - argv[0], def->nnets, nveths); + argv[0], ctrl->def->nnets, nveths); goto cleanup; } - if ((sockpath = lxcMonitorPath(def)) == NULL) + if ((sockpath = lxcMonitorPath(ctrl)) == NULL) goto cleanup; if ((monitor = lxcMonitorServer(sockpath)) < 0) @@ -1835,17 +1884,17 @@ int main(int argc, char *argv[]) goto cleanup; } - rc = lxcControllerRun(def, securityDriver, - nveths, veths, monitor, client, - ttyFDs, nttyFDs, handshakefd); + rc = virLXCControllerRun(ctrl, securityDriver, + nveths, veths, monitor, client, + ttyFDs, nttyFDs, handshakefd); cleanup: - if (def) - virPidFileDelete(LXC_STATE_DIR, def->name); + virPidFileDelete(LXC_STATE_DIR, name); lxcControllerCleanupInterfaces(nveths, veths); if (sockpath) unlink(sockpath); VIR_FREE(sockpath); + virLXCControllerFree(ctrl); return rc ? EXIT_FAILURE : EXIT_SUCCESS; } -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:43 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The LXC controller code is having to pass around an ever increasing number of parameters between methods. To make the code more managable introduce a virLXCControllerPtr to hold all this state, starting with the container name and virDomainDefPtr object
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 133 +++++++++++++++++++++++++++++++--------------- 1 file changed, 91 insertions(+), 42 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index b0e9f46..7447f6c 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -80,6 +80,64 @@ struct cgroup_device_policy { };
+typedef struct _virLXCController virLXCController; +typedef virLXCController *virLXCControllerPtr; +struct _virLXCController { + char *name; + virDomainDefPtr def; +}; + +static void virLXCControllerFree(virLXCControllerPtr ctrl); + +static virLXCControllerPtr virLXCControllerNew(const char *name) +{ + virLXCControllerPtr ctrl = NULL; + virCapsPtr caps = NULL; + char *configFile = NULL; + + if (VIR_ALLOC(ctrl) < 0) + goto no_memory; + + if (!(ctrl->name = strdup(name))) + goto no_memory; + + if ((caps = lxcCapsInit(NULL)) == NULL) + goto cleanup; + + if ((configFile = virDomainConfigFile(LXC_STATE_DIR, + ctrl->name)) == NULL) + goto cleanup; + + if ((ctrl->def = virDomainDefParseFile(caps, + configFile, + 1 << VIR_DOMAIN_VIRT_LXC, + 0)) == NULL) + goto cleanup;
I think all three "goto cleanup"s should rather be goto error...
+ +cleanup: + VIR_FREE(configFile); + virCapabilitiesFree(caps); + return ctrl; + +no_memory: + virReportOOMError(); +//error:
and this label should not be commented out.
+ virLXCControllerFree(ctrl); + ctrl = NULL; + goto cleanup; +} + +static void virLXCControllerFree(virLXCControllerPtr ctrl) +{ + if (!ctrl) + return; + + virDomainDefFree(ctrl->def); + VIR_FREE(ctrl->name); + + VIR_FREE(ctrl); +} + static int lxcGetLoopFD(char **dev_name) { int fd = -1; @@ -625,12 +683,12 @@ cleanup: return rc; }
-static char*lxcMonitorPath(virDomainDefPtr def) +static char*lxcMonitorPath(virLXCControllerPtr ctrl) { char *sockpath;
if (virAsprintf(&sockpath, "%s/%s.sock", - LXC_STATE_DIR, def->name) < 0) + LXC_STATE_DIR, ctrl->def->name) < 0) virReportOOMError(); return sockpath; } @@ -1362,15 +1420,15 @@ cleanup: }
static int -lxcControllerRun(virDomainDefPtr def, - virSecurityManagerPtr securityDriver, - unsigned int nveths, - char **veths, - int monitor, - int client, - int *ttyFDs, - size_t nttyFDs, - int handshakefd) +virLXCControllerRun(virLXCControllerPtr ctrl, + virSecurityManagerPtr securityDriver, + unsigned int nveths, + char **veths, + int monitor, + int client, + int *ttyFDs, + size_t nttyFDs, + int handshakefd) { int rc = -1; int control[2] = { -1, -1}; @@ -1407,12 +1465,12 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; }
- if (lxcSetupLoopDevices(def, &nloopDevs, &loopDevs) < 0) + if (lxcSetupLoopDevices(ctrl->def, &nloopDevs, &loopDevs) < 0) goto cleanup;
- root = virDomainGetRootFilesystem(def); + root = virDomainGetRootFilesystem(ctrl->def);
- if (lxcSetContainerResources(def) < 0) + if (lxcSetContainerResources(ctrl->def) < 0) goto cleanup;
/* @@ -1436,7 +1494,7 @@ lxcControllerRun(virDomainDefPtr def, * marked as shared */ if (root) { - mount_options = virSecurityManagerGetMountOptions(securityDriver, def); + mount_options = virSecurityManagerGetMountOptions(securityDriver, ctrl->def); char *opts; VIR_DEBUG("Setting up private /dev/pts");
@@ -1525,10 +1583,10 @@ lxcControllerRun(virDomainDefPtr def, } }
- if (lxcSetPersonality(def) < 0) + if (lxcSetPersonality(ctrl->def) < 0) goto cleanup;
- if ((container = lxcContainerStart(def, + if ((container = lxcContainerStart(ctrl->def, securityDriver, nveths, veths, @@ -1618,6 +1676,8 @@ cleanup: }
+ +
Looks like a bogus addition of two more empty lines.
int main(int argc, char *argv[]) { pid_t pid; @@ -1629,9 +1689,6 @@ int main(int argc, char *argv[]) int monitor = -1; int handshakefd = -1; int bg = 0; - virCapsPtr caps = NULL; - virDomainDefPtr def = NULL; - char *configFile = NULL; char *sockpath = NULL; const struct option options[] = { { "background", 0, NULL, 'b' }, @@ -1646,6 +1703,7 @@ int main(int argc, char *argv[]) int *ttyFDs = NULL; size_t nttyFDs = 0; virSecurityManagerPtr securityDriver = NULL; + virLXCControllerPtr ctrl = NULL;
if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || @@ -1766,31 +1824,22 @@ int main(int argc, char *argv[])
virEventRegisterDefaultImpl();
- if ((caps = lxcCapsInit(NULL)) == NULL) - goto cleanup; - - if ((configFile = virDomainConfigFile(LXC_STATE_DIR, - name)) == NULL) - goto cleanup; - - if ((def = virDomainDefParseFile(caps, configFile, - 1 << VIR_DOMAIN_VIRT_LXC, - 0)) == NULL) + if (!(ctrl = virLXCControllerNew(name))) goto cleanup;
Oh I see, the wrong labels were caused by cut&pasting the code from here.
VIR_DEBUG("Security model %s type %s label %s imagelabel %s", - NULLSTR(def->seclabel.model), - virDomainSeclabelTypeToString(def->seclabel.type), - NULLSTR(def->seclabel.label), - NULLSTR(def->seclabel.imagelabel)); + NULLSTR(ctrl->def->seclabel.model), + virDomainSeclabelTypeToString(ctrl->def->seclabel.type), + NULLSTR(ctrl->def->seclabel.label), + NULLSTR(ctrl->def->seclabel.imagelabel));
- if (def->nnets != nveths) { + if (ctrl->def->nnets != nveths) { fprintf(stderr, "%s: expecting %d veths, but got %d\n", - argv[0], def->nnets, nveths); + argv[0], ctrl->def->nnets, nveths); goto cleanup; }
- if ((sockpath = lxcMonitorPath(def)) == NULL) + if ((sockpath = lxcMonitorPath(ctrl)) == NULL) goto cleanup;
if ((monitor = lxcMonitorServer(sockpath)) < 0) @@ -1835,17 +1884,17 @@ int main(int argc, char *argv[]) goto cleanup; }
- rc = lxcControllerRun(def, securityDriver, - nveths, veths, monitor, client, - ttyFDs, nttyFDs, handshakefd); + rc = virLXCControllerRun(ctrl, securityDriver, + nveths, veths, monitor, client, + ttyFDs, nttyFDs, handshakefd);
cleanup: - if (def) - virPidFileDelete(LXC_STATE_DIR, def->name); + virPidFileDelete(LXC_STATE_DIR, name); lxcControllerCleanupInterfaces(nveths, veths); if (sockpath) unlink(sockpath); VIR_FREE(sockpath); + virLXCControllerFree(ctrl);
return rc ? EXIT_FAILURE : EXIT_SUCCESS; }
ACK with the labels fixed. Jirka

On Wed, Jul 04, 2012 at 12:20:44PM +0200, Jiri Denemark wrote:
On Tue, Jul 03, 2012 at 16:58:43 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The LXC controller code is having to pass around an ever increasing number of parameters between methods. To make the code more managable introduce a virLXCControllerPtr to hold all this state, starting with the container name and virDomainDefPtr object
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 133 +++++++++++++++++++++++++++++++--------------- 1 file changed, 91 insertions(+), 42 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index b0e9f46..7447f6c 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -80,6 +80,64 @@ struct cgroup_device_policy { };
+typedef struct _virLXCController virLXCController; +typedef virLXCController *virLXCControllerPtr; +struct _virLXCController { + char *name; + virDomainDefPtr def; +}; + +static void virLXCControllerFree(virLXCControllerPtr ctrl); + +static virLXCControllerPtr virLXCControllerNew(const char *name) +{ + virLXCControllerPtr ctrl = NULL; + virCapsPtr caps = NULL; + char *configFile = NULL; + + if (VIR_ALLOC(ctrl) < 0) + goto no_memory; + + if (!(ctrl->name = strdup(name))) + goto no_memory; + + if ((caps = lxcCapsInit(NULL)) == NULL) + goto cleanup; + + if ((configFile = virDomainConfigFile(LXC_STATE_DIR, + ctrl->name)) == NULL) + goto cleanup; + + if ((ctrl->def = virDomainDefParseFile(caps, + configFile, + 1 << VIR_DOMAIN_VIRT_LXC, + 0)) == NULL) + goto cleanup;
I think all three "goto cleanup"s should rather be goto error...
Doh, of course they should. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: "Daniel P. Berrange" <berrange@redhat.com> Move the veth device name state into the virLXCControllerPtr object and stop passing it around. Also use size_t instead of unsigned int for the array length parameters. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 10 +++--- src/lxc/lxc_container.h | 2 +- src/lxc/lxc_controller.c | 79 ++++++++++++++++++++++++++++++---------------- 3 files changed, 57 insertions(+), 34 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 910e82b..4f8703c 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -93,7 +93,7 @@ typedef struct __lxc_child_argv lxc_child_argv_t; struct __lxc_child_argv { virDomainDefPtr config; virSecurityManagerPtr securityDriver; - unsigned int nveths; + size_t nveths; char **veths; int monitor; char **ttyPaths; @@ -262,15 +262,15 @@ int lxcContainerWaitForContinue(int control) * Returns 0 on success or nonzero in case of error */ static int lxcContainerRenameAndEnableInterfaces(bool privNet, - unsigned int nveths, + size_t nveths, char **veths) { int rc = 0; - unsigned int i; + size_t i; char *newname = NULL; for (i = 0 ; i < nveths ; i++) { - if (virAsprintf(&newname, "eth%d", i) < 0) { + if (virAsprintf(&newname, "eth%zu", i) < 0) { virReportOOMError(); rc = -1; goto error_out; @@ -1775,7 +1775,7 @@ const char *lxcContainerGetAlt32bitArch(const char *arch) */ int lxcContainerStart(virDomainDefPtr def, virSecurityManagerPtr securityDriver, - unsigned int nveths, + size_t nveths, char **veths, int control, int handshakefd, diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h index 77fb9b2..6f1cc8b 100644 --- a/src/lxc/lxc_container.h +++ b/src/lxc/lxc_container.h @@ -51,7 +51,7 @@ int lxcContainerWaitForContinue(int control); int lxcContainerStart(virDomainDefPtr def, virSecurityManagerPtr securityDriver, - unsigned int nveths, + size_t nveths, char **veths, int control, int handshakefd, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7447f6c..ad11307 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -85,6 +85,9 @@ typedef virLXCController *virLXCControllerPtr; struct _virLXCController { char *name; virDomainDefPtr def; + + size_t nveths; + char **veths; }; static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -129,15 +132,35 @@ no_memory: static void virLXCControllerFree(virLXCControllerPtr ctrl) { + size_t i; + if (!ctrl) return; + for (i = 0 ; i < ctrl->nveths ; i++) + VIR_FREE(ctrl->veths[i]); + VIR_FREE(ctrl->veths); + virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name); VIR_FREE(ctrl); } + +static int virLXCControllerValidateNICs(virLXCControllerPtr ctrl) +{ + if (ctrl->def->nnets != ctrl->nveths) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("expecting %d veths, but got %zu"), + ctrl->def->nnets, ctrl->nveths); + return -1; + } + + return 0; +} + + static int lxcGetLoopFD(char **dev_name) { int fd = -1; @@ -1307,7 +1330,7 @@ cleanup2: /** - * lxcControllerMoveInterfaces + * virLXCControllerMoveInterfaces * @nveths: number of interfaces * @veths: interface names * @container: pid of container @@ -1316,13 +1339,13 @@ cleanup2: * * Returns 0 on success or -1 in case of error */ -static int lxcControllerMoveInterfaces(unsigned int nveths, - char **veths, - pid_t container) +static int virLXCControllerMoveInterfaces(virLXCControllerPtr ctrl, + pid_t container) { - unsigned int i; - for (i = 0 ; i < nveths ; i++) - if (virNetDevSetNamespace(veths[i], container) < 0) + size_t i; + + for (i = 0 ; i < ctrl->nveths ; i++) + if (virNetDevSetNamespace(ctrl->veths[i], container) < 0) return -1; return 0; @@ -1330,24 +1353,26 @@ static int lxcControllerMoveInterfaces(unsigned int nveths, /** - * lxcCleanupInterfaces: - * @nveths: number of interfaces - * @veths: interface names + * virLXCControllerDeleteInterfaces: + * @ctrl: the LXC controller * * Cleans up the container interfaces by deleting the veth device pairs. * * Returns 0 on success or -1 in case of error */ -static int lxcControllerCleanupInterfaces(unsigned int nveths, - char **veths) +static int virLXCControllerDeleteInterfaces(virLXCControllerPtr ctrl) { - unsigned int i; - for (i = 0 ; i < nveths ; i++) - ignore_value(virNetDevVethDelete(veths[i])); + size_t i; + int ret = 0; - return 0; + for (i = 0 ; i < ctrl->nveths ; i++) + if (virNetDevVethDelete(ctrl->veths[i]) < 0) + ret = -1; + + return ret; } + static int lxcSetPersonality(virDomainDefPtr def) { struct utsname utsname; @@ -1422,8 +1447,6 @@ cleanup: static int virLXCControllerRun(virLXCControllerPtr ctrl, virSecurityManagerPtr securityDriver, - unsigned int nveths, - char **veths, int monitor, int client, int *ttyFDs, @@ -1588,8 +1611,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl, if ((container = lxcContainerStart(ctrl->def, securityDriver, - nveths, - veths, + ctrl->nveths, + ctrl->veths, control[1], containerhandshake[1], containerTtyPaths, @@ -1598,7 +1621,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, VIR_FORCE_CLOSE(control[1]); VIR_FORCE_CLOSE(containerhandshake[1]); - if (lxcControllerMoveInterfaces(nveths, veths, container) < 0) + if (virLXCControllerMoveInterfaces(ctrl, container) < 0) goto cleanup; if (lxcContainerSendContinue(control[0]) < 0) { @@ -1684,7 +1707,7 @@ int main(int argc, char *argv[]) int rc = 1; int client; char *name = NULL; - int nveths = 0; + size_t nveths = 0; char **veths = NULL; int monitor = -1; int handshakefd = -1; @@ -1833,11 +1856,11 @@ int main(int argc, char *argv[]) NULLSTR(ctrl->def->seclabel.label), NULLSTR(ctrl->def->seclabel.imagelabel)); - if (ctrl->def->nnets != nveths) { - fprintf(stderr, "%s: expecting %d veths, but got %d\n", - argv[0], ctrl->def->nnets, nveths); + ctrl->veths = veths; + ctrl->nveths = nveths; + + if (virLXCControllerValidateNICs(ctrl) < 0) goto cleanup; - } if ((sockpath = lxcMonitorPath(ctrl)) == NULL) goto cleanup; @@ -1885,12 +1908,12 @@ int main(int argc, char *argv[]) } rc = virLXCControllerRun(ctrl, securityDriver, - nveths, veths, monitor, client, + monitor, client, ttyFDs, nttyFDs, handshakefd); cleanup: virPidFileDelete(LXC_STATE_DIR, name); - lxcControllerCleanupInterfaces(nveths, veths); + virLXCControllerDeleteInterfaces(ctrl); if (sockpath) unlink(sockpath); VIR_FREE(sockpath); -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:44 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Move the veth device name state into the virLXCControllerPtr object and stop passing it around. Also use size_t instead of unsigned int for the array length parameters.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_container.c | 10 +++--- src/lxc/lxc_container.h | 2 +- src/lxc/lxc_controller.c | 79 ++++++++++++++++++++++++++++++---------------- 3 files changed, 57 insertions(+), 34 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 910e82b..4f8703c 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -93,7 +93,7 @@ typedef struct __lxc_child_argv lxc_child_argv_t; struct __lxc_child_argv { virDomainDefPtr config; virSecurityManagerPtr securityDriver; - unsigned int nveths; + size_t nveths; char **veths; int monitor; char **ttyPaths; @@ -262,15 +262,15 @@ int lxcContainerWaitForContinue(int control) * Returns 0 on success or nonzero in case of error */ static int lxcContainerRenameAndEnableInterfaces(bool privNet, - unsigned int nveths, + size_t nveths, char **veths) { int rc = 0; - unsigned int i; + size_t i; char *newname = NULL;
for (i = 0 ; i < nveths ; i++) { - if (virAsprintf(&newname, "eth%d", i) < 0) { + if (virAsprintf(&newname, "eth%zu", i) < 0) { virReportOOMError(); rc = -1; goto error_out; @@ -1775,7 +1775,7 @@ const char *lxcContainerGetAlt32bitArch(const char *arch) */ int lxcContainerStart(virDomainDefPtr def, virSecurityManagerPtr securityDriver, - unsigned int nveths, + size_t nveths, char **veths, int control, int handshakefd, diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h index 77fb9b2..6f1cc8b 100644 --- a/src/lxc/lxc_container.h +++ b/src/lxc/lxc_container.h @@ -51,7 +51,7 @@ int lxcContainerWaitForContinue(int control);
int lxcContainerStart(virDomainDefPtr def, virSecurityManagerPtr securityDriver, - unsigned int nveths, + size_t nveths, char **veths, int control, int handshakefd, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7447f6c..ad11307 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -85,6 +85,9 @@ typedef virLXCController *virLXCControllerPtr; struct _virLXCController { char *name; virDomainDefPtr def; + + size_t nveths; + char **veths; };
static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -129,15 +132,35 @@ no_memory:
static void virLXCControllerFree(virLXCControllerPtr ctrl) { + size_t i; + if (!ctrl) return;
+ for (i = 0 ; i < ctrl->nveths ; i++) + VIR_FREE(ctrl->veths[i]); + VIR_FREE(ctrl->veths); + virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name);
VIR_FREE(ctrl); }
+ +static int virLXCControllerValidateNICs(virLXCControllerPtr ctrl) +{ + if (ctrl->def->nnets != ctrl->nveths) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("expecting %d veths, but got %zu"), + ctrl->def->nnets, ctrl->nveths); + return -1; + } + + return 0; +} + + static int lxcGetLoopFD(char **dev_name) { int fd = -1; @@ -1307,7 +1330,7 @@ cleanup2:
/** - * lxcControllerMoveInterfaces + * virLXCControllerMoveInterfaces * @nveths: number of interfaces * @veths: interface names * @container: pid of container @@ -1316,13 +1339,13 @@ cleanup2: * * Returns 0 on success or -1 in case of error */ -static int lxcControllerMoveInterfaces(unsigned int nveths, - char **veths, - pid_t container) +static int virLXCControllerMoveInterfaces(virLXCControllerPtr ctrl, + pid_t container) { - unsigned int i; - for (i = 0 ; i < nveths ; i++) - if (virNetDevSetNamespace(veths[i], container) < 0) + size_t i; + + for (i = 0 ; i < ctrl->nveths ; i++) + if (virNetDevSetNamespace(ctrl->veths[i], container) < 0) return -1;
Since you are touching this, I think it would be a good idea to add {} around the content of this for loop.
return 0; @@ -1330,24 +1353,26 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
/** - * lxcCleanupInterfaces: - * @nveths: number of interfaces - * @veths: interface names + * virLXCControllerDeleteInterfaces: + * @ctrl: the LXC controller * * Cleans up the container interfaces by deleting the veth device pairs. * * Returns 0 on success or -1 in case of error */ -static int lxcControllerCleanupInterfaces(unsigned int nveths, - char **veths) +static int virLXCControllerDeleteInterfaces(virLXCControllerPtr ctrl) { - unsigned int i; - for (i = 0 ; i < nveths ; i++) - ignore_value(virNetDevVethDelete(veths[i])); + size_t i; + int ret = 0;
- return 0; + for (i = 0 ; i < ctrl->nveths ; i++) + if (virNetDevVethDelete(ctrl->veths[i]) < 0) + ret = -1;
And here as well...
+ + return ret; }
+ static int lxcSetPersonality(virDomainDefPtr def) { struct utsname utsname; @@ -1422,8 +1447,6 @@ cleanup: static int virLXCControllerRun(virLXCControllerPtr ctrl, virSecurityManagerPtr securityDriver, - unsigned int nveths, - char **veths, int monitor, int client, int *ttyFDs, @@ -1588,8 +1611,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl,
if ((container = lxcContainerStart(ctrl->def, securityDriver, - nveths, - veths, + ctrl->nveths, + ctrl->veths, control[1], containerhandshake[1], containerTtyPaths, @@ -1598,7 +1621,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, VIR_FORCE_CLOSE(control[1]); VIR_FORCE_CLOSE(containerhandshake[1]);
- if (lxcControllerMoveInterfaces(nveths, veths, container) < 0) + if (virLXCControllerMoveInterfaces(ctrl, container) < 0) goto cleanup;
if (lxcContainerSendContinue(control[0]) < 0) { @@ -1684,7 +1707,7 @@ int main(int argc, char *argv[]) int rc = 1; int client; char *name = NULL; - int nveths = 0; + size_t nveths = 0; char **veths = NULL; int monitor = -1; int handshakefd = -1; @@ -1833,11 +1856,11 @@ int main(int argc, char *argv[]) NULLSTR(ctrl->def->seclabel.label), NULLSTR(ctrl->def->seclabel.imagelabel));
- if (ctrl->def->nnets != nveths) { - fprintf(stderr, "%s: expecting %d veths, but got %d\n", - argv[0], ctrl->def->nnets, nveths); + ctrl->veths = veths; + ctrl->nveths = nveths;
I was a bit afraid of double free after the two lines above but since we didn't actually free veths array before this patch (which was a little bit wrong), we are safe.
+ + if (virLXCControllerValidateNICs(ctrl) < 0) goto cleanup; - }
if ((sockpath = lxcMonitorPath(ctrl)) == NULL) goto cleanup; @@ -1885,12 +1908,12 @@ int main(int argc, char *argv[]) }
rc = virLXCControllerRun(ctrl, securityDriver, - nveths, veths, monitor, client, + monitor, client, ttyFDs, nttyFDs, handshakefd);
cleanup: virPidFileDelete(LXC_STATE_DIR, name); - lxcControllerCleanupInterfaces(nveths, veths); + virLXCControllerDeleteInterfaces(ctrl); if (sockpath) unlink(sockpath); VIR_FREE(sockpath);
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Keep a record of the init PID in the virLXCController object and create a virLXCControllerStopInit method for killing this process Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 74 +++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 30 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index ad11307..4c5943e 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -86,6 +86,8 @@ struct _virLXCController { char *name; virDomainDefPtr def; + pid_t initpid; + size_t nveths; char **veths; }; @@ -130,6 +132,17 @@ no_memory: goto cleanup; } + +static void virLXCControllerStopInit(virLXCControllerPtr ctrl) +{ + if (ctrl->initpid == 0) + return; + + virPidAbort(ctrl->initpid); + ctrl->initpid = 0; +} + + static void virLXCControllerFree(virLXCControllerPtr ctrl) { size_t i; @@ -137,6 +150,8 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) if (!ctrl) return; + virLXCControllerStopInit(ctrl); + for (i = 0 ; i < ctrl->nveths ; i++) VIR_FREE(ctrl->veths[i]); VIR_FREE(ctrl->veths); @@ -791,22 +806,23 @@ static bool quit = false; static virMutex lock; static int sigpipe[2]; -static void lxcSignalChildHandler(int signum ATTRIBUTE_UNUSED) +static void virLXCControllerSignalChildHandler(int signum ATTRIBUTE_UNUSED) { ignore_value(write(sigpipe[1], "1", 1)); } -static void lxcSignalChildIO(int watch ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED, - int events ATTRIBUTE_UNUSED, void *opaque) +static void virLXCControllerSignalChildIO(int watch ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED, + int events ATTRIBUTE_UNUSED, + void *opaque) { char buf[1]; int ret; - int *container = opaque; + virLXCControllerPtr ctrl = opaque; ignore_value(read(sigpipe[0], buf, 1)); ret = waitpid(-1, NULL, WNOHANG); - if (ret == *container) { + if (ret == ctrl->initpid) { virMutexLock(&lock); quit = true; virMutexUnlock(&lock); @@ -1174,12 +1190,12 @@ error: * * Returns 0 on success or -1 in case of error */ -static int lxcControllerMain(int serverFd, - int clientFd, - int *hostFds, - int *contFds, - size_t nFds, - pid_t container) +static int virLXCControllerMain(virLXCControllerPtr ctrl, + int serverFd, + int clientFd, + int *hostFds, + int *contFds, + size_t nFds) { struct lxcConsole *consoles = NULL; struct lxcMonitor monitor = { @@ -1201,15 +1217,15 @@ static int lxcControllerMain(int serverFd, if (virEventAddHandle(sigpipe[0], VIR_EVENT_HANDLE_READABLE, - lxcSignalChildIO, - &container, + virLXCControllerSignalChildIO, + ctrl, NULL) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch signal pipe")); goto cleanup; } - if (signal(SIGCHLD, lxcSignalChildHandler) == SIG_ERR) { + if (signal(SIGCHLD, virLXCControllerSignalChildHandler) == SIG_ERR) { virReportSystemError(errno, "%s", _("Cannot install signal handler")); goto cleanup; @@ -1339,13 +1355,12 @@ cleanup2: * * Returns 0 on success or -1 in case of error */ -static int virLXCControllerMoveInterfaces(virLXCControllerPtr ctrl, - pid_t container) +static int virLXCControllerMoveInterfaces(virLXCControllerPtr ctrl) { size_t i; for (i = 0 ; i < ctrl->nveths ; i++) - if (virNetDevSetNamespace(ctrl->veths[i], container) < 0) + if (virNetDevSetNamespace(ctrl->veths[i], ctrl->initpid) < 0) return -1; return 0; @@ -1458,7 +1473,6 @@ virLXCControllerRun(virLXCControllerPtr ctrl, int containerhandshake[2] = { -1, -1 }; int *containerTtyFDs = NULL; char **containerTtyPaths = NULL; - pid_t container = -1; virDomainFSDefPtr root; char *devpts = NULL; char *devptmx = NULL; @@ -1609,19 +1623,19 @@ virLXCControllerRun(virLXCControllerPtr ctrl, if (lxcSetPersonality(ctrl->def) < 0) goto cleanup; - if ((container = lxcContainerStart(ctrl->def, - securityDriver, - ctrl->nveths, - ctrl->veths, - control[1], - containerhandshake[1], - containerTtyPaths, - nttyFDs)) < 0) + if ((ctrl->initpid = lxcContainerStart(ctrl->def, + securityDriver, + ctrl->nveths, + ctrl->veths, + control[1], + containerhandshake[1], + containerTtyPaths, + nttyFDs)) < 0) goto cleanup; VIR_FORCE_CLOSE(control[1]); VIR_FORCE_CLOSE(containerhandshake[1]); - if (virLXCControllerMoveInterfaces(ctrl, container) < 0) + if (virLXCControllerMoveInterfaces(ctrl) < 0) goto cleanup; if (lxcContainerSendContinue(control[0]) < 0) { @@ -1669,7 +1683,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, } } - rc = lxcControllerMain(monitor, client, ttyFDs, containerTtyFDs, nttyFDs, container); + rc = virLXCControllerMain(ctrl, monitor, client, ttyFDs, containerTtyFDs, nttyFDs); monitor = client = -1; cleanup: @@ -1693,7 +1707,7 @@ cleanup: VIR_FORCE_CLOSE(loopDevs[i]); VIR_FREE(loopDevs); - virPidAbort(container); + virLXCControllerStopInit(ctrl); return rc; } -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:45 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Keep a record of the init PID in the virLXCController object and create a virLXCControllerStopInit method for killing this process
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 74 +++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 30 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Turn 'struct lxc_console' into virLXCControllerConsolePtr and make it a part of virLXCControllerPtr Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 264 ++++++++++++++++++++++++++-------------------- 1 file changed, 152 insertions(+), 112 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 4c5943e..32410ec 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -80,6 +80,30 @@ struct cgroup_device_policy { }; +typedef struct _virLXCControllerConsole virLXCControllerConsole; +typedef virLXCControllerConsole *virLXCControllerConsolePtr; +struct _virLXCControllerConsole { + int hostWatch; + int hostFd; /* PTY FD in the host OS */ + bool hostClosed; + int hostEpoll; + bool hostBlocking; + + int contWatch; + int contFd; /* PTY FD in the container */ + bool contClosed; + int contEpoll; + bool contBlocking; + + int epollWatch; + int epollFd; /* epoll FD for dealing with EOF */ + + size_t fromHostLen; + char fromHostBuf[1024]; + size_t fromContLen; + char fromContBuf[1024]; +}; + typedef struct _virLXCController virLXCController; typedef virLXCController *virLXCControllerPtr; struct _virLXCController { @@ -90,6 +114,9 @@ struct _virLXCController { size_t nveths; char **veths; + + size_t nconsoles; + virLXCControllerConsolePtr consoles; }; static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -143,6 +170,22 @@ static void virLXCControllerStopInit(virLXCControllerPtr ctrl) } +static void virLXCControllerConsoleClose(virLXCControllerConsolePtr console) +{ + if (console->hostWatch != -1) + virEventRemoveHandle(console->hostWatch); + VIR_FORCE_CLOSE(console->hostFd); + + if (console->contWatch != -1) + virEventRemoveHandle(console->contWatch); + VIR_FORCE_CLOSE(console->contFd); + + if (console->epollWatch != -1) + virEventRemoveHandle(console->epollWatch); + VIR_FORCE_CLOSE(console->epollFd); +} + + static void virLXCControllerFree(virLXCControllerPtr ctrl) { size_t i; @@ -156,6 +199,10 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) VIR_FREE(ctrl->veths[i]); VIR_FREE(ctrl->veths); + for (i = 0 ; i < ctrl->nconsoles ; i++) + virLXCControllerConsoleClose(&(ctrl->consoles[i])); + VIR_FREE(ctrl->consoles); + virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name); @@ -163,6 +210,38 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) } +static int virLXCControllerAddConsole(virLXCControllerPtr ctrl, + int hostFd) +{ + if (VIR_EXPAND_N(ctrl->consoles, ctrl->nconsoles, 1) < 0) { + virReportOOMError(); + return -1; + } + ctrl->consoles[ctrl->nconsoles-1].hostFd = hostFd; + ctrl->consoles[ctrl->nconsoles-1].hostWatch = -1; + + ctrl->consoles[ctrl->nconsoles-1].contFd = -1; + ctrl->consoles[ctrl->nconsoles-1].contWatch = -1; + + ctrl->consoles[ctrl->nconsoles-1].epollFd = -1; + ctrl->consoles[ctrl->nconsoles-1].epollWatch = -1; + return 0; +} + + +static int virLXCControllerConsoleSetNonblocking(virLXCControllerConsolePtr console) +{ + if (virSetBlocking(console->hostFd, false) < 0 || + virSetBlocking(console->contFd, false) < 0) { + virReportSystemError(errno, "%s", + _("Unable to set console file descriptor non-blocking")); + return -1; + } + + return 0; +} + + static int virLXCControllerValidateNICs(virLXCControllerPtr ctrl) { if (ctrl->def->nnets != ctrl->nveths) { @@ -176,6 +255,19 @@ static int virLXCControllerValidateNICs(virLXCControllerPtr ctrl) } +static int virLXCControllerValidateConsoles(virLXCControllerPtr ctrl) +{ + if (ctrl->def->nconsoles != ctrl->nconsoles) { + lxcError(VIR_ERR_INTERNAL_ERROR, + _("expecting %d consoles, but got %zu tty file handlers"), + ctrl->def->nconsoles, ctrl->nconsoles); + return -1; + } + + return 0; +} + + static int lxcGetLoopFD(char **dev_name) { int fd = -1; @@ -830,29 +922,6 @@ static void virLXCControllerSignalChildIO(int watch ATTRIBUTE_UNUSED, } -struct lxcConsole { - - int hostWatch; - int hostFd; /* PTY FD in the host OS */ - bool hostClosed; - int hostEpoll; - bool hostBlocking; - - int contWatch; - int contFd; /* PTY FD in the container */ - bool contClosed; - int contEpoll; - bool contBlocking; - - int epollWatch; - int epollFd; /* epoll FD for dealing with EOF */ - - size_t fromHostLen; - char fromHostBuf[1024]; - size_t fromContLen; - char fromContBuf[1024]; -}; - struct lxcMonitor { int serverWatch; int serverFd; /* Server listen socket */ @@ -936,7 +1005,7 @@ static void lxcServerAccept(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRI } } -static void lxcConsoleUpdateWatch(struct lxcConsole *console) +static void virLXCControllerConsoleUpdateWatch(virLXCControllerConsolePtr console) { int hostEvents = 0; int contEvents = 0; @@ -1038,9 +1107,9 @@ cleanup: } -static void lxcEpollIO(int watch, int fd, int events, void *opaque) +static void virLXCControllerConsoleEPoll(int watch, int fd, int events, void *opaque) { - struct lxcConsole *console = opaque; + virLXCControllerConsolePtr console = opaque; virMutexLock(&lock); VIR_DEBUG("IO event watch=%d fd=%d events=%d fromHost=%zu fromcont=%zu", @@ -1076,7 +1145,7 @@ static void lxcEpollIO(int watch, int fd, int events, void *opaque) } else { console->contClosed = false; } - lxcConsoleUpdateWatch(console); + virLXCControllerConsoleUpdateWatch(console); break; } } @@ -1085,9 +1154,9 @@ cleanup: virMutexUnlock(&lock); } -static void lxcConsoleIO(int watch, int fd, int events, void *opaque) +static void virLXCControllerConsoleIO(int watch, int fd, int events, void *opaque) { - struct lxcConsole *console = opaque; + virLXCControllerConsolePtr console = opaque; virMutexLock(&lock); VIR_DEBUG("IO event watch=%d fd=%d events=%d fromHost=%zu fromcont=%zu", @@ -1166,7 +1235,7 @@ static void lxcConsoleIO(int watch, int fd, int events, void *opaque) VIR_DEBUG("Got EOF on %d %d", watch, fd); } - lxcConsoleUpdateWatch(console); + virLXCControllerConsoleUpdateWatch(console); virMutexUnlock(&lock); return; @@ -1183,8 +1252,6 @@ error: * lxcControllerMain * @serverFd: server socket fd to accept client requests * @clientFd: initial client which is the libvirtd daemon - * @hostFd: open fd for application facing Pty - * @contFd: open fd for container facing Pty * * Processes I/O on consoles and the monitor * @@ -1192,12 +1259,8 @@ error: */ static int virLXCControllerMain(virLXCControllerPtr ctrl, int serverFd, - int clientFd, - int *hostFds, - int *contFds, - size_t nFds) + int clientFd) { - struct lxcConsole *consoles = NULL; struct lxcMonitor monitor = { .serverFd = serverFd, .clientFd = clientFd, @@ -1256,53 +1319,38 @@ static int virLXCControllerMain(virLXCControllerPtr ctrl, goto cleanup; } - if (VIR_ALLOC_N(consoles, nFds) < 0) { - virReportOOMError(); - goto cleanup; - } - - for (i = 0 ; i < nFds ; i++) { - consoles[i].epollFd = -1; - consoles[i].epollWatch = -1; - consoles[i].hostWatch = -1; - consoles[i].contWatch = -1; - } - - for (i = 0 ; i < nFds ; i++) { - consoles[i].hostFd = hostFds[i]; - consoles[i].contFd = contFds[i]; - - if ((consoles[i].epollFd = epoll_create1(EPOLL_CLOEXEC)) < 0) { + for (i = 0 ; i < ctrl->nconsoles ; i++) { + if ((ctrl->consoles[i].epollFd = epoll_create1(EPOLL_CLOEXEC)) < 0) { virReportSystemError(errno, "%s", _("Unable to create epoll fd")); goto cleanup; } - if ((consoles[i].epollWatch = virEventAddHandle(consoles[i].epollFd, - VIR_EVENT_HANDLE_READABLE, - lxcEpollIO, - &consoles[i], - NULL)) < 0) { + if ((ctrl->consoles[i].epollWatch = virEventAddHandle(ctrl->consoles[i].epollFd, + VIR_EVENT_HANDLE_READABLE, + virLXCControllerConsoleEPoll, + &(ctrl->consoles[i]), + NULL)) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch epoll FD")); goto cleanup; } - if ((consoles[i].hostWatch = virEventAddHandle(consoles[i].hostFd, - VIR_EVENT_HANDLE_READABLE, - lxcConsoleIO, - &consoles[i], - NULL)) < 0) { + if ((ctrl->consoles[i].hostWatch = virEventAddHandle(ctrl->consoles[i].hostFd, + VIR_EVENT_HANDLE_READABLE, + virLXCControllerConsoleIO, + &(ctrl->consoles[i]), + NULL)) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch host console PTY")); goto cleanup; } - if ((consoles[i].contWatch = virEventAddHandle(consoles[i].contFd, - VIR_EVENT_HANDLE_READABLE, - lxcConsoleIO, - &consoles[i], - NULL)) < 0) { + if ((ctrl->consoles[i].contWatch = virEventAddHandle(ctrl->consoles[i].contFd, + VIR_EVENT_HANDLE_READABLE, + virLXCControllerConsoleIO, + &(ctrl->consoles[i]), + NULL)) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch host console PTY")); goto cleanup; @@ -1329,17 +1377,9 @@ cleanup2: VIR_FORCE_CLOSE(monitor.serverFd); VIR_FORCE_CLOSE(monitor.clientFd); - for (i = 0 ; i < nFds ; i++) { - if (consoles[i].epollWatch != -1) - virEventRemoveHandle(consoles[i].epollWatch); - VIR_FORCE_CLOSE(consoles[i].epollFd); - if (consoles[i].contWatch != -1) - virEventRemoveHandle(consoles[i].contWatch); - if (consoles[i].hostWatch != -1) - virEventRemoveHandle(consoles[i].hostWatch); - } + for (i = 0 ; i < ctrl->nconsoles ; i++) + virLXCControllerConsoleClose(&(ctrl->consoles[i])); - VIR_FREE(consoles); return rc; } @@ -1464,15 +1504,12 @@ virLXCControllerRun(virLXCControllerPtr ctrl, virSecurityManagerPtr securityDriver, int monitor, int client, - int *ttyFDs, - size_t nttyFDs, int handshakefd) { int rc = -1; int control[2] = { -1, -1}; int containerhandshake[2] = { -1, -1 }; - int *containerTtyFDs = NULL; - char **containerTtyPaths = NULL; + char **containerTTYPaths = NULL; virDomainFSDefPtr root; char *devpts = NULL; char *devptmx = NULL; @@ -1481,11 +1518,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, size_t i; char *mount_options = NULL; - if (VIR_ALLOC_N(containerTtyFDs, nttyFDs) < 0) { - virReportOOMError(); - goto cleanup; - } - if (VIR_ALLOC_N(containerTtyPaths, nttyFDs) < 0) { + if (VIR_ALLOC_N(containerTTYPaths, ctrl->nconsoles) < 0) { virReportOOMError(); goto cleanup; } @@ -1591,27 +1624,28 @@ virLXCControllerRun(virLXCControllerPtr ctrl, VIR_FREE(devptmx); } } else { - if (nttyFDs != 1) { + if (ctrl->nconsoles != 1) { lxcError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Expected exactly one TTY fd, but got %zu"), nttyFDs); + _("Expected exactly one console, but got %zu"), + ctrl->nconsoles); goto cleanup; } } - for (i = 0 ; i < nttyFDs ; i++) { + for (i = 0 ; i < ctrl->nconsoles ; i++) { if (devptmx) { VIR_DEBUG("Opening tty on private %s", devptmx); if (lxcCreateTty(devptmx, - &containerTtyFDs[i], - &containerTtyPaths[i]) < 0) { + &ctrl->consoles[i].contFd, + &containerTTYPaths[i]) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); goto cleanup; } } else { VIR_DEBUG("Opening tty on shared /dev/ptmx"); - if (virFileOpenTty(&containerTtyFDs[i], - &containerTtyPaths[i], + if (virFileOpenTty(&ctrl->consoles[i].contFd, + &containerTTYPaths[i], 0) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); @@ -1629,8 +1663,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl, ctrl->veths, control[1], containerhandshake[1], - containerTtyPaths, - nttyFDs)) < 0) + containerTTYPaths, + ctrl->nconsoles)) < 0) goto cleanup; VIR_FORCE_CLOSE(control[1]); VIR_FORCE_CLOSE(containerhandshake[1]); @@ -1674,16 +1708,11 @@ virLXCControllerRun(virLXCControllerPtr ctrl, _("Unable to set file descriptor non-blocking")); goto cleanup; } - for (i = 0 ; i < nttyFDs ; i++) { - if (virSetBlocking(ttyFDs[i], false) < 0 || - virSetBlocking(containerTtyFDs[i], false) < 0) { - virReportSystemError(errno, "%s", - _("Unable to set file descriptor non-blocking")); + for (i = 0 ; i < ctrl->nconsoles ; i++) + if (virLXCControllerConsoleSetNonblocking(&(ctrl->consoles[i])) < 0) goto cleanup; - } - } - rc = virLXCControllerMain(ctrl, monitor, client, ttyFDs, containerTtyFDs, nttyFDs); + rc = virLXCControllerMain(ctrl, monitor, client); monitor = client = -1; cleanup: @@ -1696,12 +1725,9 @@ cleanup: VIR_FORCE_CLOSE(containerhandshake[0]); VIR_FORCE_CLOSE(containerhandshake[1]); - for (i = 0 ; i < nttyFDs ; i++) - VIR_FREE(containerTtyPaths[i]); - VIR_FREE(containerTtyPaths); - for (i = 0 ; i < nttyFDs ; i++) - VIR_FORCE_CLOSE(containerTtyFDs[i]); - VIR_FREE(containerTtyFDs); + for (i = 0 ; i < ctrl->nconsoles ; i++) + VIR_FREE(containerTTYPaths[i]); + VIR_FREE(containerTTYPaths); for (i = 0 ; i < nloopDevs ; i++) VIR_FORCE_CLOSE(loopDevs[i]); @@ -1741,6 +1767,7 @@ int main(int argc, char *argv[]) size_t nttyFDs = 0; virSecurityManagerPtr securityDriver = NULL; virLXCControllerPtr ctrl = NULL; + size_t i; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || @@ -1873,9 +1900,18 @@ int main(int argc, char *argv[]) ctrl->veths = veths; ctrl->nveths = nveths; + for (i = 0 ; i < nttyFDs ; i++) { + if (virLXCControllerAddConsole(ctrl, ttyFDs[i]) < 0) + goto cleanup; + ttyFDs[i] = -1; + } + if (virLXCControllerValidateNICs(ctrl) < 0) goto cleanup; + if (virLXCControllerValidateConsoles(ctrl) < 0) + goto cleanup; + if ((sockpath = lxcMonitorPath(ctrl)) == NULL) goto cleanup; @@ -1923,7 +1959,7 @@ int main(int argc, char *argv[]) rc = virLXCControllerRun(ctrl, securityDriver, monitor, client, - ttyFDs, nttyFDs, handshakefd); + handshakefd); cleanup: virPidFileDelete(LXC_STATE_DIR, name); @@ -1931,6 +1967,10 @@ cleanup: if (sockpath) unlink(sockpath); VIR_FREE(sockpath); + for (i = 0 ; i < nttyFDs ; i++) + VIR_FORCE_CLOSE(ttyFDs[i]); + VIR_FREE(ttyFDs); + virLXCControllerFree(ctrl); return rc ? EXIT_FAILURE : EXIT_SUCCESS; -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:46 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Turn 'struct lxc_console' into virLXCControllerConsolePtr and make it a part of virLXCControllerPtr
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 264 ++++++++++++++++++++++++++-------------------- 1 file changed, 152 insertions(+), 112 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Keep the FD uses to handshake with the libvirtd daemon in the virLXCControllerPtr object. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 32410ec..06d31d3 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -110,6 +110,8 @@ struct _virLXCController { char *name; virDomainDefPtr def; + int handshakeFd; + pid_t initpid; size_t nveths; @@ -203,6 +205,8 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) virLXCControllerConsoleClose(&(ctrl->consoles[i])); VIR_FREE(ctrl->consoles); + VIR_FORCE_CLOSE(ctrl->handshakeFd); + virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name); @@ -242,6 +246,18 @@ static int virLXCControllerConsoleSetNonblocking(virLXCControllerConsolePtr cons } +static int virLXCControllerDaemonHandshake(virLXCControllerPtr ctrl) +{ + if (lxcContainerSendContinue(ctrl->handshakeFd) < 0) { + virReportSystemError(errno, "%s", + _("error sending continue signal to daemon")); + return -1; + } + VIR_FORCE_CLOSE(ctrl->handshakeFd); + return 0; +} + + static int virLXCControllerValidateNICs(virLXCControllerPtr ctrl) { if (ctrl->def->nnets != ctrl->nveths) { @@ -1503,8 +1519,7 @@ static int virLXCControllerRun(virLXCControllerPtr ctrl, virSecurityManagerPtr securityDriver, int monitor, - int client, - int handshakefd) + int client) { int rc = -1; int control[2] = { -1, -1}; @@ -1695,12 +1710,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl, if (lxcControllerClearCapabilities() < 0) goto cleanup; - if (lxcContainerSendContinue(handshakefd) < 0) { - virReportSystemError(errno, "%s", - _("error sending continue signal to parent")); + if (virLXCControllerDaemonHandshake(ctrl) < 0) goto cleanup; - } - VIR_FORCE_CLOSE(handshakefd); if (virSetBlocking(monitor, false) < 0 || virSetBlocking(client, false) < 0) { @@ -1721,7 +1732,6 @@ cleanup: VIR_FREE(devpts); VIR_FORCE_CLOSE(control[0]); VIR_FORCE_CLOSE(control[1]); - VIR_FORCE_CLOSE(handshakefd); VIR_FORCE_CLOSE(containerhandshake[0]); VIR_FORCE_CLOSE(containerhandshake[1]); @@ -1750,7 +1760,7 @@ int main(int argc, char *argv[]) size_t nveths = 0; char **veths = NULL; int monitor = -1; - int handshakefd = -1; + int handshakeFd = -1; int bg = 0; char *sockpath = NULL; const struct option options[] = { @@ -1824,7 +1834,7 @@ int main(int argc, char *argv[]) break; case 's': - if (virStrToLong_i(optarg, NULL, 10, &handshakefd) < 0) { + if (virStrToLong_i(optarg, NULL, 10, &handshakeFd) < 0) { fprintf(stderr, "malformed --handshakefd argument '%s'", optarg); goto cleanup; @@ -1875,7 +1885,7 @@ int main(int argc, char *argv[]) goto cleanup; } - if (handshakefd < 0) { + if (handshakeFd < 0) { fprintf(stderr, "%s: missing --handshake argument for container PTY\n", argv[0]); goto cleanup; @@ -1891,6 +1901,8 @@ int main(int argc, char *argv[]) if (!(ctrl = virLXCControllerNew(name))) goto cleanup; + ctrl->handshakeFd = handshakeFd; + VIR_DEBUG("Security model %s type %s label %s imagelabel %s", NULLSTR(ctrl->def->seclabel.model), virDomainSeclabelTypeToString(ctrl->def->seclabel.type), @@ -1958,8 +1970,7 @@ int main(int argc, char *argv[]) } rc = virLXCControllerRun(ctrl, securityDriver, - monitor, client, - handshakefd); + monitor, client); cleanup: virPidFileDelete(LXC_STATE_DIR, name); -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:47 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Keep the FD uses to handshake with the libvirtd daemon in the
s/uses/used/
virLXCControllerPtr object.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Move the list of loop device FDs into the virLXCControllerPtr object and make sure that virLXCControllerStopInit will close them all Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 50 +++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 06d31d3..d6002c4 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -119,6 +119,9 @@ struct _virLXCController { size_t nconsoles; virLXCControllerConsolePtr consoles; + + size_t nloopDevs; + int *loopDevFds; }; static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -162,11 +165,33 @@ no_memory: } +static int virLXCControllerCloseLoopDevices(virLXCControllerPtr ctrl, + bool force) +{ + size_t i; + + for (i = 0 ; i < ctrl->nloopDevs ; i++) { + if (force) { + VIR_FORCE_CLOSE(ctrl->loopDevFds[i]); + } else { + if (VIR_CLOSE(ctrl->loopDevFds[i]) < 0) { + virReportSystemError(errno, "%s", + _("Unable to close loop device")); + return -1; + } + } + } + + return 0; +} + + static void virLXCControllerStopInit(virLXCControllerPtr ctrl) { if (ctrl->initpid == 0) return; + virLXCControllerCloseLoopDevices(ctrl, true); virPidAbort(ctrl->initpid); ctrl->initpid = 0; } @@ -406,28 +431,28 @@ cleanup: } -static int lxcSetupLoopDevices(virDomainDefPtr def, size_t *nloopDevs, int **loopDevs) +static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl) { size_t i; int ret = -1; - for (i = 0 ; i < def->nfss ; i++) { + for (i = 0 ; i < ctrl->def->nfss ; i++) { int fd; - if (def->fss[i]->type != VIR_DOMAIN_FS_TYPE_FILE) + if (ctrl->def->fss[i]->type != VIR_DOMAIN_FS_TYPE_FILE) continue; - fd = lxcSetupLoopDevice(def->fss[i]); + fd = lxcSetupLoopDevice(ctrl->def->fss[i]); if (fd < 0) goto cleanup; VIR_DEBUG("Saving loop fd %d", fd); - if (VIR_REALLOC_N(*loopDevs, *nloopDevs+1) < 0) { + if (VIR_EXPAND_N(ctrl->loopDevFds, ctrl->nloopDevs, 1) < 0) { VIR_FORCE_CLOSE(fd); virReportOOMError(); goto cleanup; } - (*loopDevs)[(*nloopDevs)++] = fd; + ctrl->loopDevFds[ctrl->nloopDevs - 1] = fd; } VIR_DEBUG("Setup all loop devices"); @@ -1528,8 +1553,6 @@ virLXCControllerRun(virLXCControllerPtr ctrl, virDomainFSDefPtr root; char *devpts = NULL; char *devptmx = NULL; - size_t nloopDevs = 0; - int *loopDevs = NULL; size_t i; char *mount_options = NULL; @@ -1550,7 +1573,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, goto cleanup; } - if (lxcSetupLoopDevices(ctrl->def, &nloopDevs, &loopDevs) < 0) + if (virLXCControllerSetupLoopDevices(ctrl) < 0) goto cleanup; root = virDomainGetRootFilesystem(ctrl->def); @@ -1702,9 +1725,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl, /* Now the container is fully setup... */ /* ...we can close the loop devices... */ - - for (i = 0 ; i < nloopDevs ; i++) - VIR_FORCE_CLOSE(loopDevs[i]); + if (virLXCControllerCloseLoopDevices(ctrl, false) < 0) + goto cleanup; /* ...and reduce our privileges */ if (lxcControllerClearCapabilities() < 0) @@ -1739,10 +1761,6 @@ cleanup: VIR_FREE(containerTTYPaths[i]); VIR_FREE(containerTTYPaths); - for (i = 0 ; i < nloopDevs ; i++) - VIR_FORCE_CLOSE(loopDevs[i]); - VIR_FREE(loopDevs); - virLXCControllerStopInit(ctrl); return rc; -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:48 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Move the list of loop device FDs into the virLXCControllerPtr object and make sure that virLXCControllerStopInit will close them all
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 50 +++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 16 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Move the security manager object into the virLXCControllerPtr object. Also simplify the code creating it in the first place Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index d6002c4..af8a936 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -122,6 +122,8 @@ struct _virLXCController { size_t nloopDevs; int *loopDevFds; + + virSecurityManagerPtr securityManager; }; static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -222,6 +224,8 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) virLXCControllerStopInit(ctrl); + virSecurityManagerFree(ctrl->securityManager); + for (i = 0 ; i < ctrl->nveths ; i++) VIR_FREE(ctrl->veths[i]); VIR_FREE(ctrl->veths); @@ -1542,7 +1546,6 @@ cleanup: static int virLXCControllerRun(virLXCControllerPtr ctrl, - virSecurityManagerPtr securityDriver, int monitor, int client) { @@ -1602,7 +1605,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl, * marked as shared */ if (root) { - mount_options = virSecurityManagerGetMountOptions(securityDriver, ctrl->def); + mount_options = virSecurityManagerGetMountOptions(ctrl->securityManager, + ctrl->def); char *opts; VIR_DEBUG("Setting up private /dev/pts"); @@ -1696,7 +1700,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, goto cleanup; if ((ctrl->initpid = lxcContainerStart(ctrl->def, - securityDriver, + ctrl->securityManager, ctrl->nveths, ctrl->veths, control[1], @@ -1793,9 +1797,9 @@ int main(int argc, char *argv[]) }; int *ttyFDs = NULL; size_t nttyFDs = 0; - virSecurityManagerPtr securityDriver = NULL; virLXCControllerPtr ctrl = NULL; size_t i; + const char *securityDriver = "none"; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || @@ -1860,13 +1864,7 @@ int main(int argc, char *argv[]) break; case 'S': - if (!(securityDriver = virSecurityManagerNew(optarg, - LXC_DRIVER_NAME, - false, false, false))) { - fprintf(stderr, "Cannot create security manager '%s'", - optarg); - goto cleanup; - } + securityDriver = optarg; break; case 'h': @@ -1888,16 +1886,6 @@ int main(int argc, char *argv[]) } } - if (securityDriver == NULL) { - if (!(securityDriver = virSecurityManagerNew("none", - LXC_DRIVER_NAME, - false, false, false))) { - fprintf(stderr, "%s: cannot initialize nop security manager", argv[0]); - goto cleanup; - } - } - - if (name == NULL) { fprintf(stderr, "%s: missing --name argument for configuration\n", argv[0]); goto cleanup; @@ -1921,6 +1909,11 @@ int main(int argc, char *argv[]) ctrl->handshakeFd = handshakeFd; + if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver, + LXC_DRIVER_NAME, + false, false, false))) + goto cleanup; + VIR_DEBUG("Security model %s type %s label %s imagelabel %s", NULLSTR(ctrl->def->seclabel.model), virDomainSeclabelTypeToString(ctrl->def->seclabel.type), @@ -1987,7 +1980,7 @@ int main(int argc, char *argv[]) goto cleanup; } - rc = virLXCControllerRun(ctrl, securityDriver, + rc = virLXCControllerRun(ctrl, monitor, client); cleanup: -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:49 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Move the security manager object into the virLXCControllerPtr object. Also simplify the code creating it in the first place
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> The virLXCControllerRun method is getting a little too large, and about 50% of its code is related to setting up a /dev/pts mount. Move the latter out into a dedicated method Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 229 ++++++++++++++++++++++++++-------------------- 1 file changed, 128 insertions(+), 101 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index af8a936..1ae53de 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -119,6 +119,7 @@ struct _virLXCController { size_t nconsoles; virLXCControllerConsolePtr consoles; + char *devptmx; size_t nloopDevs; int *loopDevFds; @@ -236,6 +237,8 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) VIR_FORCE_CLOSE(ctrl->handshakeFd); + VIR_FREE(ctrl->devptmx); + virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name); @@ -1544,45 +1547,27 @@ cleanup: return ret; } + static int -virLXCControllerRun(virLXCControllerPtr ctrl, - int monitor, - int client) +virLXCControllerSetupDevPTS(virLXCControllerPtr ctrl) { - int rc = -1; - int control[2] = { -1, -1}; - int containerhandshake[2] = { -1, -1 }; - char **containerTTYPaths = NULL; - virDomainFSDefPtr root; - char *devpts = NULL; - char *devptmx = NULL; - size_t i; + virDomainFSDefPtr root = virDomainGetRootFilesystem(ctrl->def); char *mount_options = NULL; + char *opts; + char *devpts = NULL; + int ret = -1; - if (VIR_ALLOC_N(containerTTYPaths, ctrl->nconsoles) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (socketpair(PF_UNIX, SOCK_STREAM, 0, control) < 0) { - virReportSystemError(errno, "%s", - _("sockpair failed")); - goto cleanup; - } - - if (socketpair(PF_UNIX, SOCK_STREAM, 0, containerhandshake) < 0) { - virReportSystemError(errno, "%s", - _("socketpair failed")); - goto cleanup; + if (!root) { + if (ctrl->nconsoles != 1) { + lxcError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Expected exactly one console, but got %zu"), + ctrl->nconsoles); + return -1; + } + return 0; } - if (virLXCControllerSetupLoopDevices(ctrl) < 0) - goto cleanup; - - root = virDomainGetRootFilesystem(ctrl->def); - - if (lxcSetContainerResources(ctrl->def) < 0) - goto cleanup; + VIR_DEBUG("Setting up private /dev/pts"); /* * If doing a chroot style setup, we need to prepare @@ -1604,85 +1589,87 @@ virLXCControllerRun(virLXCControllerPtr ctrl, * into slave mode, just in case it was currently * marked as shared */ - if (root) { - mount_options = virSecurityManagerGetMountOptions(ctrl->securityManager, - ctrl->def); - char *opts; - VIR_DEBUG("Setting up private /dev/pts"); + mount_options = virSecurityManagerGetMountOptions(ctrl->securityManager, + ctrl->def); - if (!virFileExists(root->src)) { - virReportSystemError(errno, - _("root source %s does not exist"), - root->src); - goto cleanup; - } + if (!virFileExists(root->src)) { + virReportSystemError(errno, + _("root source %s does not exist"), + root->src); + goto cleanup; + } - if (unshare(CLONE_NEWNS) < 0) { - virReportSystemError(errno, "%s", - _("Cannot unshare mount namespace")); - goto cleanup; - } + if (unshare(CLONE_NEWNS) < 0) { + virReportSystemError(errno, "%s", + _("Cannot unshare mount namespace")); + goto cleanup; + } - if (mount("", "/", NULL, MS_SLAVE|MS_REC, NULL) < 0) { - virReportSystemError(errno, "%s", - _("Failed to switch root mount into slave mode")); - goto cleanup; - } + if (mount("", "/", NULL, MS_SLAVE|MS_REC, NULL) < 0) { + virReportSystemError(errno, "%s", + _("Failed to switch root mount into slave mode")); + goto cleanup; + } - if (virAsprintf(&devpts, "%s/dev/pts", root->src) < 0 || - virAsprintf(&devptmx, "%s/dev/pts/ptmx", root->src) < 0) { - virReportOOMError(); - goto cleanup; - } + if (virAsprintf(&devpts, "%s/dev/pts", root->src) < 0 || + virAsprintf(&ctrl->devptmx, "%s/dev/pts/ptmx", root->src) < 0) { + virReportOOMError(); + goto cleanup; + } - if (virFileMakePath(devpts) < 0) { - virReportSystemError(errno, - _("Failed to make path %s"), - devpts); - goto cleanup; - } + if (virFileMakePath(devpts) < 0) { + virReportSystemError(errno, + _("Failed to make path %s"), + devpts); + goto cleanup; + } - /* XXX should we support gid=X for X!=5 for distros which use - * a different gid for tty? */ - if (virAsprintf(&opts, "newinstance,ptmxmode=0666,mode=0620,gid=5%s", - (mount_options ? mount_options : "")) < 0) { - virReportOOMError(); - goto cleanup; - } + /* XXX should we support gid=X for X!=5 for distros which use + * a different gid for tty? */ + if (virAsprintf(&opts, "newinstance,ptmxmode=0666,mode=0620,gid=5%s", + (mount_options ? mount_options : "")) < 0) { + virReportOOMError(); + goto cleanup; + } - VIR_DEBUG("Mount devpts on %s type=tmpfs flags=%x, opts=%s", - devpts, MS_NOSUID, opts); - if (mount("devpts", devpts, "devpts", MS_NOSUID, opts) < 0) { - VIR_FREE(opts); - virReportSystemError(errno, - _("Failed to mount devpts on %s"), - devpts); - goto cleanup; - } - VIR_FREE(opts); + VIR_DEBUG("Mount devpts on %s type=tmpfs flags=%x, opts=%s", + devpts, MS_NOSUID, opts); + if (mount("devpts", devpts, "devpts", MS_NOSUID, opts) < 0) { + virReportSystemError(errno, + _("Failed to mount devpts on %s"), + devpts); + goto cleanup; + } - if (access(devptmx, R_OK) < 0) { - VIR_WARN("Kernel does not support private devpts, using shared devpts"); - VIR_FREE(devptmx); - } - } else { - if (ctrl->nconsoles != 1) { - lxcError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Expected exactly one console, but got %zu"), - ctrl->nconsoles); - goto cleanup; - } + if (access(ctrl->devptmx, R_OK) < 0) { + VIR_WARN("Kernel does not support private devpts, using shared devpts"); + VIR_FREE(ctrl->devptmx); } + ret = 0; + +cleanup: + VIR_FREE(opts); + VIR_FREE(devpts); + return ret; +} + + +static int +virLXCControllerSetupConsoles(virLXCControllerPtr ctrl, + char **containerTTYPaths) +{ + size_t i; + for (i = 0 ; i < ctrl->nconsoles ; i++) { - if (devptmx) { - VIR_DEBUG("Opening tty on private %s", devptmx); - if (lxcCreateTty(devptmx, + if (ctrl->devptmx) { + VIR_DEBUG("Opening tty on private %s", ctrl->devptmx); + if (lxcCreateTty(ctrl->devptmx, &ctrl->consoles[i].contFd, &containerTTYPaths[i]) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); - goto cleanup; + return -1; } } else { VIR_DEBUG("Opening tty on shared /dev/ptmx"); @@ -1691,10 +1678,53 @@ virLXCControllerRun(virLXCControllerPtr ctrl, 0) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); - goto cleanup; + return -1; } } } + return 0; +} + + +static int +virLXCControllerRun(virLXCControllerPtr ctrl, + int monitor, + int client) +{ + int rc = -1; + int control[2] = { -1, -1}; + int containerhandshake[2] = { -1, -1 }; + char **containerTTYPaths = NULL; + size_t i; + + if (VIR_ALLOC_N(containerTTYPaths, ctrl->nconsoles) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (socketpair(PF_UNIX, SOCK_STREAM, 0, control) < 0) { + virReportSystemError(errno, "%s", + _("sockpair failed")); + goto cleanup; + } + + if (socketpair(PF_UNIX, SOCK_STREAM, 0, containerhandshake) < 0) { + virReportSystemError(errno, "%s", + _("socketpair failed")); + goto cleanup; + } + + if (virLXCControllerSetupLoopDevices(ctrl) < 0) + goto cleanup; + + if (lxcSetContainerResources(ctrl->def) < 0) + goto cleanup; + + if (virLXCControllerSetupDevPTS(ctrl) < 0) + goto cleanup; + + if (virLXCControllerSetupConsoles(ctrl, containerTTYPaths) < 0) + goto cleanup; if (lxcSetPersonality(ctrl->def) < 0) goto cleanup; @@ -1753,9 +1783,6 @@ virLXCControllerRun(virLXCControllerPtr ctrl, monitor = client = -1; cleanup: - VIR_FREE(mount_options); - VIR_FREE(devptmx); - VIR_FREE(devpts); VIR_FORCE_CLOSE(control[0]); VIR_FORCE_CLOSE(control[1]); VIR_FORCE_CLOSE(containerhandshake[0]); -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:50 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virLXCControllerRun method is getting a little too large, and about 50% of its code is related to setting up a /dev/pts mount. Move the latter out into a dedicated method
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 229 ++++++++++++++++++++++++++-------------------- 1 file changed, 128 insertions(+), 101 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Move the monitor FDs into the virLXCControllerPtr object removing the need for the 'struct lxcMonitor' object Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 115 ++++++++++++++++++++++------------------------ 1 file changed, 55 insertions(+), 60 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1ae53de..fc350ea 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -125,6 +125,11 @@ struct _virLXCController { int *loopDevFds; virSecurityManagerPtr securityManager; + + int monitorServerFd; + int monitorServerWatch; + int monitorClientFd; + int monitorClientWatch; }; static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -138,6 +143,11 @@ static virLXCControllerPtr virLXCControllerNew(const char *name) if (VIR_ALLOC(ctrl) < 0) goto no_memory; + ctrl->monitorServerFd = -1; + ctrl->monitorServerWatch = -1; + ctrl->monitorClientFd = -1; + ctrl->monitorClientWatch = -1; + if (!(ctrl->name = strdup(name))) goto no_memory; @@ -242,6 +252,13 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name); + if (ctrl->monitorServerWatch != -1) + virEventRemoveHandle(ctrl->monitorServerWatch); + VIR_FORCE_CLOSE(ctrl->monitorServerFd); + if (ctrl->monitorClientWatch != -1) + virEventRemoveHandle(ctrl->monitorClientWatch); + VIR_FORCE_CLOSE(ctrl->monitorClientFd); + VIR_FREE(ctrl); } @@ -970,24 +987,16 @@ static void virLXCControllerSignalChildIO(int watch ATTRIBUTE_UNUSED, } -struct lxcMonitor { - int serverWatch; - int serverFd; /* Server listen socket */ - int clientWatch; - int clientFd; /* Current client FD (if any) */ -}; - - -static void lxcClientIO(int watch ATTRIBUTE_UNUSED, int fd, int events, void *opaque) +static void virLXCControllerClientIO(int watch ATTRIBUTE_UNUSED, int fd, int events, void *opaque) { - struct lxcMonitor *monitor = opaque; + virLXCControllerPtr ctrl = opaque; char buf[1024]; ssize_t ret; if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) { - virEventRemoveHandle(monitor->clientWatch); - monitor->clientWatch = -1; + virEventRemoveHandle(ctrl->monitorClientWatch); + ctrl->monitorClientWatch = -1; return; } @@ -1007,16 +1016,16 @@ reread: } if (ret == 0) { VIR_DEBUG("Client %d gone", fd); - VIR_FORCE_CLOSE(monitor->clientFd); - virEventRemoveHandle(monitor->clientWatch); - monitor->clientWatch = -1; + VIR_FORCE_CLOSE(ctrl->monitorClientFd); + virEventRemoveHandle(ctrl->monitorClientWatch); + ctrl->monitorClientWatch = -1; } } -static void lxcServerAccept(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRIBUTE_UNUSED, void *opaque) +static void virLXCControllerServerAccept(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRIBUTE_UNUSED, void *opaque) { - struct lxcMonitor *monitor = opaque; + virLXCControllerPtr ctrl = opaque; int client; if ((client = accept(fd, NULL, NULL)) < 0) { @@ -1034,16 +1043,16 @@ static void lxcServerAccept(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRI virMutexUnlock(&lock); return; } - VIR_DEBUG("New client %d (old %d)\n", client, monitor->clientFd); - VIR_FORCE_CLOSE(monitor->clientFd); - virEventRemoveHandle(monitor->clientWatch); + VIR_DEBUG("New client %d (old %d)\n", client, ctrl->monitorClientFd); + VIR_FORCE_CLOSE(ctrl->monitorClientFd); + virEventRemoveHandle(ctrl->monitorClientWatch); - monitor->clientFd = client; - if ((monitor->clientWatch = virEventAddHandle(monitor->clientFd, - VIR_EVENT_HANDLE_READABLE, - lxcClientIO, - monitor, - NULL)) < 0) { + ctrl->monitorClientFd = client; + if ((ctrl->monitorClientWatch = virEventAddHandle(ctrl->monitorClientFd, + VIR_EVENT_HANDLE_READABLE, + virLXCControllerClientIO, + ctrl, + NULL)) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch client socket")); virMutexLock(&lock); @@ -1305,14 +1314,8 @@ error: * * Returns 0 on success or -1 in case of error */ -static int virLXCControllerMain(virLXCControllerPtr ctrl, - int serverFd, - int clientFd) +static int virLXCControllerMain(virLXCControllerPtr ctrl) { - struct lxcMonitor monitor = { - .serverFd = serverFd, - .clientFd = clientFd, - }; virErrorPtr err; int rc = -1; size_t i; @@ -1343,25 +1346,25 @@ static int virLXCControllerMain(virLXCControllerPtr ctrl, } VIR_DEBUG("serverFd=%d clientFd=%d", - serverFd, clientFd); + ctrl->monitorServerFd, ctrl->monitorClientFd); virResetLastError(); - if ((monitor.serverWatch = virEventAddHandle(monitor.serverFd, - VIR_EVENT_HANDLE_READABLE, - lxcServerAccept, - &monitor, - NULL)) < 0) { + if ((ctrl->monitorServerWatch = virEventAddHandle(ctrl->monitorServerFd, + VIR_EVENT_HANDLE_READABLE, + virLXCControllerServerAccept, + ctrl, + NULL)) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch monitor socket")); goto cleanup; } - if (monitor.clientFd != -1 && - (monitor.clientWatch = virEventAddHandle(monitor.clientFd, - VIR_EVENT_HANDLE_READABLE, - lxcClientIO, - &monitor, - NULL)) < 0) { + if (ctrl->monitorClientFd != -1 && + (ctrl->monitorClientWatch = virEventAddHandle(ctrl->monitorClientFd, + VIR_EVENT_HANDLE_READABLE, + virLXCControllerClientIO, + ctrl, + NULL)) < 0) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to watch client socket")); goto cleanup; @@ -1422,8 +1425,6 @@ cleanup: virMutexDestroy(&lock); signal(SIGCHLD, SIG_DFL); cleanup2: - VIR_FORCE_CLOSE(monitor.serverFd); - VIR_FORCE_CLOSE(monitor.clientFd); for (i = 0 ; i < ctrl->nconsoles ; i++) virLXCControllerConsoleClose(&(ctrl->consoles[i])); @@ -1687,9 +1688,7 @@ virLXCControllerSetupConsoles(virLXCControllerPtr ctrl, static int -virLXCControllerRun(virLXCControllerPtr ctrl, - int monitor, - int client) +virLXCControllerRun(virLXCControllerPtr ctrl) { int rc = -1; int control[2] = { -1, -1}; @@ -1769,8 +1768,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl, if (virLXCControllerDaemonHandshake(ctrl) < 0) goto cleanup; - if (virSetBlocking(monitor, false) < 0 || - virSetBlocking(client, false) < 0) { + if (virSetBlocking(ctrl->monitorServerFd, false) < 0 || + virSetBlocking(ctrl->monitorClientFd, false) < 0) { virReportSystemError(errno, "%s", _("Unable to set file descriptor non-blocking")); goto cleanup; @@ -1779,8 +1778,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl, if (virLXCControllerConsoleSetNonblocking(&(ctrl->consoles[i])) < 0) goto cleanup; - rc = virLXCControllerMain(ctrl, monitor, client); - monitor = client = -1; + rc = virLXCControllerMain(ctrl); cleanup: VIR_FORCE_CLOSE(control[0]); @@ -1804,11 +1802,9 @@ int main(int argc, char *argv[]) { pid_t pid; int rc = 1; - int client; char *name = NULL; size_t nveths = 0; char **veths = NULL; - int monitor = -1; int handshakeFd = -1; int bg = 0; char *sockpath = NULL; @@ -1965,7 +1961,7 @@ int main(int argc, char *argv[]) if ((sockpath = lxcMonitorPath(ctrl)) == NULL) goto cleanup; - if ((monitor = lxcMonitorServer(sockpath)) < 0) + if ((ctrl->monitorServerFd = lxcMonitorServer(sockpath)) < 0) goto cleanup; if (bg) { @@ -2001,14 +1997,13 @@ int main(int argc, char *argv[]) } /* Accept initial client which is the libvirtd daemon */ - if ((client = accept(monitor, NULL, 0)) < 0) { + if ((ctrl->monitorClientFd = accept(ctrl->monitorServerFd, NULL, 0)) < 0) { virReportSystemError(errno, "%s", _("Failed to accept a connection from driver")); goto cleanup; } - rc = virLXCControllerRun(ctrl, - monitor, client); + rc = virLXCControllerRun(ctrl); cleanup: virPidFileDelete(LXC_STATE_DIR, name); -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:51 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Move the monitor FDs into the virLXCControllerPtr object removing the need for the 'struct lxcMonitor' object
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 115 ++++++++++++++++++++++------------------------ 1 file changed, 55 insertions(+), 60 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> Move the cgroup object into virLXCControllerPtr and rename all the setup methods to include 'Cgroup' in their name if appropriate Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 132 +++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 65 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index fc350ea..05ca52a 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -130,6 +130,8 @@ struct _virLXCController { int monitorServerWatch; int monitorClientFd; int monitorClientWatch; + + virCgroupPtr cgroup; }; static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -207,6 +209,8 @@ static void virLXCControllerStopInit(virLXCControllerPtr ctrl) virLXCControllerCloseLoopDevices(ctrl, true); virPidAbort(ctrl->initpid); ctrl->initpid = 0; + + virCgroupFree(&ctrl->cgroup); } @@ -487,7 +491,7 @@ cleanup: } #if HAVE_NUMACTL -static int lxcSetContainerNUMAPolicy(virDomainDefPtr def) +static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl) { nodemask_t mask; int mode = -1; @@ -497,7 +501,7 @@ static int lxcSetContainerNUMAPolicy(virDomainDefPtr def) int maxnode = 0; bool warned = false; - if (!def->numatune.memory.nodemask) + if (!ctrl->def->numatune.memory.nodemask) return 0; VIR_DEBUG("Setting NUMA memory policy"); @@ -513,7 +517,7 @@ static int lxcSetContainerNUMAPolicy(virDomainDefPtr def) /* Convert nodemask to NUMA bitmask. */ nodemask_zero(&mask); for (i = 0; i < VIR_DOMAIN_CPUMASK_LEN; i++) { - if (def->numatune.memory.nodemask[i]) { + if (ctrl->def->numatune.memory.nodemask[i]) { if (i > NUMA_NUM_NODES) { lxcError(VIR_ERR_CONFIG_UNSUPPORTED, _("Host cannot support NUMA node %d"), i); @@ -528,7 +532,7 @@ static int lxcSetContainerNUMAPolicy(virDomainDefPtr def) } } - mode = def->numatune.memory.mode; + mode = ctrl->def->numatune.memory.mode; if (mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT) { numa_set_bind_policy(1); @@ -567,9 +571,9 @@ cleanup: return ret; } #else -static int lxcSetContainerNUMAPolicy(virDomainDefPtr def) +static int virLXCControllerSetupNUMAPolicy(virLXCControllerPtr ctrl) { - if (def->numatune.memory.nodemask) { + if (ctrl->def->numatune.memory.nodemask) { lxcError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("NUMA policy is not available on this platform")); return -1; @@ -583,7 +587,7 @@ static int lxcSetContainerNUMAPolicy(virDomainDefPtr def) /* * To be run while still single threaded */ -static int lxcSetContainerCpuAffinity(virDomainDefPtr def) +static int virLXCControllerSetupCpuAffinity(virLXCControllerPtr ctrl) { int i, hostcpus, maxcpu = CPU_SETSIZE; virNodeInfo nodeinfo; @@ -607,11 +611,11 @@ static int lxcSetContainerCpuAffinity(virDomainDefPtr def) return -1; } - if (def->cpumask) { + if (ctrl->def->cpumask) { /* XXX why don't we keep 'cpumask' in the libvirt cpumap * format to start with ?!?! */ - for (i = 0 ; i < maxcpu && i < def->cpumasklen ; i++) - if (def->cpumask[i]) + for (i = 0 ; i < maxcpu && i < ctrl->def->cpumasklen ; i++) + if (ctrl->def->cpumask[i]) VIR_USE_CPU(cpumap, i); } else { /* You may think this is redundant, but we can't assume libvirtd @@ -637,33 +641,33 @@ static int lxcSetContainerCpuAffinity(virDomainDefPtr def) } -static int lxcSetContainerCpuTune(virCgroupPtr cgroup, virDomainDefPtr def) +static int virLXCControllerSetupCgroupsCpuTune(virLXCControllerPtr ctrl) { int ret = -1; - if (def->cputune.shares != 0) { - int rc = virCgroupSetCpuShares(cgroup, def->cputune.shares); + if (ctrl->def->cputune.shares != 0) { + int rc = virCgroupSetCpuShares(ctrl->cgroup, ctrl->def->cputune.shares); if (rc != 0) { virReportSystemError(-rc, _("Unable to set io cpu shares for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } - if (def->cputune.quota != 0) { - int rc = virCgroupSetCpuCfsQuota(cgroup, def->cputune.quota); + if (ctrl->def->cputune.quota != 0) { + int rc = virCgroupSetCpuCfsQuota(ctrl->cgroup, ctrl->def->cputune.quota); if (rc != 0) { virReportSystemError(-rc, _("Unable to set io cpu quota for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } - if (def->cputune.period != 0) { - int rc = virCgroupSetCpuCfsPeriod(cgroup, def->cputune.period); + if (ctrl->def->cputune.period != 0) { + int rc = virCgroupSetCpuCfsPeriod(ctrl->cgroup, ctrl->def->cputune.period); if (rc != 0) { virReportSystemError(-rc, _("Unable to set io cpu period for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } @@ -673,16 +677,16 @@ cleanup: } -static int lxcSetContainerBlkioTune(virCgroupPtr cgroup, virDomainDefPtr def) +static int virLXCControllerSetupCgroupsBlkioTune(virLXCControllerPtr ctrl) { int ret = -1; - if (def->blkio.weight) { - int rc = virCgroupSetBlkioWeight(cgroup, def->blkio.weight); + if (ctrl->def->blkio.weight) { + int rc = virCgroupSetBlkioWeight(ctrl->cgroup, ctrl->def->blkio.weight); if (rc != 0) { virReportSystemError(-rc, _("Unable to set Blkio weight for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } @@ -693,45 +697,45 @@ cleanup: } -static int lxcSetContainerMemTune(virCgroupPtr cgroup, virDomainDefPtr def) +static int virLXCControllerSetupCgroupsMemTune(virLXCControllerPtr ctrl) { int ret = -1; int rc; - rc = virCgroupSetMemory(cgroup, def->mem.max_balloon); + rc = virCgroupSetMemory(ctrl->cgroup, ctrl->def->mem.max_balloon); if (rc != 0) { virReportSystemError(-rc, _("Unable to set memory limit for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } - if (def->mem.hard_limit) { - rc = virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit); + if (ctrl->def->mem.hard_limit) { + rc = virCgroupSetMemoryHardLimit(ctrl->cgroup, ctrl->def->mem.hard_limit); if (rc != 0) { virReportSystemError(-rc, _("Unable to set memory hard limit for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } - if (def->mem.soft_limit) { - rc = virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit); + if (ctrl->def->mem.soft_limit) { + rc = virCgroupSetMemorySoftLimit(ctrl->cgroup, ctrl->def->mem.soft_limit); if (rc != 0) { virReportSystemError(-rc, _("Unable to set memory soft limit for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } - if (def->mem.swap_hard_limit) { - rc = virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit); + if (ctrl->def->mem.swap_hard_limit) { + rc = virCgroupSetMemSwapHardLimit(ctrl->cgroup, ctrl->def->mem.swap_hard_limit); if (rc != 0) { virReportSystemError(-rc, _("Unable to set swap hard limit for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } } @@ -742,7 +746,7 @@ cleanup: } -static int lxcSetContainerDeviceACL(virCgroupPtr cgroup, virDomainDefPtr def) +static int virLXCControllerSetupCgroupsDeviceACL(virLXCControllerPtr ctrl) { int ret = -1; int rc; @@ -757,17 +761,17 @@ static int lxcSetContainerDeviceACL(virCgroupPtr cgroup, virDomainDefPtr def) {'c', LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX}, {0, 0, 0}}; - rc = virCgroupDenyAllDevices(cgroup); + rc = virCgroupDenyAllDevices(ctrl->cgroup); if (rc != 0) { virReportSystemError(-rc, _("Unable to deny devices for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } for (i = 0; devices[i].type != 0; i++) { const struct cgroup_device_policy *dev = &devices[i]; - rc = virCgroupAllowDevice(cgroup, + rc = virCgroupAllowDevice(ctrl->cgroup, dev->type, dev->major, dev->minor, @@ -775,34 +779,34 @@ static int lxcSetContainerDeviceACL(virCgroupPtr cgroup, virDomainDefPtr def) if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %c:%d:%d for domain %s"), - dev->type, dev->major, dev->minor, def->name); + dev->type, dev->major, dev->minor, ctrl->def->name); goto cleanup; } } - for (i = 0 ; i < def->nfss ; i++) { - if (def->fss[i]->type != VIR_DOMAIN_FS_TYPE_BLOCK) + for (i = 0 ; i < ctrl->def->nfss ; i++) { + if (ctrl->def->fss[i]->type != VIR_DOMAIN_FS_TYPE_BLOCK) continue; - rc = virCgroupAllowDevicePath(cgroup, - def->fss[i]->src, - def->fss[i]->readonly ? + rc = virCgroupAllowDevicePath(ctrl->cgroup, + ctrl->def->fss[i]->src, + ctrl->def->fss[i]->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow device %s for domain %s"), - def->fss[i]->src, def->name); + ctrl->def->fss[i]->src, ctrl->def->name); goto cleanup; } } - rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY, + rc = virCgroupAllowDeviceMajor(ctrl->cgroup, 'c', LXC_DEV_MAJ_PTY, VIR_CGROUP_DEVICE_RWM); if (rc != 0) { virReportSystemError(-rc, _("Unable to allow PTY devices for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } @@ -813,24 +817,23 @@ cleanup: /** - * lxcSetContainerResources - * @def: pointer to virtual machine structure + * virLXCControllerSetupResourceLimits + * @ctrl: the controller state * * Creates a cgroup for the container, moves the task inside, * and sets resource limits * * Returns 0 on success or -1 in case of error */ -static int lxcSetContainerResources(virDomainDefPtr def) +static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) { virCgroupPtr driver; - virCgroupPtr cgroup; int rc = -1; - if (lxcSetContainerCpuAffinity(def) < 0) + if (virLXCControllerSetupCpuAffinity(ctrl) < 0) return -1; - if (lxcSetContainerNUMAPolicy(def) < 0) + if (virLXCControllerSetupNUMAPolicy(ctrl) < 0) return -1; rc = virCgroupForDriver("lxc", &driver, 1, 0); @@ -844,36 +847,35 @@ static int lxcSetContainerResources(virDomainDefPtr def) return rc; } - rc = virCgroupForDomain(driver, def->name, &cgroup, 1); + rc = virCgroupForDomain(driver, ctrl->def->name, &ctrl->cgroup, 1); if (rc != 0) { virReportSystemError(-rc, _("Unable to create cgroup for domain %s"), - def->name); + ctrl->def->name); goto cleanup; } - if (lxcSetContainerCpuTune(cgroup, def) < 0) + if (virLXCControllerSetupCgroupsCpuTune(ctrl) < 0) goto cleanup; - if (lxcSetContainerBlkioTune(cgroup, def) < 0) + if (virLXCControllerSetupCgroupsBlkioTune(ctrl) < 0) goto cleanup; - if (lxcSetContainerMemTune(cgroup, def) < 0) + if (virLXCControllerSetupCgroupsMemTune(ctrl) < 0) goto cleanup; - if (lxcSetContainerDeviceACL(cgroup, def) < 0) + if (virLXCControllerSetupCgroupsDeviceACL(ctrl) < 0) goto cleanup; - rc = virCgroupAddTask(cgroup, getpid()); + rc = virCgroupAddTask(ctrl->cgroup, getpid()); if (rc != 0) { virReportSystemError(-rc, _("Unable to add task %d to cgroup for domain %s"), - getpid(), def->name); + getpid(), ctrl->def->name); } cleanup: virCgroupFree(&driver); - virCgroupFree(&cgroup); return rc; } @@ -1716,7 +1718,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl) if (virLXCControllerSetupLoopDevices(ctrl) < 0) goto cleanup; - if (lxcSetContainerResources(ctrl->def) < 0) + if (virLXCControllerSetupResourceLimits(ctrl) < 0) goto cleanup; if (virLXCControllerSetupDevPTS(ctrl) < 0) -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:52 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Move the cgroup object into virLXCControllerPtr and rename all the setup methods to include 'Cgroup' in their name if appropriate
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_controller.c | 132 +++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 65 deletions(-)
ACK Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> While it is not currently used elsewhere in libvirt, the code for finding a free loop device & associating a file with it is not LXC specific. Move it into the viffile.{c,h} file where potentially shared code is more commonly kept. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 112 ++----------------------------------- src/util/virfile.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 + 4 files changed, 147 insertions(+), 107 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2fe5068..6625fc6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1243,6 +1243,7 @@ virDomainListSnapshots; # virfile.h +virFileLoopDeviceAssociate; virFileClose; virFileDirectFdFlag; virFileWrapperFdClose; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 05ca52a..27b51b4 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -39,8 +39,6 @@ #include <getopt.h> #include <sys/mount.h> #include <locale.h> -#include <linux/loop.h> -#include <dirent.h> #include <grp.h> #include <sys/stat.h> #include <time.h> @@ -337,108 +335,15 @@ static int virLXCControllerValidateConsoles(virLXCControllerPtr ctrl) } -static int lxcGetLoopFD(char **dev_name) -{ - int fd = -1; - DIR *dh = NULL; - struct dirent *de; - char *looppath; - struct loop_info64 lo; - - VIR_DEBUG("Looking for loop devices in /dev"); - - if (!(dh = opendir("/dev"))) { - virReportSystemError(errno, "%s", - _("Unable to read /dev")); - goto cleanup; - } - - while ((de = readdir(dh)) != NULL) { - if (!STRPREFIX(de->d_name, "loop")) - continue; - - if (virAsprintf(&looppath, "/dev/%s", de->d_name) < 0) { - virReportOOMError(); - goto cleanup; - } - - VIR_DEBUG("Checking up on device %s", looppath); - if ((fd = open(looppath, O_RDWR)) < 0) { - virReportSystemError(errno, - _("Unable to open %s"), looppath); - goto cleanup; - } - - if (ioctl(fd, LOOP_GET_STATUS64, &lo) < 0) { - /* Got a free device, return the fd */ - if (errno == ENXIO) - goto cleanup; - - VIR_FORCE_CLOSE(fd); - virReportSystemError(errno, - _("Unable to get loop status on %s"), - looppath); - goto cleanup; - } - - /* Oh well, try the next device */ - VIR_FORCE_CLOSE(fd); - VIR_FREE(looppath); - } - - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to find a free loop device in /dev")); - -cleanup: - if (fd != -1) { - VIR_DEBUG("Got free loop device %s %d", looppath, fd); - *dev_name = looppath; - } else { - VIR_DEBUG("No free loop devices available"); - VIR_FREE(looppath); - } - if (dh) - closedir(dh); - return fd; -} - -static int lxcSetupLoopDevice(virDomainFSDefPtr fs) +static int virLXCControllerSetupLoopDevice(virDomainFSDefPtr fs) { int lofd = -1; - int fsfd = -1; - struct loop_info64 lo; char *loname = NULL; - int ret = -1; - if ((lofd = lxcGetLoopFD(&loname)) < 0) - return -1; - memset(&lo, 0, sizeof(lo)); - lo.lo_flags = LO_FLAGS_AUTOCLEAR; - - if ((fsfd = open(fs->src, O_RDWR)) < 0) { - virReportSystemError(errno, - _("Unable to open %s"), fs->src); - goto cleanup; - } - - if (ioctl(lofd, LOOP_SET_FD, fsfd) < 0) { - virReportSystemError(errno, - _("Unable to attach %s to loop device"), - fs->src); - goto cleanup; - } - - if (ioctl(lofd, LOOP_SET_STATUS64, &lo) < 0) { - virReportSystemError(errno, "%s", - _("Unable to mark loop device as autoclear")); - - if (ioctl(lofd, LOOP_CLR_FD, 0) < 0) - VIR_WARN("Unable to detach %s from loop device", fs->src); - goto cleanup; - } + if ((lofd = virFileLoopDeviceAssociate(fs->src, &loname)) < 0) + return -1; - VIR_DEBUG("Attached loop device %s %d to %s", fs->src, lofd, loname); /* * We now change it into a block device type, so that * the rest of container setup 'just works' @@ -448,14 +353,7 @@ static int lxcSetupLoopDevice(virDomainFSDefPtr fs) fs->src = loname; loname = NULL; - ret = 0; - -cleanup: - VIR_FREE(loname); - VIR_FORCE_CLOSE(fsfd); - if (ret == -1) - VIR_FORCE_CLOSE(lofd); - return lofd; + return 0; } @@ -470,7 +368,7 @@ static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl) if (ctrl->def->fss[i]->type != VIR_DOMAIN_FS_TYPE_FILE) continue; - fd = lxcSetupLoopDevice(ctrl->def->fss[i]); + fd = virLXCControllerSetupLoopDevice(ctrl->def->fss[i]); if (fd < 0) goto cleanup; diff --git a/src/util/virfile.c b/src/util/virfile.c index 6c69217..8387ae9 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -30,6 +30,12 @@ #include <fcntl.h> #include <sys/stat.h> #include <unistd.h> +#include <dirent.h> + +#ifdef __linux__ +# include <linux/loop.h> +# include <sys/ioctl.h> +#endif #include "command.h" #include "configmake.h" @@ -493,3 +499,135 @@ int virFileUpdatePerm(const char *path, return 0; } + + +#ifdef __linux__ +static int virFileLoopDeviceOpen(char **dev_name) +{ + int fd = -1; + DIR *dh = NULL; + struct dirent *de; + char *looppath; + struct loop_info64 lo; + + VIR_DEBUG("Looking for loop devices in /dev"); + + if (!(dh = opendir("/dev"))) { + virReportSystemError(errno, "%s", + _("Unable to read /dev")); + goto cleanup; + } + + while ((de = readdir(dh)) != NULL) { + if (!STRPREFIX(de->d_name, "loop")) + continue; + + if (virAsprintf(&looppath, "/dev/%s", de->d_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + VIR_DEBUG("Checking up on device %s", looppath); + if ((fd = open(looppath, O_RDWR)) < 0) { + virReportSystemError(errno, + _("Unable to open %s"), looppath); + goto cleanup; + } + + if (ioctl(fd, LOOP_GET_STATUS64, &lo) < 0) { + /* Got a free device, return the fd */ + if (errno == ENXIO) + goto cleanup; + + VIR_FORCE_CLOSE(fd); + virReportSystemError(errno, + _("Unable to get loop status on %s"), + looppath); + goto cleanup; + } + + /* Oh well, try the next device */ + VIR_FORCE_CLOSE(fd); + VIR_FREE(looppath); + } + + virFileError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to find a free loop device in /dev")); + +cleanup: + if (fd != -1) { + VIR_DEBUG("Got free loop device %s %d", looppath, fd); + *dev_name = looppath; + } else { + VIR_DEBUG("No free loop devices available"); + VIR_FREE(looppath); + } + if (dh) + closedir(dh); + return fd; +} + + +int virFileLoopDeviceAssociate(const char *file, + char **dev) +{ + int lofd = -1; + int fsfd = -1; + struct loop_info64 lo; + char *loname = NULL; + int ret = -1; + + if ((lofd = virFileLoopDeviceOpen(&loname)) < 0) + return -1; + + memset(&lo, 0, sizeof(lo)); + lo.lo_flags = LO_FLAGS_AUTOCLEAR; + + if ((fsfd = open(file, O_RDWR)) < 0) { + virReportSystemError(errno, + _("Unable to open %s"), file); + goto cleanup; + } + + if (ioctl(lofd, LOOP_SET_FD, fsfd) < 0) { + virReportSystemError(errno, + _("Unable to attach %s to loop device"), + file); + goto cleanup; + } + + if (ioctl(lofd, LOOP_SET_STATUS64, &lo) < 0) { + virReportSystemError(errno, "%s", + _("Unable to mark loop device as autoclear")); + + if (ioctl(lofd, LOOP_CLR_FD, 0) < 0) + VIR_WARN("Unable to detach %s from loop device", file); + goto cleanup; + } + + VIR_DEBUG("Attached loop device %s %d to %s", file, lofd, loname); + *dev = loname; + loname = NULL; + + ret = 0; + +cleanup: + VIR_FREE(loname); + VIR_FORCE_CLOSE(fsfd); + if (ret == -1) + VIR_FORCE_CLOSE(lofd); + return lofd; +} + +#else /* __linux__ */ + +int virFileLoopDeviceAssociate(const char *file, + char **dev) +{ + virReportSystemError(ENOSYS, + _("Unable to associate file %s with loop device"), + file); + return -1;m +} + +#endif /* __linux__ */ diff --git a/src/util/virfile.h b/src/util/virfile.h index fb8109b..101953a 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -105,4 +105,7 @@ int virFileUpdatePerm(const char *path, mode_t mode_remove, mode_t mode_add); +int virFileLoopDeviceAssociate(const char *file, + char **dev); + #endif /* __VIR_FILES_H */ -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:53 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
While it is not currently used elsewhere in libvirt, the code for finding a free loop device & associating a file with it is not LXC specific. Move it into the viffile.{c,h} file where potentially shared code is more commonly kept.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 1 + src/lxc/lxc_controller.c | 112 ++----------------------------------- src/util/virfile.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 + 4 files changed, 147 insertions(+), 107 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2fe5068..6625fc6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1243,6 +1243,7 @@ virDomainListSnapshots;
# virfile.h +virFileLoopDeviceAssociate; virFileClose; virFileDirectFdFlag; virFileWrapperFdClose; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 05ca52a..27b51b4 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -39,8 +39,6 @@ #include <getopt.h> #include <sys/mount.h> #include <locale.h> -#include <linux/loop.h> -#include <dirent.h> #include <grp.h> #include <sys/stat.h> #include <time.h> @@ -337,108 +335,15 @@ static int virLXCControllerValidateConsoles(virLXCControllerPtr ctrl) }
-static int lxcGetLoopFD(char **dev_name) -{ - int fd = -1; - DIR *dh = NULL; - struct dirent *de; - char *looppath; - struct loop_info64 lo; - - VIR_DEBUG("Looking for loop devices in /dev"); - - if (!(dh = opendir("/dev"))) { - virReportSystemError(errno, "%s", - _("Unable to read /dev")); - goto cleanup; - } - - while ((de = readdir(dh)) != NULL) { - if (!STRPREFIX(de->d_name, "loop")) - continue; - - if (virAsprintf(&looppath, "/dev/%s", de->d_name) < 0) { - virReportOOMError(); - goto cleanup; - } - - VIR_DEBUG("Checking up on device %s", looppath); - if ((fd = open(looppath, O_RDWR)) < 0) { - virReportSystemError(errno, - _("Unable to open %s"), looppath); - goto cleanup; - } - - if (ioctl(fd, LOOP_GET_STATUS64, &lo) < 0) { - /* Got a free device, return the fd */ - if (errno == ENXIO) - goto cleanup; - - VIR_FORCE_CLOSE(fd); - virReportSystemError(errno, - _("Unable to get loop status on %s"), - looppath); - goto cleanup; - } - - /* Oh well, try the next device */ - VIR_FORCE_CLOSE(fd); - VIR_FREE(looppath); - } - - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to find a free loop device in /dev")); - -cleanup: - if (fd != -1) { - VIR_DEBUG("Got free loop device %s %d", looppath, fd); - *dev_name = looppath; - } else { - VIR_DEBUG("No free loop devices available"); - VIR_FREE(looppath); - } - if (dh) - closedir(dh); - return fd; -} - -static int lxcSetupLoopDevice(virDomainFSDefPtr fs) +static int virLXCControllerSetupLoopDevice(virDomainFSDefPtr fs) { int lofd = -1;
This lofd = -1 initialization is no longer needed.
- int fsfd = -1; - struct loop_info64 lo; char *loname = NULL; - int ret = -1;
- if ((lofd = lxcGetLoopFD(&loname)) < 0) - return -1;
- memset(&lo, 0, sizeof(lo)); - lo.lo_flags = LO_FLAGS_AUTOCLEAR; - - if ((fsfd = open(fs->src, O_RDWR)) < 0) { - virReportSystemError(errno, - _("Unable to open %s"), fs->src); - goto cleanup; - } - - if (ioctl(lofd, LOOP_SET_FD, fsfd) < 0) { - virReportSystemError(errno, - _("Unable to attach %s to loop device"), - fs->src); - goto cleanup; - } - - if (ioctl(lofd, LOOP_SET_STATUS64, &lo) < 0) { - virReportSystemError(errno, "%s", - _("Unable to mark loop device as autoclear")); - - if (ioctl(lofd, LOOP_CLR_FD, 0) < 0) - VIR_WARN("Unable to detach %s from loop device", fs->src); - goto cleanup; - } + if ((lofd = virFileLoopDeviceAssociate(fs->src, &loname)) < 0) + return -1;
- VIR_DEBUG("Attached loop device %s %d to %s", fs->src, lofd, loname); /* * We now change it into a block device type, so that * the rest of container setup 'just works' @@ -448,14 +353,7 @@ static int lxcSetupLoopDevice(virDomainFSDefPtr fs) fs->src = loname; loname = NULL;
- ret = 0; - -cleanup: - VIR_FREE(loname); - VIR_FORCE_CLOSE(fsfd); - if (ret == -1) - VIR_FORCE_CLOSE(lofd); - return lofd; + return 0; }
Oops, you actually want to return lofd rather than zero.
@@ -470,7 +368,7 @@ static int virLXCControllerSetupLoopDevices(virLXCControllerPtr ctrl)
... ACK with the fix squashed in. Jirka

From: "Daniel P. Berrange" <berrange@redhat.com> In preparation for introducing a full RPC protocol for libvirt_lxc, switch over to using the virNetServer APIs for the monitor connection Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 2 + src/lxc/lxc_controller.c | 268 +++++++++------------------------------------- 2 files changed, 54 insertions(+), 216 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index eeeda1c..6c3eaa7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1523,6 +1523,8 @@ libvirt_lxc_SOURCES = \ libvirt_lxc_LDFLAGS = $(WARN_CFLAGS) $(AM_LDFLAGS) libvirt_lxc_LDADD = \ $(NUMACTL_LIBS) \ + libvirt-net-rpc-server.la \ + libvirt-net-rpc.la \ libvirt_driver_security.la \ libvirt_conf.la \ libvirt_util.la \ diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 27b51b4..ed43aad 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -68,6 +68,7 @@ #include "processinfo.h" #include "nodeinfo.h" #include "virrandom.h" +#include "rpc/virnetserver.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -124,10 +125,8 @@ struct _virLXCController { virSecurityManagerPtr securityManager; - int monitorServerFd; - int monitorServerWatch; - int monitorClientFd; - int monitorClientWatch; + /* Server socket */ + virNetServerPtr server; virCgroupPtr cgroup; }; @@ -143,11 +142,6 @@ static virLXCControllerPtr virLXCControllerNew(const char *name) if (VIR_ALLOC(ctrl) < 0) goto no_memory; - ctrl->monitorServerFd = -1; - ctrl->monitorServerWatch = -1; - ctrl->monitorClientFd = -1; - ctrl->monitorClientWatch = -1; - if (!(ctrl->name = strdup(name))) goto no_memory; @@ -254,12 +248,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) virDomainDefFree(ctrl->def); VIR_FREE(ctrl->name); - if (ctrl->monitorServerWatch != -1) - virEventRemoveHandle(ctrl->monitorServerWatch); - VIR_FORCE_CLOSE(ctrl->monitorServerFd); - if (ctrl->monitorClientWatch != -1) - virEventRemoveHandle(ctrl->monitorClientWatch); - VIR_FORCE_CLOSE(ctrl->monitorClientFd); + virNetServerFree(ctrl->server); VIR_FREE(ctrl); } @@ -778,54 +767,58 @@ cleanup: return rc; } -static char*lxcMonitorPath(virLXCControllerPtr ctrl) -{ - char *sockpath; - if (virAsprintf(&sockpath, "%s/%s.sock", - LXC_STATE_DIR, ctrl->def->name) < 0) - virReportOOMError(); - return sockpath; +static int virLXCControllerClientHook(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + void *opaque) +{ + virLXCControllerPtr ctrl = opaque; + virNetServerClientSetPrivateData(client, ctrl, NULL); + return 0; } -static int lxcMonitorServer(const char *sockpath) + +static int virLXCControllerSetupServer(virLXCControllerPtr ctrl) { - int fd; - struct sockaddr_un addr; + virNetServerServicePtr svc = NULL; + char *sockpath; - if ((fd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) { - virReportSystemError(errno, - _("failed to create server socket '%s'"), - sockpath); - goto error; + if (virAsprintf(&sockpath, "%s/%s.sock", + LXC_STATE_DIR, ctrl->name) < 0) { + virReportOOMError(); + return -1; } - unlink(sockpath); - memset(&addr, 0, sizeof(addr)); - addr.sun_family = AF_UNIX; - if (virStrcpyStatic(addr.sun_path, sockpath) == NULL) { - lxcError(VIR_ERR_INTERNAL_ERROR, - _("Socket path %s too long for destination"), sockpath); + if (!(ctrl->server = virNetServerNew(0, 0, 0, 1, + -1, 0, false, + NULL, + virLXCControllerClientHook, + ctrl))) goto error; - } - if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { - virReportSystemError(errno, - _("failed to bind server socket '%s'"), - sockpath); + if (!(svc = virNetServerServiceNewUNIX(sockpath, + 0700, + 0, + 0, + false, + 5, + NULL))) goto error; - } - if (listen(fd, 30 /* backlog */ ) < 0) { - virReportSystemError(errno, - _("failed to listen server socket %s"), - sockpath); + + if (virNetServerAddService(ctrl->server, svc, NULL) < 0) goto error; - } + virNetServerServiceFree(svc); + svc = NULL; - return fd; + virNetServerUpdateServices(ctrl->server, true); + VIR_FREE(sockpath); + return 0; error: - VIR_FORCE_CLOSE(fd); + VIR_FREE(sockpath); + virNetServerFree(ctrl->server); + ctrl->server = NULL; + virNetServerServiceFree(svc); return -1; } @@ -848,120 +841,23 @@ static int lxcControllerClearCapabilities(void) return 0; } -/* Return true if it is ok to ignore an accept-after-epoll syscall - that fails with the specified errno value. Else false. */ -static bool -ignorable_accept_errno(int errnum) -{ - return (errnum == EINVAL - || errnum == ECONNABORTED - || errnum == EAGAIN - || errnum == EWOULDBLOCK); -} - static bool quit = false; static virMutex lock; -static int sigpipe[2]; -static void virLXCControllerSignalChildHandler(int signum ATTRIBUTE_UNUSED) -{ - ignore_value(write(sigpipe[1], "1", 1)); -} -static void virLXCControllerSignalChildIO(int watch ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED, - int events ATTRIBUTE_UNUSED, +static void virLXCControllerSignalChildIO(virNetServerPtr server ATTRIBUTE_UNUSED, + siginfo_t *info ATTRIBUTE_UNUSED, void *opaque) { - char buf[1]; - int ret; virLXCControllerPtr ctrl = opaque; + int ret; - ignore_value(read(sigpipe[0], buf, 1)); ret = waitpid(-1, NULL, WNOHANG); - if (ret == ctrl->initpid) { - virMutexLock(&lock); - quit = true; - virMutexUnlock(&lock); - } -} - - -static void virLXCControllerClientIO(int watch ATTRIBUTE_UNUSED, int fd, int events, void *opaque) -{ - virLXCControllerPtr ctrl = opaque; - char buf[1024]; - ssize_t ret; - - if (events & (VIR_EVENT_HANDLE_HANGUP | - VIR_EVENT_HANDLE_ERROR)) { - virEventRemoveHandle(ctrl->monitorClientWatch); - ctrl->monitorClientWatch = -1; - return; - } - -reread: - ret = read(fd, buf, sizeof(buf)); - if (ret == -1 && errno == EINTR) - goto reread; - if (ret == -1 && errno == EAGAIN) - return; - if (ret == -1) { - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to read from monitor client")); - virMutexLock(&lock); - quit = true; - virMutexUnlock(&lock); - return; - } - if (ret == 0) { - VIR_DEBUG("Client %d gone", fd); - VIR_FORCE_CLOSE(ctrl->monitorClientFd); - virEventRemoveHandle(ctrl->monitorClientWatch); - ctrl->monitorClientWatch = -1; - } + if (ret == ctrl->initpid) + virNetServerQuit(ctrl->server); } -static void virLXCControllerServerAccept(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRIBUTE_UNUSED, void *opaque) -{ - virLXCControllerPtr ctrl = opaque; - int client; - - if ((client = accept(fd, NULL, NULL)) < 0) { - /* First reflex may be simply to declare accept failure - to be a fatal error. However, accept may fail when - a client quits between the above poll and here. - That case is not fatal, but rather to be expected, - if not common, so ignore it. */ - if (ignorable_accept_errno(errno)) - return; - virReportSystemError(errno, "%s", - _("Unable to accept monitor client")); - virMutexLock(&lock); - quit = true; - virMutexUnlock(&lock); - return; - } - VIR_DEBUG("New client %d (old %d)\n", client, ctrl->monitorClientFd); - VIR_FORCE_CLOSE(ctrl->monitorClientFd); - virEventRemoveHandle(ctrl->monitorClientWatch); - - ctrl->monitorClientFd = client; - if ((ctrl->monitorClientWatch = virEventAddHandle(ctrl->monitorClientFd, - VIR_EVENT_HANDLE_READABLE, - virLXCControllerClientIO, - ctrl, - NULL)) < 0) { - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to watch client socket")); - virMutexLock(&lock); - quit = true; - virMutexUnlock(&lock); - return; - } -} - static void virLXCControllerConsoleUpdateWatch(virLXCControllerConsolePtr console) { int hostEvents = 0; @@ -1223,53 +1119,14 @@ static int virLXCControllerMain(virLXCControllerPtr ctrl) if (virMutexInit(&lock) < 0) goto cleanup2; - if (pipe2(sigpipe, O_CLOEXEC|O_NONBLOCK) < 0) { - virReportSystemError(errno, "%s", - _("Cannot create signal pipe")); - goto cleanup; - } - - if (virEventAddHandle(sigpipe[0], - VIR_EVENT_HANDLE_READABLE, - virLXCControllerSignalChildIO, - ctrl, - NULL) < 0) { - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to watch signal pipe")); - goto cleanup; - } - - if (signal(SIGCHLD, virLXCControllerSignalChildHandler) == SIG_ERR) { - virReportSystemError(errno, "%s", - _("Cannot install signal handler")); + if (virNetServerAddSignalHandler(ctrl->server, + SIGCHLD, + virLXCControllerSignalChildIO, + ctrl) < 0) goto cleanup; - } - VIR_DEBUG("serverFd=%d clientFd=%d", - ctrl->monitorServerFd, ctrl->monitorClientFd); virResetLastError(); - if ((ctrl->monitorServerWatch = virEventAddHandle(ctrl->monitorServerFd, - VIR_EVENT_HANDLE_READABLE, - virLXCControllerServerAccept, - ctrl, - NULL)) < 0) { - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to watch monitor socket")); - goto cleanup; - } - - if (ctrl->monitorClientFd != -1 && - (ctrl->monitorClientWatch = virEventAddHandle(ctrl->monitorClientFd, - VIR_EVENT_HANDLE_READABLE, - virLXCControllerClientIO, - ctrl, - NULL)) < 0) { - lxcError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to watch client socket")); - goto cleanup; - } - for (i = 0 ; i < ctrl->nconsoles ; i++) { if ((ctrl->consoles[i].epollFd = epoll_create1(EPOLL_CLOEXEC)) < 0) { virReportSystemError(errno, "%s", @@ -1323,7 +1180,6 @@ static int virLXCControllerMain(virLXCControllerPtr ctrl) cleanup: virMutexDestroy(&lock); - signal(SIGCHLD, SIG_DFL); cleanup2: for (i = 0 ; i < ctrl->nconsoles ; i++) @@ -1668,12 +1524,6 @@ virLXCControllerRun(virLXCControllerPtr ctrl) if (virLXCControllerDaemonHandshake(ctrl) < 0) goto cleanup; - if (virSetBlocking(ctrl->monitorServerFd, false) < 0 || - virSetBlocking(ctrl->monitorClientFd, false) < 0) { - virReportSystemError(errno, "%s", - _("Unable to set file descriptor non-blocking")); - goto cleanup; - } for (i = 0 ; i < ctrl->nconsoles ; i++) if (virLXCControllerConsoleSetNonblocking(&(ctrl->consoles[i])) < 0) goto cleanup; @@ -1707,7 +1557,6 @@ int main(int argc, char *argv[]) char **veths = NULL; int handshakeFd = -1; int bg = 0; - char *sockpath = NULL; const struct option options[] = { { "background", 0, NULL, 'b' }, { "name", 1, NULL, 'n' }, @@ -1858,10 +1707,7 @@ int main(int argc, char *argv[]) if (virLXCControllerValidateConsoles(ctrl) < 0) goto cleanup; - if ((sockpath = lxcMonitorPath(ctrl)) == NULL) - goto cleanup; - - if ((ctrl->monitorServerFd = lxcMonitorServer(sockpath)) < 0) + if (virLXCControllerSetupServer(ctrl) < 0) goto cleanup; if (bg) { @@ -1896,21 +1742,11 @@ int main(int argc, char *argv[]) } } - /* Accept initial client which is the libvirtd daemon */ - if ((ctrl->monitorClientFd = accept(ctrl->monitorServerFd, NULL, 0)) < 0) { - virReportSystemError(errno, "%s", - _("Failed to accept a connection from driver")); - goto cleanup; - } - rc = virLXCControllerRun(ctrl); cleanup: virPidFileDelete(LXC_STATE_DIR, name); virLXCControllerDeleteInterfaces(ctrl); - if (sockpath) - unlink(sockpath); - VIR_FREE(sockpath); for (i = 0 ; i < nttyFDs ; i++) VIR_FORCE_CLOSE(ttyFDs[i]); VIR_FREE(ttyFDs); -- 1.7.10.4

On Tue, Jul 03, 2012 at 16:58:54 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
In preparation for introducing a full RPC protocol for libvirt_lxc, switch over to using the virNetServer APIs for the monitor connection
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 2 + src/lxc/lxc_controller.c | 268 +++++++++------------------------------------- 2 files changed, 54 insertions(+), 216 deletions(-)
Nice. This looks OK and I trust you actually tested it still works :-) ACK Jirka

On Wed, Jul 04, 2012 at 05:09:57PM +0200, Jiri Denemark wrote:
On Tue, Jul 03, 2012 at 16:58:54 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
In preparation for introducing a full RPC protocol for libvirt_lxc, switch over to using the virNetServer APIs for the monitor connection
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 2 + src/lxc/lxc_controller.c | 268 +++++++++------------------------------------- 2 files changed, 54 insertions(+), 216 deletions(-)
Nice. This looks OK and I trust you actually tested it still works :-)
Yep, but there's lots more changes coming which will need more testing still, so I'm not hugely worried at this stage. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

于 2012年07月03日 23:58, Daniel P. Berrange 写道:
From: "Daniel P. Berrange" <berrange@redhat.com>
In preparation for introducing a full RPC protocol for libvirt_lxc, switch over to using the virNetServer APIs for the monitor connection
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Hi Daniel With this patch [15/15] applied, I found the exit cmd failed in libvirt lxc. after I execute exit cmd in lxc,there has no response at all. when I revert this patch,it works well. Thanks Gao

On Mon, Jul 09, 2012 at 04:26:55PM +0800, Gao feng wrote:
于 2012年07月03日 23:58, Daniel P. Berrange 写道:
From: "Daniel P. Berrange" <berrange@redhat.com>
In preparation for introducing a full RPC protocol for libvirt_lxc, switch over to using the virNetServer APIs for the monitor connection
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Hi Daniel
With this patch [15/15] applied, I found the exit cmd failed in libvirt lxc. after I execute exit cmd in lxc,there has no response at all.
when I revert this patch,it works well.
Thanks I'll look into this. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (5)
-
Daniel P. Berrange
-
Eric Blake
-
Gao feng
-
Jiri Denemark
-
Peter Krempa