[libvirt] [PATCH] lxc / docker containers gives option to inherit the namespaces. Example lxc-start has option [ --share-[net|ipc|uts] name|pid ] where --share-net name|pid means Inherit a network namespace from a name container or a pid.
Imran Khan
ik.nitk at gmail.com
Tue Jul 21 15:06:17 UTC 2015
Gentle reminder. Humble request for another round of review.
thanks
imran
On Wed, Jul 1, 2015 at 11:30 PM, Imran Khan <ik.nitk at gmail.com> wrote:
> Dear Michal,
>
> Thanks a lot for reviewing the changes. I really appreciate your ability
> to spot very minute errors which have not been spotted by me or my local
> reviews.
>
> I have reworked almost all of the review comments you made. and Have sent
> a separate email with subject "Inherit namespace feature". But for
> convenience i would like to paste the changes here too. Please forgive for
> big mail. To answer some of your questions below:
> 1. Why this feature :
> (imran): As the latest container technology (lxc-tools and
> docker) already provides sharing of network namespace. We think that this
> feature is important to be added in libvirt lxc too.
> please check this link for lxc-start option :[ *--share-[net|ipc|uts]
> **name|pid* ]
> http://man7.org/linux/man-pages/man1/lxc-start.1.html
> please check this link for docker --net option :
> https://docs.docker.com/articles/networking/:
> --net=container:NAME_or_ID
> 2. Moving open namespace in driver.
> (imran) I have moved the code to driver now. But regarding the
> function i used the readymade functions instead of using internal data
> structure because i would end up writing almost same functions again which
> i felt was redundant.
> 3. Regarding the security:
> (imran) This can always be taken care by adding checks in
> pre-install or post-install scripts for libvirt lxc:
> https://libvirt.org/drvlxc.html#security
>
>
> code snippet
>
>
> ---
> docs/drvlxc.html.in | 18 +++
> docs/schemas/domaincommon.rng | 42 ++++++
> src/Makefile.am | 4 +-
> src/lxc/lxc_conf.c | 2 +-
> src/lxc/lxc_conf.h | 15 +++
> src/lxc/lxc_container.c | 236
> +++++++++++++++++++++++++++++++++-
> src/lxc/lxc_domain.c | 164 ++++++++++++++++++++++-
> src/lxc/lxc_domain.h | 1 +
> tests/lxcxml2xmldata/lxc-sharenet.xml | 33 +++++
> tests/lxcxml2xmltest.c | 1 +
> 10 files changed, 507 insertions(+), 9 deletions(-)
> create mode 100644 tests/lxcxml2xmldata/lxc-sharenet.xml
>
> diff --git a/docs/drvlxc.html.in b/docs/drvlxc.html.in
> index a094bd9..d14d4c7 100644
> --- a/docs/drvlxc.html.in
> +++ b/docs/drvlxc.html.in
> @@ -590,6 +590,24 @@ Note that allowing capabilities that are normally
> dropped by default can serious
> affect the security of the container and the host.
> </p>
>
> +<h2><a name="share">Inherit namespaces</a></h2>
> +
> +<p>
> +Libvirt allows you to inherit the namespace from container/process just
> like lxc tools
> +or docker provides to share the network namespace. The following can be
> used to share
> +required namespaces. If we want to share only one then the other
> namespaces can be ignored.
> +</p>
> +<pre>
> +<domain type='lxc' xmlns:lxc='
> http://libvirt.org/schemas/domain/lxc/1.0'>
> +...
> +<lxc:namespace>
> + <lxc:sharenet type='netns' value='red'/>
> + <lxc:shareuts type='name' value='container1'/>
> + <lxc:shareipc type='pid' value='12345'/>
> +</lxc:namespace>
> +</domain>
> +</pre>
> +
> <h2><a name="usage">Container usage / management</a></h2>
>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 1120003..803b327 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -68,6 +68,9 @@
> <ref name='qemucmdline'/>
> </optional>
> <optional>
> + <ref name='lxcsharens'/>
> + </optional>
> + <optional>
> <ref name='keywrap'/>
> </optional>
> </interleave>
> @@ -5012,6 +5015,45 @@
> </element>
> </define>
>
> + <!--
> + Optional hypervisor extensions in their own namespace:
> + LXC
> + -->
> + <define name="lxcsharens">
> + <element name="namespace" ns="
> http://libvirt.org/schemas/domain/lxc/1.0">
> + <zeroOrMore>
> + <element name="sharenet">
> + <attribute name="type">
> + <choice>
> + <value>netns</value>
> + <value>name</value>
> + <value>pid</value>
> + </choice>
> + </attribute>
> + <attribute name='value'/>
> + </element>
> + <element name="shareipc">
> + <attribute name="type">
> + <choice>
> + <value>name</value>
> + <value>pid</value>
> + </choice>
> + </attribute>
> + <attribute name='value'/>
> + </element>
> + <element name="shareuts">
> + <attribute name="type">
> + <choice>
> + <value>name</value>
> + <value>pid</value>
> + </choice>
> + </attribute>
> + <attribute name='value'/>
> + </element>
> + </zeroOrMore>
> + </element>
> + </define>
> +
> <define name="metadata">
> <element name="metadata">
> <zeroOrMore>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index be63e26..ef96a5a 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1319,7 +1319,7 @@ libvirt_driver_lxc_impl_la_CFLAGS = \
> -I$(srcdir)/access \
> -I$(srcdir)/conf \
> $(AM_CFLAGS)
> -libvirt_driver_lxc_impl_la_LIBADD = $(CAPNG_LIBS) $(LIBNL_LIBS)
> $(FUSE_LIBS)
> +libvirt_driver_lxc_impl_la_LIBADD = $(CAPNG_LIBS) $(LIBNL_LIBS)
> $(LIBXML_LIBS) libvirt-lxc.la $(FUSE_LIBS)
> if WITH_BLKID
> libvirt_driver_lxc_impl_la_CFLAGS += $(BLKID_CFLAGS)
> libvirt_driver_lxc_impl_la_LIBADD += $(BLKID_LIBS)
> @@ -2709,6 +2709,8 @@ libvirt_lxc_LDADD = \
> libvirt-net-rpc.la \
> libvirt_security_manager.la \
> libvirt_conf.la \
> + libvirt.la \
> + libvirt-lxc.la \
> libvirt_util.la \
> ../gnulib/lib/libgnu.la
> if WITH_DTRACE_PROBES
> diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
> index c393cb5..96a0f47 100644
> --- a/src/lxc/lxc_conf.c
> +++ b/src/lxc/lxc_conf.c
> @@ -213,7 +213,7 @@ lxcDomainXMLConfInit(void)
> {
> return virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig,
> &virLXCDriverPrivateDataCallbacks,
> - NULL);
> + &virLXCDriverDomainXMLNamespace);
> }
>
>
> diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h
> index 8340b1f..72b1d44 100644
> --- a/src/lxc/lxc_conf.h
> +++ b/src/lxc/lxc_conf.h
> @@ -67,6 +67,21 @@ struct _virLXCDriverConfig {
> bool securityRequireConfined;
> };
>
> +
> +typedef enum {
> + VIR_DOMAIN_NAMESPACE_SHARENET = 0,
> + VIR_DOMAIN_NAMESPACE_SHAREIPC,
> + VIR_DOMAIN_NAMESPACE_SHAREUTS,
> + VIR_DOMAIN_NAMESPACE_LAST,
> +} virDomainNamespace;
> +
> +typedef struct _lxcDomainDef lxcDomainDef;
> +typedef lxcDomainDef *lxcDomainDefPtr;
> +struct _lxcDomainDef {
> + char *ns_type[VIR_DOMAIN_NAMESPACE_LAST];
> + char *ns_val[VIR_DOMAIN_NAMESPACE_LAST];
> +};
> +
> struct _virLXCDriver {
> virMutex lock;
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 11e9514..d8362ab 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -27,6 +27,7 @@
> #include <config.h>
>
> #include <fcntl.h>
> +#include <sched.h>
> #include <limits.h>
> #include <stdlib.h>
> #include <stdio.h>
> @@ -38,7 +39,6 @@
> #include <mntent.h>
> #include <sys/reboot.h>
> #include <linux/reboot.h>
> -
> /* Yes, we want linux private one, for _syscall2() macro */
> #include <linux/unistd.h>
>
> @@ -2321,6 +2321,181 @@ virArch lxcContainerGetAlt32bitArch(virArch arch)
> return VIR_ARCH_NONE;
> }
>
> +struct ns_info {
> + const char *proc_name;
> + int clone_flag;
> +}ns_info_local[VIR_DOMAIN_NAMESPACE_LAST] = {
> + [VIR_DOMAIN_NAMESPACE_SHARENET] = {"net", CLONE_NEWNET},
> + [VIR_DOMAIN_NAMESPACE_SHAREIPC] = {"ipc", CLONE_NEWIPC},
> + [VIR_DOMAIN_NAMESPACE_SHAREUTS] = {"uts", CLONE_NEWUTS}
> +};
> +
> +static int lxcOpen_ns(lxcDomainDefPtr lxcDef, int
> ns_fd[VIR_DOMAIN_NAMESPACE_LAST])
> +{
> + int i, n, rc = 0;
> + virDomainPtr dom = NULL;
> + virConnectPtr conn = NULL;
> + pid_t pid;
> + int nfdlist;
> + int *fdlist;
> + char *path = NULL;
> + char *eptr;
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++)
> + ns_fd[i] = -1;
> +
> + if (STREQ_NULLABLE("netns",
> lxcDef->ns_type[VIR_DOMAIN_NAMESPACE_SHARENET])) {
> + if (virAsprintf(&path, "/var/run/netns/%s",
> lxcDef->ns_val[VIR_DOMAIN_NAMESPACE_SHARENET]) < 0)
> + return -1;
> + ns_fd[VIR_DOMAIN_NAMESPACE_SHARENET] = open(path, O_RDONLY);
> + VIR_FREE(path);
> + if (ns_fd[VIR_DOMAIN_NAMESPACE_SHARENET] < 0) {
> + virReportSystemError(errno,
> + _("failed to open netns %s"),
> lxcDef->ns_val[VIR_DOMAIN_NAMESPACE_SHARENET]);
> + return -1;
> + }
> + }
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
> + /* If not yet intialized by above: netns*/
> + if (lxcDef->ns_type[i] && ns_fd[i] == -1) {
> + pid = strtol(lxcDef->ns_val[i], &eptr, 10);
> + if (*eptr != '\0' || pid < 1) {
> + /* check if the domain is running, then set the namespaces
> + * to that container
> + */
> + const char *ns[] = { "user", "ipc", "uts", "net", "pid",
> "mnt" };
> + conn = virConnectOpen("lxc:///");
> + if (!conn) {
> + virReportError(virGetLastError()->code,
> + _("unable to get connect to lxc %s"),
> lxcDef->ns_val[i]);
> + rc = -1;
> + goto cleanup;
> + }
> + dom = virDomainLookupByName(conn, lxcDef->ns_val[i]);
> + if (!dom) {
> + virReportError(virGetLastError()->code,
> + _("Unable to lookup peer containeri
> %s"),
> + lxcDef->ns_val[i]);
> + rc = -1;
> + goto cleanup;
> + }
> + if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist,
> 0)) < 0) {
> + virReportError(virGetLastError()->code,
> + _("Unable to open %s"),
> lxcDef->ns_val[i]);
> + rc = -1;
> + goto cleanup;
> + }
> + for (n = 0; n < ARRAY_CARDINALITY(ns); n++) {
> + if (STREQ(ns[n], ns_info_local[i].proc_name)) {
> + ns_fd[i] = fdlist[n];
> + } else {
> + if (VIR_CLOSE(fdlist[n]) < 0)
> + VIR_ERROR(_("failed to close fd. ignoring.."));
> + }
> + }
> + if (nfdlist > 0)
> + VIR_FREE(fdlist);
> + } else {
> + if (virAsprintf(&path, "/proc/%d/ns/%s", pid,
> ns_info_local[i].proc_name) < 0)
> + return -1;
> + ns_fd[i] = open(path, O_RDONLY);
> + VIR_FREE(path);
> + if (ns_fd[i] < 0) {
> + virReportSystemError(errno,
> + _("failed to open ns %s"), lxcDef->ns_val[i]);
> + return -1;
> + }
> + }
> + }
> + }
> + cleanup:
> + if (dom)
> + virDomainFree(dom);
> + if (conn)
> + virConnectClose(conn);
> + return rc;
> +}
> +
> +
> +static void lxcClose_ns(int ns_fd[VIR_DOMAIN_NAMESPACE_LAST])
>
> +{
> + int i;
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
> + if (ns_fd[i] > -1) {
> + if (VIR_CLOSE(ns_fd[i]) < 0)
> + virReportSystemError(errno, "%s", _("failed to close
> file"));
> + ns_fd[i] = -1;
> + }
> + }
> +}
> +
> +
> +/**
> + * lxcPreserve_ns:
> + * @ns_fd: array to store current namespace
> + * @clone_flags: namespaces that need to be preserved
> + */
> +static int lxcPreserve_ns(int ns_fd[VIR_DOMAIN_NAMESPACE_LAST], int
> clone_flags)
> +{
> + int i, saved_errno;
> + char *path = NULL;
> +
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++)
> + ns_fd[i] = -1;
> +
> + if (!virFileExists("/proc/self/ns")) {
> + virReportSystemError(errno, "%s",
> + _("Kernel does not support attach;
> preserve_ns ignored"));
> + return -1;
> + }
> +
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
> + if ((clone_flags & ns_info_local[i].clone_flag) == 0)
> + continue;
> + if (virAsprintf(&path, "/proc/self/ns/%s",
> + ns_info_local[i].proc_name) < 0)
> + goto error;
> + ns_fd[i] = open(path, O_RDONLY | O_CLOEXEC);
> + if (ns_fd[i] < 0)
> + goto error;
> + VIR_FREE(path);
> + }
> + return 0;
> + error:
> + saved_errno = errno;
> + lxcClose_ns(ns_fd);
> + errno = saved_errno;
> + virReportSystemError(errno, _("lxcPreserve_ns failed for '%s'"),
> path);
> + VIR_FREE(path);
> + return -1;
> +}
> +
> +/**
> + * lxcAttach_ns:
> + * @ns_fd: array of namespaces to attach
> + */
> +static int lxcAttach_ns(const int ns_fd[VIR_DOMAIN_NAMESPACE_LAST])
> +{
> + int i;
> +
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
> + if (ns_fd[i] < 0)
> + continue;
> + VIR_DEBUG("Setting into namespace\n");
> + /* We get EINVAL if new NS is same as the current
> + * NS, or if the fd namespace doesn't match the
> + * type passed to setns()'s second param. Since we
> + * pass 0, we know the EINVAL is harmless
> + */
> + if (setns(ns_fd[i], 0) < 0 &&
> + errno != EINVAL) {
> + virReportSystemError(errno, _("failed to set namespace '%s'")
> + , ns_info_local[i].proc_name);
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
>
> /**
> * lxcContainerStart:
> @@ -2346,9 +2521,13 @@ int lxcContainerStart(virDomainDefPtr def,
> char **ttyPaths)
> {
> pid_t pid;
> - int cflags;
> + int cflags, i;
> int stacksize = getpagesize() * 4;
> char *stack, *stacktop;
> + int saved_ns_fd[VIR_DOMAIN_NAMESPACE_LAST];
> + int ns_inherit_fd[VIR_DOMAIN_NAMESPACE_LAST];
> + int preserve_mask = 0;
> + lxcDomainDefPtr lxcDef;
> lxc_child_argv_t args = {
> .config = def,
> .securityDriver = securityDriver,
> @@ -2368,7 +2547,12 @@ int lxcContainerStart(virDomainDefPtr def,
>
> stacktop = stack + stacksize;
>
> - cflags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD;
> + lxcDef = def->namespaceData;
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++)
> + if (lxcDef && lxcDef->ns_type[i])
> + preserve_mask |= ns_info_local[i].clone_flag;
> +
> + cflags = CLONE_NEWPID|CLONE_NEWNS|SIGCHLD;
>
> if (userns_required(def)) {
> if (userns_supported()) {
> @@ -2381,10 +2565,43 @@ int lxcContainerStart(virDomainDefPtr def,
> return -1;
> }
> }
> + if (!lxcDef || !lxcDef->ns_type[VIR_DOMAIN_NAMESPACE_SHARENET]) {
> + if (lxcNeedNetworkNamespace(def)) {
> + VIR_DEBUG("Enable network namespaces");
> + cflags |= CLONE_NEWNET;
> + }
> + } else {
> + VIR_DEBUG("Inheriting a net namespace");
> + }
> +
> + if (!lxcDef || !lxcDef->ns_type[VIR_DOMAIN_NAMESPACE_SHAREIPC]) {
> + cflags |= CLONE_NEWIPC;
> + } else {
> + VIR_DEBUG("Inheriting an IPC namespace");
> + }
> +
> + if (!lxcDef || !lxcDef->ns_type[VIR_DOMAIN_NAMESPACE_SHAREUTS]) {
> + cflags |= CLONE_NEWUTS;
> + } else {
> + VIR_DEBUG("Inheriting a UTS namespace");
> + }
> +
> + if (lxcDef && lxcPreserve_ns(saved_ns_fd, preserve_mask) < 0) {
> + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> + _("failed to preserve the namespace"));
> + return -1;
> + }
>
> - if (lxcNeedNetworkNamespace(def)) {
> - VIR_DEBUG("Enable network namespaces");
> - cflags |= CLONE_NEWNET;
> + if (lxcDef && lxcOpen_ns(lxcDef, ns_inherit_fd)) {
> + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> + _("failed to open the namespace"));
> + return -1;
> + }
> +
> + if (lxcDef && lxcAttach_ns(ns_inherit_fd) < 0) {
> + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> + _("failed to attach the namespace"));
> + return -1;
> }
>
> VIR_DEBUG("Cloning container init process");
> @@ -2397,7 +2614,14 @@ int lxcContainerStart(virDomainDefPtr def,
> _("Failed to run clone container"));
> return -1;
> }
> + if (lxcDef && lxcAttach_ns(saved_ns_fd)) {
> + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> + _("failed to restore saved namespaces"));
> + }
>
> + /* clean up */
> + if (lxcDef)
> + lxcClose_ns(ns_inherit_fd);
> return pid;
> }
>
> diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
> index 70606f3..5e63969 100644
> --- a/src/lxc/lxc_domain.c
> +++ b/src/lxc/lxc_domain.c
> @@ -26,8 +26,14 @@
> #include "viralloc.h"
> #include "virlog.h"
> #include "virerror.h"
> +#include <fcntl.h>
> +#include <libxml/xpathInternals.h>
> +#include "virstring.h"
> +#include "virutil.h"
> +#include "virfile.h"
>
> #define VIR_FROM_THIS VIR_FROM_LXC
> +#define LXC_NAMESPACE_HREF "http://libvirt.org/schemas/domain/lxc/1.0"
>
> VIR_LOG_INIT("lxc.lxc_domain");
>
> @@ -41,6 +47,163 @@ static void *virLXCDomainObjPrivateAlloc(void)
> return priv;
> }
>
> +VIR_ENUM_DECL(virDomainNamespace)
> +VIR_ENUM_IMPL(virDomainNamespace, VIR_DOMAIN_NAMESPACE_LAST,
> + N_("sharenet"),
> + N_("shareipc"),
> + N_("shareuts"))
> +
> +static void
> +lxcDomainDefNamespaceFree(void *nsdata)
> +{
> + int j;
> + lxcDomainDefPtr lxcDef = nsdata;
> + for (j = 0; j < VIR_DOMAIN_NAMESPACE_LAST; j++) {
> + VIR_FREE(lxcDef->ns_type[j]);
> + VIR_FREE(lxcDef->ns_val[j]);
> + }
> + VIR_FREE(nsdata);
> +}
> +
> +static int
> +lxcDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED,
> + xmlNodePtr root ATTRIBUTE_UNUSED,
> + xmlXPathContextPtr ctxt,
> + void **data)
> +{
> + lxcDomainDefPtr lxcDef = NULL;
> + xmlNodePtr *nodes = NULL;
> + bool uses_lxc_ns = false;
> + xmlNodePtr node;
> + int feature;
> + int n;
> + char *tmp = NULL;
> + size_t i;
> +
> + if (xmlXPathRegisterNs(ctxt, BAD_CAST "lxc", BAD_CAST
> LXC_NAMESPACE_HREF) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to register xml namespace '%s'"),
> + LXC_NAMESPACE_HREF);
> + return -1;
> + }
> +
> + if (VIR_ALLOC(lxcDef) < 0)
> + return -1;
> + /* Init ns_herit_fd for namespaces */
> + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
> + lxcDef->ns_type[i] = NULL;
>
> + lxcDef->ns_val[i] = NULL;
> + }
> +
> + node = ctxt->node;
> + if ((n = virXPathNodeSet("./lxc:namespace/*", ctxt, &nodes)) < 0)
> + goto error;
> + uses_lxc_ns |= n > 0;
> +
> + for (i = 0; i < n; i++) {
> + feature =
> + virDomainNamespaceTypeFromString((const char *)
> nodes[i]->name);
> + if (feature < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported Namespace feature: %s"),
> + nodes[i]->name);
> + goto error;
> + }
> +
> + ctxt->node = nodes[i];
> +
> + switch ((virDomainNamespace) feature) {
> + case VIR_DOMAIN_NAMESPACE_SHARENET:
> + case VIR_DOMAIN_NAMESPACE_SHAREIPC:
> + case VIR_DOMAIN_NAMESPACE_SHAREUTS:
> + {
> + tmp = virXMLPropString(nodes[i], "type");
> + if (tmp == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("No lxc environment type
> specified"));
> + goto error;
> + }
> + /* save the tmp so that its needed while writing to xml */
> + lxcDef->ns_type[feature] = tmp;
> + tmp = virXMLPropString(nodes[i], "value");
> + if (tmp == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("No lxc environment type
> specified"));
> + goto error;
> + }
> + lxcDef->ns_val[feature] = tmp;
> + }
> + break;
> + case VIR_DOMAIN_NAMESPACE_LAST:
> + break;
> + }
> + }
> + VIR_FREE(nodes);
> + ctxt->node = node;
> + if (uses_lxc_ns)
> + *data = lxcDef;
> + else
> + VIR_FREE(lxcDef);
> + return 0;
> + error:
> + VIR_FREE(nodes);
> + lxcDomainDefNamespaceFree(lxcDef);
> + return -1;
> +}
> +
> +
> +static int
> +lxcDomainDefNamespaceFormatXML(virBufferPtr buf,
> + void *nsdata)
> +{
> + lxcDomainDefPtr lxcDef = nsdata;
> + size_t j;
> +
> + if (!lxcDef)
> + return 0;
> +
> + virBufferAddLit(buf, "<lxc:namespace>\n");
> + virBufferAdjustIndent(buf, 2);
> +
> + for (j = 0; j < VIR_DOMAIN_NAMESPACE_LAST; j++) {
> + switch ((virDomainNamespace) j) {
> + case VIR_DOMAIN_NAMESPACE_SHAREIPC:
> + case VIR_DOMAIN_NAMESPACE_SHAREUTS:
> + case VIR_DOMAIN_NAMESPACE_SHARENET:
> + {
> + if (lxcDef->ns_type[j]) {
> + virBufferAsprintf(buf, "<lxc:%s type='%s'
> value='%s'/>\n",
>
> + virDomainNamespaceTypeToString(j),
> + lxcDef->ns_type[j],
> + lxcDef->ns_val[j]);
> + }
> + }
> + break;
> + case VIR_DOMAIN_NAMESPACE_LAST:
> + break;
> + }
> + }
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</lxc:namespace>\n");
> + return 0;
> +}
> +
> +static const char *
> +lxcDomainDefNamespaceHref(void)
> +{
> + return "xmlns:lxc='" LXC_NAMESPACE_HREF "'";
> +}
> +
> +
> +virDomainXMLNamespace virLXCDriverDomainXMLNamespace = {
> + .parse = lxcDomainDefNamespaceParse,
> + .free = lxcDomainDefNamespaceFree,
> + .format = lxcDomainDefNamespaceFormatXML,
> + .href = lxcDomainDefNamespaceHref,
> +};
> +
> +
> static void virLXCDomainObjPrivateFree(void *data)
> {
> virLXCDomainObjPrivatePtr priv = data;
> @@ -77,7 +240,6 @@ virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
> } else {
> priv->initpid = thepid;
> }
> -
> return 0;
> }
>
> diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
> index 751aece..25df999 100644
> --- a/src/lxc/lxc_domain.h
> +++ b/src/lxc/lxc_domain.h
> @@ -41,6 +41,7 @@ struct _virLXCDomainObjPrivate {
> virCgroupPtr cgroup;
> };
>
> +extern virDomainXMLNamespace virLXCDriverDomainXMLNamespace;
> extern virDomainXMLPrivateDataCallbacks virLXCDriverPrivateDataCallbacks;
> extern virDomainDefParserConfig virLXCDriverDomainDefParserConfig;
>
> diff --git a/tests/lxcxml2xmldata/lxc-sharenet.xml
> b/tests/lxcxml2xmldata/lxc-sharenet.xml
> new file mode 100644
> index 0000000..a2b8d1b
> --- /dev/null
> +++ b/tests/lxcxml2xmldata/lxc-sharenet.xml
> @@ -0,0 +1,33 @@
> +<domain type='lxc' xmlns:lxc='http://libvirt.org/schemas/domain/lxc/1.0'>
> + <name>jessie</name>
> + <uuid>e21987a5-e98e-9c99-0e35-803e4d9ad1fe</uuid>
> + <memory unit='KiB'>1048576</memory>
> + <currentMemory unit='KiB'>1048576</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <resource>
> + <partition>/machine</partition>
> + </resource>
> + <os>
> + <type arch='x86_64'>exe</type>
> + <init>/sbin/init</init>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>restart</on_crash>
> + <devices>
> + <emulator>/usr/libexec/libvirt_lxc</emulator>
> + <filesystem type='mount' accessmode='passthrough'>
> + <source dir='/mach/jessie'/>
> + <target dir='/'/>
> + </filesystem>
> + <console type='pty'>
> + <target type='lxc' port='0'/>
> + </console>
> + </devices>
> + <lxc:namespace>
> + <lxc:sharenet type='netns' value='red'/>
> + <lxc:shareipc type='pid' value='12345'/>
> + <lxc:shareuts type='name' value='container1'/>
> + </lxc:namespace>
> +</domain>
> diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c
> index 3e00347..8d824b9 100644
> --- a/tests/lxcxml2xmltest.c
> +++ b/tests/lxcxml2xmltest.c
> @@ -133,6 +133,7 @@ mymain(void)
> DO_TEST("filesystem-root");
> DO_TEST("idmap");
> DO_TEST("capabilities");
> + DO_TEST("sharenet");
>
> virObjectUnref(caps);
> virObjectUnref(xmlopt);
>
> -----------------------
> -imran
>
>
>
> On Thu, Jun 11, 2015 at 5:47 PM, Michal Privoznik <mprivozn at redhat.com>
> wrote:
>
>> On 21.05.2015 19:43, ik.nitk wrote:
>> > This patch tries to add the similar option to libvirt lxc. So to
>> inherit namespace from name
>> > container c2.
>> > add this into xml.
>> > <lxc:namespace>
>> > <sharenet type='name' value='c2'/>
>> > </lxc:namespace>
>> >
>> > And to inherit namespace from a pid.
>> > add this into xml.
>> > <lxc:namespace>
>> > <sharenet type='pid' value='10245'/>
>> > </lxc:namespace>
>> >
>> > And to inherit namespace from a netns.
>> > add this into xml.
>> > <lxc:namespace>
>> > <sharenet type='netns' value='red'/>
>> > </lxc:namespace>
>> >
>> > Similar options for ipc/uts.
>> > <shareipc /> , <shareuts />
>> >
>> > The reasong lxc xml namespace is added because this feature is very
>> specific to lxc. Therfore wanted to
>> > keep it seperated from actual libvirt xml domain.
>> >
>> > -imran
>> > ---
>>
>> The subject line is just too long. Look at git log to see the style we
>> use to write commit messages.
>>
>> > src/Makefile.am | 5 +-
>> > src/lxc/lxc_conf.c | 2 +-
>> > src/lxc/lxc_conf.h | 23 +++++
>> > src/lxc/lxc_container.c | 191 ++++++++++++++++++++++++++++++++++--
>> > src/lxc/lxc_domain.c | 254
>> +++++++++++++++++++++++++++++++++++++++++++++++-
>> > src/lxc/lxc_domain.h | 1 +
>> > 6 files changed, 463 insertions(+), 13 deletions(-)
>>
>>
>> You are introducing new elements and namespace to the XML. This must
>> always go hand in hand with RNG schema adjustment and a test case or two
>> under tests/. I NACK every patch that does not comply with this rule.
>> But let me review the rest.
>>
>> >
>> > diff --git a/src/Makefile.am b/src/Makefile.am
>> > index 579421d..1a78fde 100644
>> > --- a/src/Makefile.am
>> > +++ b/src/Makefile.am
>> > @@ -1293,7 +1293,8 @@ libvirt_driver_lxc_impl_la_CFLAGS = \
>> > -I$(srcdir)/access \
>> > -I$(srcdir)/conf \
>> > $(AM_CFLAGS)
>> > -libvirt_driver_lxc_impl_la_LIBADD = $(CAPNG_LIBS) $(LIBNL_LIBS)
>> $(FUSE_LIBS)
>> > +libvirt_driver_lxc_impl_la_LIBADD = $(CAPNG_LIBS) $(LIBNL_LIBS)
>> $(LIBXML_LIBS) $(FUSE_LIBS)
>> > +libvirt_driver_lxc_impl_la_LDFLAGS = libvirt-lxc.la
>>
>> This won't fly. If you need libvirt-lxc.la to be added, you must put it
>> into LIBADD. Otherwise automake will fail to see the dependency tree. It
>> happened to me when I was building this with -j5. Although, this won't
>> be needed at all IMO, but more on that later.
>>
>> > if WITH_BLKID
>> > libvirt_driver_lxc_impl_la_CFLAGS += $(BLKID_CFLAGS)
>> > libvirt_driver_lxc_impl_la_LIBADD += $(BLKID_LIBS)
>> > @@ -2652,6 +2653,8 @@ libvirt_lxc_LDADD = \
>> > libvirt-net-rpc.la \
>> > libvirt_security_manager.la \
>> > libvirt_conf.la \
>> > + libvirt.la \
>> > + libvirt-lxc.la \
>> > libvirt_util.la \
>> > ../gnulib/lib/libgnu.la
>>
>> > if WITH_DTRACE_PROBES
>> > diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c
>> > index c393cb5..96a0f47 100644
>> > --- a/src/lxc/lxc_conf.c
>> > +++ b/src/lxc/lxc_conf.c
>> > @@ -213,7 +213,7 @@ lxcDomainXMLConfInit(void)
>> > {
>> > return virDomainXMLOptionNew(&virLXCDriverDomainDefParserConfig,
>> > &virLXCDriverPrivateDataCallbacks,
>> > - NULL);
>> > + &virLXCDriverDomainXMLNamespace);
>> > }
>> >
>> >
>> > diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h
>> > index 8340b1f..59002e5 100644
>> > --- a/src/lxc/lxc_conf.h
>> > +++ b/src/lxc/lxc_conf.h
>> > @@ -67,6 +67,29 @@ struct _virLXCDriverConfig {
>> > bool securityRequireConfined;
>> > };
>> >
>> > +
>> > +typedef enum {
>> > + VIR_DOMAIN_NAMESPACE_SHARENET = 0,
>> > + VIR_DOMAIN_NAMESPACE_SHAREIPC,
>> > + VIR_DOMAIN_NAMESPACE_SHAREUTS,
>> > + VIR_DOMAIN_NAMESPACE_LAST,
>> > +} virDomainNamespace;
>> > +
>> > +struct ns_info {
>> > + const char *proc_name;
>> > + int clone_flag;
>> > +};
>> > +
>> > +extern const struct ns_info ns_info[VIR_DOMAIN_NAMESPACE_LAST];
>> > +
>> > +typedef struct _lxcDomainDef lxcDomainDef;
>> > +typedef lxcDomainDef *lxcDomainDefPtr;
>> > +struct _lxcDomainDef {
>> > + int ns_inherit_fd[VIR_DOMAIN_NAMESPACE_LAST];
>> > + char *ns_type[VIR_DOMAIN_NAMESPACE_LAST];
>> > + char *ns_val[VIR_DOMAIN_NAMESPACE_LAST];
>> > +};
>> > +
>> > struct _virLXCDriver {
>> > virMutex lock;
>> >
>> > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
>> > index 9a9ae5c..a9a7ba0 100644
>> > --- a/src/lxc/lxc_container.c
>> > +++ b/src/lxc/lxc_container.c
>> > @@ -25,8 +25,8 @@
>> > */
>> >
>> > #include <config.h>
>> > -
>>
>> No, we like the extra space here. config.h has a special status.
>>
>> > #include <fcntl.h>
>> > +#include <sched.h>
>> > #include <limits.h>
>> > #include <stdlib.h>
>> > #include <stdio.h>
>> > @@ -38,7 +38,6 @@
>> > #include <mntent.h>
>> > #include <sys/reboot.h>
>> > #include <linux/reboot.h>
>> > -
>> > /* Yes, we want linux private one, for _syscall2() macro */
>> > #include <linux/unistd.h>
>> >
>> > @@ -99,6 +98,50 @@ VIR_LOG_INIT("lxc.lxc_container");
>> > typedef char lxc_message_t;
>> > #define LXC_CONTINUE_MSG 'c'
>> >
>>
>>
>> > +#ifdef __linux__
>> > +/*
>> > + * Workaround older glibc. While kernel may support the setns
>> > + * syscall, the glibc wrapper might not exist. If that's the
>> > + * case, use our own.
>> > + */
>> > +# ifndef __NR_setns
>> > +# if defined(__x86_64__)
>> > +# define __NR_setns 308
>> > +# elif defined(__i386__)
>> > +# define __NR_setns 346
>> > +# elif defined(__arm__)
>> > +# define __NR_setns 375
>> > +# elif defined(__aarch64__)
>> > +# define __NR_setns 375
>> > +# elif defined(__powerpc__)
>> > +# define __NR_setns 350
>> > +# elif defined(__s390__)
>> > +# define __NR_setns 339
>> > +# endif
>> > +# endif
>> > +
>> > +# ifndef HAVE_SETNS
>> > +# if defined(__NR_setns)
>> > +# include <sys/syscall.h>
>> > +
>> > +static inline int setns(int fd, int nstype)
>> > +{
>> > + return syscall(__NR_setns, fd, nstype);
>> > +}
>> > +# else /* !__NR_setns */
>> > +# error Please determine the syscall number for setns on your
>> architecture
>> > +# endif
>> > +# endif
>> > +#else /* !__linux__ */
>> > +static inline int setns(int fd ATTRIBUTE_UNUSED, int nstype
>> ATTRIBUTE_UNUSED)
>> > +{
>> > + virReportSystemError(ENOSYS, "%s",
>> > + _("Namespaces are not supported on this
>> platform."));
>> > + return -1;
>> > +}
>> > +#endif
>> > +
>> > +
>>
>> This seems like copied over from util/virprocess.c. I think you can use
>> setns() function defined there instead of redefining your own.
>>
>> > typedef struct __lxc_child_argv lxc_child_argv_t;
>> > struct __lxc_child_argv {
>> > virDomainDefPtr config;
>> > @@ -2233,7 +2276,6 @@ static int lxcContainerChild(void *data)
>> > vmDef->os.init);
>> > goto cleanup;
>> > }
>> > -
>>
>> I think this empty line is intentional here.
>>
>> > /* rename and enable interfaces */
>> > if (lxcContainerRenameAndEnableInterfaces(vmDef,
>> > argv->nveths,
>> > @@ -2321,6 +2363,99 @@ virArch lxcContainerGetAlt32bitArch(virArch arch)
>> > return VIR_ARCH_NONE;
>> > }
>> >
>> > +/* Used only for containers,same as the one defined in
>> > + * domain_conf.c. But used locally
>> > + */
>> > +static const struct ns_info ns_info_local[VIR_DOMAIN_NAMESPACE_LAST] =
>> {
>> > + [VIR_DOMAIN_NAMESPACE_SHARENET] = {"net", CLONE_NEWNET},
>> > + [VIR_DOMAIN_NAMESPACE_SHAREIPC] = {"ipc", CLONE_NEWIPC},
>> > + [VIR_DOMAIN_NAMESPACE_SHAREUTS] = {"uts", CLONE_NEWUTS}
>> > +};
>> > +
>> > +
>> > +static void close_ns(int ns_fd[VIR_DOMAIN_NAMESPACE_LAST])
>> > +{
>> > + int i;
>> > + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
>> > + if (ns_fd[i] > -1) {
>> > + if (VIR_CLOSE(ns_fd[i]) < 0)
>> > + virReportSystemError(errno, "%s", _("failed to close
>> file"));
>> > + ns_fd[i] = -1;
>> > + }
>> > + }
>> > +}
>> > +
>> > +
>> > +/**
>> > + * lxcPreserve_ns:
>> > + * @ns_fd: array to store current namespace
>> > + * @clone_flags: namespaces that need to be preserved
>> > + */
>> > +static int lxcPreserve_ns(int ns_fd[VIR_DOMAIN_NAMESPACE_LAST], int
>> clone_flags)
>> > +{
>> > + int i, saved_errno;
>> > + char *path = NULL;
>> > +
>> > + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++)
>> > + ns_fd[i] = -1;
>> > +
>> > + if (access("/proc/self/ns", X_OK)) {
>>
>> virFileIsExecutable. Although I guess you want tot check if the file
>> exists, in which case virFileExists is just enough.
>>
>> > + virReportSystemError(errno, "%s",
>> > + _("Kernel does not support attach;
>> preserve_ns ignored"));
>> > + return 0;
>>
>> So an error is reported, but success is claimed?
>>
>> > + }
>> > +
>> > + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
>> > + if ((clone_flags & ns_info_local[i].clone_flag) == 0)
>> > + continue;
>> > + if (virAsprintf(&path, "/proc/self/ns/%s",
>> > + ns_info_local[i].proc_name) < 0)
>> > + goto error;
>>
>> If virAsprintf fails, an error is already reported. But due to 'goto
>> error' you overwrite it with (wrong) error message.
>>
>> > + ns_fd[i] = open(path, O_RDONLY | O_CLOEXEC);
>> > + if (ns_fd[i] < 0)
>> > + goto error;
>> > + }
>> > + VIR_FREE(path);
>> > + return 0;
>> > + error:
>> > + saved_errno = errno;
>> > + close_ns(ns_fd);
>> > + errno = saved_errno;
>> > + VIR_FREE(path);
>> > + virReportSystemError(errno, _("failed to open '%s'"), path);
>>
>> Ouch. @path is NULL at this point.
>>
>> > + return -1;
>> > +}
>> > +
>> > +/**
>> > + * lxcAttach_ns:
>> > + * @ns_fd: array of namespaces to attach
>> > + */
>> > +static int lxcAttach_ns(const int ns_fd[VIR_DOMAIN_NAMESPACE_LAST])
>> > +{
>> > + int i;
>> > +
>> > + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
>> > + if (ns_fd[i] < 0)
>> > + continue;
>> > + VIR_DEBUG("Setting into namespace\n");
>> > +
>> > + /* We get EINVAL if new NS is same as the current
>> > + * NS, or if the fd namespace doesn't match the
>> > + * type passed to setns()'s second param. Since we
>> > + * pass 0, we know the EINVAL is harmless
>> > + */
>> > + if (setns(ns_fd[i], 0) < 0 &&
>> > + errno != EINVAL)
>> > + goto error;
>>
>> Any reason why the block under 'error' label is not here directly?
>>
>> > + }
>> > + return 0;
>> > +
>> > + error:
>> > + virReportSystemError(errno, _("failed to set namespace '%s'")
>> > + , ns_info_local[i].proc_name);
>> > + return -1;
>> > +}
>> > +
>> >
>> > /**
>> > * lxcContainerStart:
>> > @@ -2346,9 +2481,12 @@ int lxcContainerStart(virDomainDefPtr def,
>> > char **ttyPaths)
>> > {
>> > pid_t pid;
>> > - int cflags;
>> > + int cflags, i;
>> > int stacksize = getpagesize() * 4;
>> > char *stack, *stacktop;
>> > + int saved_ns_fd[VIR_DOMAIN_NAMESPACE_LAST];
>> > + int preserve_mask = 0;
>> > + lxcDomainDefPtr lxcDef;
>> > lxc_child_argv_t args = {
>> > .config = def,
>> > .securityDriver = securityDriver,
>> > @@ -2368,7 +2506,14 @@ int lxcContainerStart(virDomainDefPtr def,
>> >
>> > stacktop = stack + stacksize;
>> >
>> > - cflags =
>> CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|SIGCHLD;
>> > + lxcDef = def->namespaceData;
>> > + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++)
>> > + if (lxcDef && lxcDef->ns_inherit_fd[i] != -1)
>> > + preserve_mask |= ns_info_local[i].clone_flag;
>> > +
>> > +
>> > +
>> > + cflags = CLONE_NEWPID|CLONE_NEWNS|SIGCHLD;
>> >
>> > if (userns_required(def)) {
>> > if (userns_supported()) {
>> > @@ -2381,10 +2526,36 @@ int lxcContainerStart(virDomainDefPtr def,
>> > return -1;
>> > }
>> > }
>> > + if (!lxcDef || (lxcDef &&
>> lxcDef->ns_inherit_fd[VIR_DOMAIN_NAMESPACE_SHARENET] == -1)) {
>>
>> C language short-cuircuit conditions evaluation. Therefore, if lxcDef is
>> NULL, the first part of the condition will direct control into the if()
>> body. Therefore there's no need to check:
>>
>> if (!a || (a && a->something));
>>
>> It's equivallent to:
>>
>> if (!a || a->something);
>>
>> > + if (lxcNeedNetworkNamespace(def)) {
>> > + VIR_DEBUG("Enable network namespaces");
>> > + cflags |= CLONE_NEWNET;
>> > + }
>> > + } else {
>> > + VIR_DEBUG("Inheriting a net namespace");
>> > + }
>> >
>> > - if (lxcNeedNetworkNamespace(def)) {
>> > - VIR_DEBUG("Enable network namespaces");
>> > - cflags |= CLONE_NEWNET;
>> > + if (!lxcDef || (lxcDef &&
>> lxcDef->ns_inherit_fd[VIR_DOMAIN_NAMESPACE_SHAREIPC] == -1)) {
>> > + cflags |= CLONE_NEWIPC;
>> > + } else {
>> > + VIR_DEBUG("Inheriting an IPC namespace");
>> > + }
>> > +
>> > + if (!lxcDef || (lxcDef &&
>> lxcDef->ns_inherit_fd[VIR_DOMAIN_NAMESPACE_SHAREUTS] == -1)) {
>> > + cflags |= CLONE_NEWUTS;
>> > + } else {
>> > + VIR_DEBUG("Inheriting a UTS namespace");
>> > + }
>> > +
>> > + if (lxcDef && lxcPreserve_ns(saved_ns_fd, preserve_mask) < 0) {
>> > + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
>> > + _("failed to preserve the namespace"));
>> > + return -1;
>> > + }
>> > + if (lxcDef && lxcAttach_ns(lxcDef->ns_inherit_fd) < 0) {
>> > + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
>> > + _("failed to attach the namespace"));
>> > + return -1;
>> > }
>> >
>> > VIR_DEBUG("Cloning container init process");
>> > @@ -2397,6 +2568,10 @@ int lxcContainerStart(virDomainDefPtr def,
>> > _("Failed to run clone container"));
>> > return -1;
>> > }
>> > + if (lxcDef && lxcAttach_ns(saved_ns_fd)) {
>> > + virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
>> > + _("failed to restore saved namespaces"));
>> > + }
>> >
>> > return pid;
>> > }
>> > diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c
>> > index c2180cb..6e4a19a 100644
>> > --- a/src/lxc/lxc_domain.c
>> > +++ b/src/lxc/lxc_domain.c
>> > @@ -20,14 +20,18 @@
>> > */
>> >
>> > #include <config.h>
>> > -
>> > #include "lxc_domain.h"
>> > -
>>
>> I've raised this already.
>>
>> > #include "viralloc.h"
>> > #include "virlog.h"
>> > #include "virerror.h"
>> > +#include <fcntl.h>
>> > +#include <libxml/xpathInternals.h>
>> > +#include "virstring.h"
>> > +#include "virutil.h"
>> > +#include "virfile.h"
>> >
>> > #define VIR_FROM_THIS VIR_FROM_LXC
>> > +#define LXC_NAMESPACE_HREF "http://libvirt.org/schemas/domain/lxc/1.0"
>>
>> >
>> > VIR_LOG_INIT("lxc.lxc_domain");
>> >
>> > @@ -41,6 +45,251 @@ static void *virLXCDomainObjPrivateAlloc(void)
>> > return priv;
>> > }
>> >
>> > +
>> > +static int open_ns(const char *nnsname_pid, const char *ns_proc_name)
>> > +{
>> > + int fd = -1;
>> > + virDomainPtr dom = NULL;
>> > + virConnectPtr conn = NULL;
>> > + pid_t pid;
>> > + int nfdlist;
>> > + int *fdlist;
>> > + char *path = NULL;
>> > + char *eptr;
>> > + pid = strtol(nnsname_pid, &eptr, 10);
>> > + if (*eptr != '\0' || pid < 1) {
>> > + /* check if the domain is running, then set the namespaces
>> > + * to that container
>> > + */
>> > + size_t i = 0;
>> > + const char *ns[] = { "user", "ipc", "uts", "net", "pid", "mnt"
>> };
>> > + conn = virConnectOpen("lxc:///");
>> > + if (!conn)
>> > + goto cleanup;
>> > + dom = virDomainLookupByName(conn, nnsname_pid);
>> > + if (!dom)
>> > + goto cleanup;
>> > + if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0)
>> > + goto cleanup;
>> > + /* Internally above function calls virProcessGetNamespaces
>> > + * function which opens ns
>> > + * in the order { "user", "ipc", "uts", "net", "pid", "mnt" }
>> > + */
>> > + for (i = 0; i < ARRAY_CARDINALITY(ns); i++) {
>> > + if (STREQ(ns[i], ns_proc_name)) {
>> > + fd = fdlist[i];
>> > + break;
>> > + }
>> > + }
>> > + if (nfdlist > 0)
>>
>> No, please no. If you read the virDomainLxcOpenNamespace() description,
>> you'll notice that not only we must free the array, but also close all
>> the file descriptors. You'll leak all of them but one. That's not
>> acceptable.
>>
>> > + VIR_FREE(fdlist);
>> > + } else {
>> > + if (virAsprintf(&path, "/proc/%d/ns/%s", pid, ns_proc_name) <
>> 0)
>> > + goto cleanup;
>> > + fd = open(path, O_RDONLY);
>> > + }
>> > +cleanup:
>> > + if (dom)
>> > + virDomainFree(dom);
>> > + VIR_FREE(path);
>> > + (fd < 0)? VIR_ERROR(
>> > + _("failed to open ns %s"), nnsname_pid):
>> > + VIR_DEBUG("OPENED NAMESPACE : fd %d\n", fd);
>> > + return fd;
>> > +}
>>
>>
>> This is the part I'm having trouble with. IIUC, this code is run at the
>> time of domain XML parsing. The other domain I'm referring to may still
>> not be running. How am I going to set the same namespace then?
>>
>> I think this should be done at domain startup (and fail, if the
>> referenced PID or domain does not exist). Having said that, when doing
>> this in driver, that's starting a namespace, you have access to all
>> internal variables and functions. Then you don't need to open a dummy
>> connection just to look up the referenced domain. You can use an
>> internal function which does not require virConnection object. Moreover,
>> you will not need the Makefile change.
>>
>> > +
>> > +
>> > +/* Used only for containers */
>> > +const struct ns_info ns_info[VIR_DOMAIN_NAMESPACE_LAST] = {
>> > + [VIR_DOMAIN_NAMESPACE_SHARENET] = {"net", CLONE_NEWNET},
>> > + [VIR_DOMAIN_NAMESPACE_SHAREIPC] = {"ipc", CLONE_NEWIPC},
>> > + [VIR_DOMAIN_NAMESPACE_SHAREUTS] = {"uts", CLONE_NEWUTS}
>> > +};
>> > +
>> > +VIR_ENUM_DECL(virDomainNamespace)
>> > +VIR_ENUM_IMPL(virDomainNamespace, VIR_DOMAIN_NAMESPACE_LAST,
>> > + N_("sharenet"),
>> > + N_("shareipc"),
>> > + N_("shareuts"))
>> > +
>> > +static void
>> > +lxcDomainDefNamespaceFree(void *nsdata)
>> > +{
>> > + int j;
>> > + lxcDomainDefPtr lxcDef = nsdata;
>> > + for (j = 0; j < VIR_DOMAIN_NAMESPACE_LAST; j++) {
>> > + if (lxcDef->ns_inherit_fd[j] > 0) {
>> > + VIR_FREE(lxcDef->ns_type);
>> > + VIR_FREE(lxcDef->ns_val);
>> > +#if 0
>> > + if (VIR_CLOSE(lxcDef->ns_inherit_fd[j]) < 0)
>> > + virReportSystemError(errno, "%s", _("failed to close
>> file"));
>> > +#endif
>> > + }
>> > + }
>> > + VIR_FREE(nsdata);
>> > +}
>> > +
>> > +static int
>> > +lxcDomainDefNamespaceParse(xmlDocPtr xml ATTRIBUTE_UNUSED,
>> > + xmlNodePtr root ATTRIBUTE_UNUSED,
>> > + xmlXPathContextPtr ctxt,
>> > + void **data)
>> > +{
>> > + lxcDomainDefPtr lxcDef = NULL;
>> > + xmlNodePtr *nodes = NULL;
>> > + bool uses_lxc_ns = false;
>> > + xmlNodePtr node;
>> > + int feature;
>> > + int n;
>> > + char *tmp = NULL;
>> > + size_t i;
>> > + pid_t fd = -1;
>> > +
>> > + if (xmlXPathRegisterNs(ctxt, BAD_CAST "lxc", BAD_CAST
>> LXC_NAMESPACE_HREF) < 0) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + _("Failed to register xml namespace '%s'"),
>> > + LXC_NAMESPACE_HREF);
>> > + return -1;
>> > + }
>> > +
>> > + if (VIR_ALLOC(lxcDef) < 0)
>> > + return -1;
>> > +
>> > + /* Init ns_herit_fd for namespaces */
>> > + for (i = 0; i < VIR_DOMAIN_NAMESPACE_LAST; i++) {
>> > + lxcDef->ns_inherit_fd[i] = -1;
>> > + lxcDef->ns_type[i] = NULL;
>> > + lxcDef->ns_val[i] = NULL;
>> > + }
>> > +
>> > + node = ctxt->node;
>> > + if ((n = virXPathNodeSet("./lxc:namespace/*", ctxt, &nodes)) < 0)
>> > + goto error;
>> > + uses_lxc_ns |= n > 0;
>> > +
>> > + for (i = 0; i < n; i++) {
>> > + feature =
>> > + virDomainNamespaceTypeFromString((const char *)
>> nodes[i]->name);
>> > + if (feature < 0) {
>> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> > + _("unsupported Namespace feature: %s"),
>> > + nodes[i]->name);
>> > + goto error;
>> > + }
>> > +
>> > + ctxt->node = nodes[i];
>> > +
>> > + switch ((virDomainNamespace) feature) {
>> > + case VIR_DOMAIN_NAMESPACE_SHARENET:
>> > + case VIR_DOMAIN_NAMESPACE_SHAREIPC:
>> > + case VIR_DOMAIN_NAMESPACE_SHAREUTS:
>> > + {
>> > + tmp = virXMLPropString(nodes[i], "type");
>> > + if (tmp == NULL) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + "%s", _("No lxc environment type
>> specified"));
>> > + goto error;
>> > + }
>> > + /* save the tmp so that its needed while writing to
>> xml */
>> > + lxcDef->ns_type[feature] = tmp;
>> > + tmp = virXMLPropString(nodes[i], "value");
>> > + if (tmp == NULL) {
>> > + virReportError(VIR_ERR_INTERNAL_ERROR,
>> > + "%s", _("No lxc environment type
>> specified"));
>> > + goto error;
>> > + }
>> > + lxcDef->ns_val[feature] = tmp;
>> > + /*netns option is only for
>> VIR_DOMAIN_NAMESPACE_SHARENET*/
>> > + if (STREQ("netns",
>> lxcDef->ns_type[VIR_DOMAIN_NAMESPACE_SHARENET])) {
>>
>> I don't think this is safe. ns_type[SHARENET] can be NULL at this point.
>> STREQ_NULLABLE is more apropriate.
>>
>> > + char *path = NULL;
>> > + if (virAsprintf(&path, "/var/run/netns/%s", tmp) <
>> 0)
>> > + goto error;
>> > + fd = open(path, O_RDONLY);
>> > + VIR_FREE(path);
>>
>> What if open() fails?
>>
>> > + } else {
>> > + fd = open_ns(tmp, ns_info[feature].proc_name);
>> > + if (fd < 0) {
>> > + virReportError(VIR_ERR_XML_ERROR,
>> > + _("unable to open %s namespace
>> for "
>> > + "namespace feature '%s'"),
>> tmp,
>> > + nodes[i]->name);
>> > + goto error;
>> > + }
>> > + }
>> > + lxcDef->ns_inherit_fd[feature] = fd;
>> > + }
>> > + break;
>> > + case VIR_DOMAIN_NAMESPACE_LAST:
>> > + break;
>> > + }
>> > + }
>> > + VIR_FREE(nodes);
>> > + ctxt->node = node;
>> > + if (uses_lxc_ns)
>> > + *data = lxcDef;
>> > + else
>> > + VIR_FREE(lxcDef);
>> > + return 0;
>> > + error:
>> > + VIR_FREE(nodes);
>> > + lxcDomainDefNamespaceFree(lxcDef);
>> > + return -1;
>> > +}
>> > +
>> > +
>> > +static int
>> > +lxcDomainDefNamespaceFormatXML(virBufferPtr buf,
>> > + void *nsdata)
>> > +{
>> > + lxcDomainDefPtr lxcDef = nsdata;
>> > + size_t j;
>> > +
>> > + if (!lxcDef)
>> > + return 0;
>> > +
>> > + virBufferAddLit(buf, "<lxc:namespace>\n");
>> > + virBufferAdjustIndent(buf, 2);
>> > +
>> > + for (j = 0; j < VIR_DOMAIN_NAMESPACE_LAST; j++) {
>> > + switch ((virDomainNamespace) j) {
>> > + case VIR_DOMAIN_NAMESPACE_SHAREIPC:
>> > + case VIR_DOMAIN_NAMESPACE_SHAREUTS:
>> > + case VIR_DOMAIN_NAMESPACE_SHARENET:
>> > + {
>> > + if (lxcDef->ns_inherit_fd[j] > 0) {
>> > + virBufferAsprintf(buf, "<%s type='%s'
>> value='%s'/>\n",
>> > +
>> virDomainNamespaceTypeToString(j),
>> > + lxcDef->ns_type[j],
>> > + lxcDef->ns_val[j]);
>> > + }
>> > + }
>> > + break;
>> > + case VIR_DOMAIN_NAMESPACE_LAST:
>> > + break;
>> > + }
>> > + }
>> > +
>> > + virBufferAdjustIndent(buf, -2);
>> > + virBufferAddLit(buf, "</lxc:namespace>\n");
>> > + return 0;
>> > +}
>> > +
>> > +static const char *
>> > +lxcDomainDefNamespaceHref(void)
>> > +{
>> > + return "xmlns:lxc='" LXC_NAMESPACE_HREF "'";
>> > +}
>> > +
>> > +
>> > +virDomainXMLNamespace virLXCDriverDomainXMLNamespace = {
>> > + .parse = lxcDomainDefNamespaceParse,
>> > + .free = lxcDomainDefNamespaceFree,
>> > + .format = lxcDomainDefNamespaceFormatXML,
>> > + .href = lxcDomainDefNamespaceHref,
>> > +};
>> > +
>> > +
>> > static void virLXCDomainObjPrivateFree(void *data)
>> > {
>> > virLXCDomainObjPrivatePtr priv = data;
>> > @@ -73,7 +322,6 @@ static int
>> virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data)
>> > } else {
>> > priv->initpid = thepid;
>> > }
>> > -
>> > return 0;
>> > }
>>
>> Yet again, why is this change needed?
>>
>> >
>> > diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h
>> > index 751aece..25df999 100644
>> > --- a/src/lxc/lxc_domain.h
>> > +++ b/src/lxc/lxc_domain.h
>> > @@ -41,6 +41,7 @@ struct _virLXCDomainObjPrivate {
>> > virCgroupPtr cgroup;
>> > };
>> >
>> > +extern virDomainXMLNamespace virLXCDriverDomainXMLNamespace;
>> > extern virDomainXMLPrivateDataCallbacks
>> virLXCDriverPrivateDataCallbacks;
>> > extern virDomainDefParserConfig virLXCDriverDomainDefParserConfig;
>> >
>> >
>>
>> Frankly, I don't have good feeling about this. But maybe I'm missing
>> some rationale behind. What's the usecase? You want two containers to
>> share the same network namespace, or?
>> I view two or more containers in the same namespace as a security flaw,
>> not feature. With this mind set maybe I'm not the right person to review
>> the patches. But hey, I'm open for persuasion :)
>>
>> Michal
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150721/42d6fedb/attachment-0001.htm>
More information about the libvir-list
mailing list