[PATCH 0/4] lxc: Couple of improvements
I've noticed these thanks to question on the list: https://www.redhat.com/archives/libvir-list/2020-December/msg00699.html where I suggested to use namespace inheritance. Michal Prívozník (4): schema: Allow lxc:namepsace children to appear individually lxc: Allow NULL argument to lxcDomainDefNamespaceFree() lxc: Rework lxcDomainDefNamespaceParse() lxd_domain: Require that VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE is zero docs/schemas/domaincommon.rng | 66 +++++++++++++++++++---------------- src/lxc/lxc_domain.c | 44 +++++++++++++---------- src/lxc/lxc_domain.h | 2 +- 3 files changed, 63 insertions(+), 49 deletions(-) -- 2.26.2
Since its introduction in v1.2.19-rc1~8 our schema mandates that LXC domain namespace child elements appear either all three at once or not at all: <lxc:namespace> <lxc:sharenet type='netns' value='red'/> <lxc:shareipc type='pid' value='12345'/> <lxc:shareuts type='name' value='container1'/> </lxc:namespace> This is not mandated by our parser though. Neither by code that later uses it (virLXCProcessSetupNamespaces()). Relax the schema. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/schemas/domaincommon.rng | 66 +++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 795b654feb..17e25f14f2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -6821,36 +6821,42 @@ --> <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> + <interleave> + <optional> + <element name="sharenet"> + <attribute name="type"> + <choice> + <value>netns</value> + <value>name</value> + <value>pid</value> + </choice> + </attribute> + <attribute name="value"/> + </element> + </optional> + <optional> + <element name="shareipc"> + <attribute name="type"> + <choice> + <value>name</value> + <value>pid</value> + </choice> + </attribute> + <attribute name="value"/> + </element> + </optional> + <optional> + <element name="shareuts"> + <attribute name="type"> + <choice> + <value>name</value> + <value>pid</value> + </choice> + </attribute> + <attribute name="value"/> + </element> + </optional> + </interleave> </element> </define> -- 2.26.2
On 12/16/20 4:12 PM, Michal Privoznik wrote:
Since its introduction in v1.2.19-rc1~8 our schema mandates that LXC domain namespace child elements appear either all three at once or not at all:
<lxc:namespace> <lxc:sharenet type='netns' value='red'/> <lxc:shareipc type='pid' value='12345'/> <lxc:shareuts type='name' value='container1'/> </lxc:namespace>
This is not mandated by our parser though. Neither by code that later uses it (virLXCProcessSetupNamespaces()). Relax the schema.
Reviewed-by: Laine Stump <laine@redhat.com>
As all other free functions, NULL should be accepted. Even though there currently is no caller that would pass NULL, there will be in future patches. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_domain.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index df60519fca..707262336b 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -195,6 +195,10 @@ lxcDomainDefNamespaceFree(void *nsdata) { size_t i; lxcDomainDefPtr lxcDef = nsdata; + + if (!lxcDef) + return; + for (i = 0; i < VIR_LXC_DOMAIN_NAMESPACE_LAST; i++) g_free(lxcDef->ns_val[i]); g_free(nsdata); -- 2.26.2
On 12/16/20 4:12 PM, Michal Privoznik wrote:
As all other free functions, NULL should be accepted. Even though there currently is no caller that would pass NULL, there will be in future patches.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
While fixing our schema for <lxc:namespace/> I've looked into the parser and realized it could use some treating. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_domain.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 707262336b..255bfab495 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -208,26 +208,31 @@ static int lxcDomainDefNamespaceParse(xmlXPathContextPtr ctxt, void **data) { - lxcDomainDefPtr lxcDef = g_new0(lxcDomainDef, 1); + lxcDomainDefPtr lxcDef = NULL; g_autofree xmlNodePtr *nodes = NULL; - bool uses_lxc_ns = false; VIR_XPATH_NODE_AUTORESTORE(ctxt) - int feature; int n; size_t i; + int ret = -1; if ((n = virXPathNodeSet("./lxc:namespace/*", ctxt, &nodes)) < 0) - goto error; - uses_lxc_ns |= n > 0; + return -1; + + if (n == 0) + return 0; + + lxcDef = g_new0(lxcDomainDef, 1); for (i = 0; i < n; i++) { g_autofree char *tmp = NULL; + int feature; + if ((feature = virLXCDomainNamespaceTypeFromString( (const char *)nodes[i]->name)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unsupported Namespace feature: %s"), - nodes[i]->name); - goto error; + _("unsupported Namespace feature: %s"), + nodes[i]->name); + goto cleanup; } ctxt->node = nodes[i]; @@ -235,31 +240,30 @@ lxcDomainDefNamespaceParse(xmlXPathContextPtr ctxt, if (!(tmp = virXMLPropString(nodes[i], "type"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No lxc environment type specified")); - goto error; + goto cleanup; } if ((lxcDef->ns_source[feature] = virLXCDomainNamespaceSourceTypeFromString(tmp)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown LXC namespace source '%s'"), tmp); - goto error; + goto cleanup; } if (!(lxcDef->ns_val[feature] = virXMLPropString(nodes[i], "value"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No lxc environment type specified")); - goto error; + goto cleanup; } } - if (uses_lxc_ns) - *data = lxcDef; - else - g_free(lxcDef); - return 0; - error: + + *data = g_steal_pointer(&lxcDef); + ret = 0; + + cleanup: lxcDomainDefNamespaceFree(lxcDef); - return -1; + return ret; } -- 2.26.2
On 12/16/20 4:13 PM, Michal Privoznik wrote:
While fixing our schema for <lxc:namespace/> I've looked into the parser and realized it could use some treating.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Huh. Some .... er... "interesting" code you're replacing there :-) Reviewed-by: Laine Stump <laine@redhat.com>
Our parser code relies on the fact that VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE has value of zero and thus uses g_new0(). But strictly speaking, this is not mandated by the enum typedef. Fix that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_domain.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h index 319f83338f..3b5adcbe1c 100644 --- a/src/lxc/lxc_domain.h +++ b/src/lxc/lxc_domain.h @@ -35,7 +35,7 @@ typedef enum { } virLXCDomainNamespace; typedef enum { - VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE, + VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE = 0, VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NAME, VIR_LXC_DOMAIN_NAMESPACE_SOURCE_PID, VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NETNS, -- 2.26.2
On 12/16/20 4:13 PM, Michal Privoznik wrote:
Our parser code relies on the fact that VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE has value of zero and thus uses g_new0(). But strictly speaking, this is not mandated by the enum typedef. Fix that.
Is there really any C compiler that doesn't make the first value a 0 by default? (If so, I wonder why?) Reviewed-by: Laine Stump <laine@redhat.com>
On 12/17/20 3:53 AM, Laine Stump wrote:
On 12/16/20 4:13 PM, Michal Privoznik wrote:
Our parser code relies on the fact that VIR_LXC_DOMAIN_NAMESPACE_SOURCE_NONE has value of zero and thus uses g_new0(). But strictly speaking, this is not mandated by the enum typedef. Fix that.
Is there really any C compiler that doesn't make the first value a 0 by default? (If so, I wonder why?)
Honestly, I don't know.
Reviewed-by: Laine Stump <laine@redhat.com>
Thanks. Michal
participants (2)
-
Laine Stump -
Michal Privoznik