[libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.
This patch introduce the new settings for LXC 3.0 or higher. The older versions keep the compatibility to deprecated settings for LXC, but after release 3.0, the compatibility was removed. This commit adds the support to the refactored settings. v1-v2: Michal's suggestions to handle differences between versions. v2-v3: Adding suggestions from Pino and John too. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index cbdec723dd..d3ba12bb0e 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -200,8 +200,13 @@ lxcSetRootfs(virDomainDefPtr def, int type = VIR_DOMAIN_FS_TYPE_MOUNT; VIR_AUTOFREE(char *) value = NULL; - if (virConfGetValueString(properties, "lxc.rootfs", &value) <= 0) - return -1; + if (virConfGetValueString(properties, "lxc.rootfs.path", &value) <= 0) { + virResetLastError(); + + /* Check for pre LXC 3.0 legacy key */ + if (virConfGetValueString(properties, "lxc.rootfs", &value) <= 0) + return -1; + } if (STRPREFIX(value, "/dev/")) type = VIR_DOMAIN_FS_TYPE_BLOCK; @@ -684,8 +689,13 @@ lxcCreateConsoles(virDomainDefPtr def, virConfPtr properties) virDomainChrDefPtr console; size_t i; - if (virConfGetValueString(properties, "lxc.tty", &value) <= 0) - return 0; + if (virConfGetValueString(properties, "lxc.tty.max", &value) <= 0) { + virResetLastError(); + + /* Check for pre LXC 3.0 legacy key */ + if (virConfGetValueString(properties, "lxc.tty", &value) <= 0) + return 0; + } if (virStrToLong_i(value, NULL, 10, &nbttys) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse int: '%s'"), @@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void *data) char type; unsigned long start, target, count; - if (STRNEQ(name, "lxc.id_map") || !value->str) + /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ + if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str) return 0; if (sscanf(value->str, "%c %lu %lu %lu", &type, &target, &start, &count) != 4) { - virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"), + virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"), value->str); return -1; } @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config, if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0) goto error; - if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0 || - VIR_STRDUP(vmdef->name, value) < 0) - goto error; + if (virConfGetValueString(properties, "lxc.uts.name", &value) <= 0) { + virResetLastError(); + + /* Check for pre LXC 3.0 legacy key */ + if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0) + goto error; + } + if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") < 0)) goto error; if (lxcSetRootfs(vmdef, properties) < 0) goto error; - /* Look for fstab: we shouldn't have it */ - if (virConfGetValue(properties, "lxc.mount")) { + /* LXC 3.0 uses "lxc.mount.fstab", while legacy used just "lxc.mount". + * In either case, generate the error to use "lxc.mount.entry" instead */ + if (virConfGetValue(properties, "lxc.mount.fstab") || + virConfGetValue(properties, "lxc.mount")) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("lxc.mount found, use lxc.mount.entry lines instead")); + _("lxc.mount.fstab or lxc.mount found, use " + "lxc.mount.entry lines instead")); goto error; } @@ -1069,7 +1088,7 @@ lxcParseConfigString(const char *config, if (lxcCreateConsoles(vmdef, properties) < 0) goto error; - /* lxc.id_map */ + /* lxc.idmap or legacy lxc.id_map */ if (virConfWalk(properties, lxcIdmapWalkCallback, vmdef) < 0) goto error; -- 2.19.1
On 11/9/18 12:30 PM, Julio Faracco wrote:
This patch introduce the new settings for LXC 3.0 or higher. The older versions keep the compatibility to deprecated settings for LXC, but after release 3.0, the compatibility was removed. This commit adds the support to the refactored settings.
v1-v2: Michal's suggestions to handle differences between versions. v2-v3: Adding suggestions from Pino and John too.
These type of comments would go below the --- below so that they're not part of commit history...
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index cbdec723dd..d3ba12bb0e 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c
[...]
@@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void *data) char type; unsigned long start, target, count;
- if (STRNEQ(name, "lxc.id_map") || !value->str) + /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ + if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str) return 0;
This one caused lxcconf2xmltest to fail and needs to change to: /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ if (STRNEQ(name, "lxc.idmap") || !value->str) { if (!value->str || STRNEQ(name, "lxc.id_map")) return 0; } The failure occurred because of the STRNEQ OR not being true (silly me on first pass not running the tests too ;-))
if (sscanf(value->str, "%c %lu %lu %lu", &type, &target, &start, &count) != 4) { - virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"), + virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),
Do you mind if I alter this to: virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"), name, value->str); That way the conf name string is provided like it was before
value->str); return -1; } @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config, if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0) goto error;
- if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0 || - VIR_STRDUP(vmdef->name, value) < 0) - goto error; + if (virConfGetValueString(properties, "lxc.uts.name", &value) <= 0) { + virResetLastError(); + + /* Check for pre LXC 3.0 legacy key */ + if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0) + goto error; + } +
I think in this case the @value needs to be restored... Previous if the GetValueString OR the VIR_STRDUP of the value was < 0, we go to error. Although I'm not quite sure how @value would be NULL so as to cause the subsequent line to be executed... In any case, copying @value needs to be done, so add: if (VIR_STRDUP(vmdef->name, value) < 0) goto error; Which I can add if you agree. With those changes, Reviewed-by: John Ferlan <jferlan@redhat.com> John As a follow-up maybe adding/altering/updating the tests/lxcconf2xmldata to include both pre and post 3.0 type data would be a good thing. [...]
Em sáb, 10 de nov de 2018 às 11:17, John Ferlan <jferlan@redhat.com> escreveu:
On 11/9/18 12:30 PM, Julio Faracco wrote:
This patch introduce the new settings for LXC 3.0 or higher. The older versions keep the compatibility to deprecated settings for LXC, but after release 3.0, the compatibility was removed. This commit adds the support to the refactored settings.
v1-v2: Michal's suggestions to handle differences between versions. v2-v3: Adding suggestions from Pino and John too.
These type of comments would go below the --- below so that they're not part of commit history...
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index cbdec723dd..d3ba12bb0e 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c
[...]
@@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void *data) char type; unsigned long start, target, count;
- if (STRNEQ(name, "lxc.id_map") || !value->str) + /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ + if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str) return 0;
This one caused lxcconf2xmltest to fail and needs to change to:
/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ if (STRNEQ(name, "lxc.idmap") || !value->str) { if (!value->str || STRNEQ(name, "lxc.id_map")) return 0; }
The failure occurred because of the STRNEQ OR not being true (silly me on first pass not running the tests too ;-))
if (sscanf(value->str, "%c %lu %lu %lu", &type, &target, &start, &count) != 4) { - virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"), + virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),
Do you mind if I alter this to:
virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"), name, value->str);
That way the conf name string is provided like it was before
value->str); return -1; } @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config, if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0) goto error;
- if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0 || - VIR_STRDUP(vmdef->name, value) < 0) - goto error; + if (virConfGetValueString(properties, "lxc.uts.name", &value) <= 0) { + virResetLastError(); + + /* Check for pre LXC 3.0 legacy key */ + if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0) + goto error; + } +
I think in this case the @value needs to be restored... Previous if the GetValueString OR the VIR_STRDUP of the value was < 0, we go to error. Although I'm not quite sure how @value would be NULL so as to cause the subsequent line to be executed... In any case, copying @value needs to be done, so add:
if (VIR_STRDUP(vmdef->name, value) < 0) goto error;
Which I can add if you agree.
No problems, John. You can go ahead with the changes. I forgot too add VIR_STRDUP after checking the property. It was causing the test error.
With those changes,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
As a follow-up maybe adding/altering/updating the tests/lxcconf2xmldata to include both pre and post 3.0 type data would be a good thing.
Yes, I agree too. But only config files that don't have netowork settings. Version 3.X and higher have another syntax to configure network too. And it was not implemented yet. I'm planning to propose this feature in the future.
[...]
On 11/11/18 12:46 PM, Julio Faracco wrote:
Em sáb, 10 de nov de 2018 às 11:17, John Ferlan <jferlan@redhat.com> escreveu:
On 11/9/18 12:30 PM, Julio Faracco wrote:
This patch introduce the new settings for LXC 3.0 or higher. The older versions keep the compatibility to deprecated settings for LXC, but after release 3.0, the compatibility was removed. This commit adds the support to the refactored settings.
v1-v2: Michal's suggestions to handle differences between versions. v2-v3: Adding suggestions from Pino and John too.
These type of comments would go below the --- below so that they're not part of commit history...
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index cbdec723dd..d3ba12bb0e 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c
[...]
@@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr value, void *data) char type; unsigned long start, target, count;
- if (STRNEQ(name, "lxc.id_map") || !value->str) + /* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ + if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") || !value->str) return 0;
This one caused lxcconf2xmltest to fail and needs to change to:
/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */ if (STRNEQ(name, "lxc.idmap") || !value->str) { if (!value->str || STRNEQ(name, "lxc.id_map")) return 0; }
The failure occurred because of the STRNEQ OR not being true (silly me on first pass not running the tests too ;-))
if (sscanf(value->str, "%c %lu %lu %lu", &type, &target, &start, &count) != 4) { - virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"), + virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),
Do you mind if I alter this to:
virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"), name, value->str);
That way the conf name string is provided like it was before
value->str); return -1; } @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config, if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0) goto error;
- if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0 || - VIR_STRDUP(vmdef->name, value) < 0) - goto error; + if (virConfGetValueString(properties, "lxc.uts.name", &value) <= 0) { + virResetLastError(); + + /* Check for pre LXC 3.0 legacy key */ + if (virConfGetValueString(properties, "lxc.utsname", &value) <= 0) + goto error; + } +
I think in this case the @value needs to be restored... Previous if the GetValueString OR the VIR_STRDUP of the value was < 0, we go to error. Although I'm not quite sure how @value would be NULL so as to cause the subsequent line to be executed... In any case, copying @value needs to be done, so add:
if (VIR_STRDUP(vmdef->name, value) < 0) goto error;
Which I can add if you agree.
No problems, John. You can go ahead with the changes. I forgot too add VIR_STRDUP after checking the property. It was causing the test error.
With those changes,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
As a follow-up maybe adding/altering/updating the tests/lxcconf2xmldata to include both pre and post 3.0 type data would be a good thing.
Yes, I agree too. But only config files that don't have netowork settings. Version 3.X and higher have another syntax to configure network too. And it was not implemented yet. I'm planning to propose this feature in the future.
[...]
Since you have access to the V3.0 environment, perhaps it's best that you update the patch based on my comments and also add the test *.config files using the v3 syntax. John
On Fri, Nov 09, 2018 at 03:30:59PM -0200, Julio Faracco wrote:
This patch introduce the new settings for LXC 3.0 or higher. The older versions keep the compatibility to deprecated settings for LXC, but after release 3.0, the compatibility was removed. This commit adds the support to the refactored settings.
v1-v2: Michal's suggestions to handle differences between versions. v2-v3: Adding suggestions from Pino and John too.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-)
I'd expect to additions to the test suite to cover these changes eg lxcconf2xmltest data files Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/12/18 6:13 AM, Daniel P. Berrangé wrote:
On Fri, Nov 09, 2018 at 03:30:59PM -0200, Julio Faracco wrote:
This patch introduce the new settings for LXC 3.0 or higher. The older versions keep the compatibility to deprecated settings for LXC, but after release 3.0, the compatibility was removed. This commit adds the support to the refactored settings.
v1-v2: Michal's suggestions to handle differences between versions. v2-v3: Adding suggestions from Pino and John too.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/lxc/lxc_native.c | 45 +++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) I'd expect to additions to the test suite to cover these changes eg lxcconf2xmltest data files
Actually, I was just going to send mail saying that this patch breaks the *existing* lxcfonc2xml tests. (run "make check" and you'll see the failure, then run "VIR_TEST_DEBUG=1 tests/lxcconf2xml" to see more details about the failure - it's getting the name of the new domain wrong)
participants (4)
-
Daniel P. Berrangé -
John Ferlan -
Julio Faracco -
Laine Stump