Re: [libvirt] [PATCH v2] lxc: Include support to lxc version 3.0 or higher.

2018-11-06 Thread John Ferlan



On 11/5/18 1:57 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.
> 
> Signed-off-by: Julio Faracco 
> ---
>  src/lxc/lxc_native.c | 18 +-
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index cbdec723dd..8604cbaf46 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -200,7 +200,9 @@ lxcSetRootfs(virDomainDefPtr def,
>  int type = VIR_DOMAIN_FS_TYPE_MOUNT;
>  VIR_AUTOFREE(char *) value = NULL;
>  
> -if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
> +if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0 &&
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0)
>  return -1;

So "integrating" Pino's v2 comment and Michal's RFC/v1 comment plus some
error cleansing:

if (virConfGetValueString(properties, "lxc.rootfs.path", ) <= 0) {
virResetLastError();

/* Check for pre LXC 3.0 legacy key */
if (virConfGetValueString(properties, "lxc.rootfs", ) <= 0)
return -1;
}

>  
>  if (STRPREFIX(value, "/dev/"))
> @@ -684,7 +686,9 @@ lxcCreateConsoles(virDomainDefPtr def, virConfPtr 
> properties)
>  virDomainChrDefPtr console;
>  size_t i;
>  
> -if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
> +if (virConfGetValueString(properties, "lxc.tty", ) <= 0 &&
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValueString(properties, "lxc.tty.max", ) <= 0)
>  return 0;

if (virConfGetValueString(properties, "lxc.tty.max", ) <= 0) {
virResetLastError();

/* Check for pre LXC 3.0 legacy key */
if (virConfGetValueString(properties, "lxc.tty", ) <= 0)
return 0;
}


Although it could be argued that return 0 should also have a reset last error.

>  
>  if (virStrToLong_i(value, NULL, 10, ) < 0) {
> @@ -724,7 +728,7 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr 
> value, void *data)
>  char type;
>  unsigned long start, target, count;
>  
> -if (STRNEQ(name, "lxc.id_map") || !value->str)
> +if (STRNEQ(name, "lxc.id_map") || STRNEQ(name, "lxc.idmap") || 
> !value->str)
>  return 0;
>  

/* 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;

For this one, not only reverse the order, but also fix the error message:

virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"),
   name, value->str);

where name will be either "lxc.id_map" or "lxc.idmap"

>  if (sscanf(value->str, "%c %lu %lu %lu", ,
> @@ -1041,7 +1045,9 @@ lxcParseConfigString(const char *config,
>  if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
>  goto error;
>  
> -if (virConfGetValueString(properties, "lxc.utsname", ) <= 0 ||
> +if ((virConfGetValueString(properties, "lxc.utsname", ) <= 0 &&
> + /* Legacy config keys were removed after release 3.0. */
> + virConfGetValueString(properties, "lxc.uts.name", ) <= 0) ||
>  VIR_STRDUP(vmdef->name, value) < 0)
>  goto error;

if (virConfGetValueString(properties, "lxc.uts.name", ) <= 0) {
virResetLastError();

/* Check for pre LXC 3.0 legacy key */
if (virConfGetValueString(properties, "lxc.utsname", ) <= 0)
goto error;
}

if (VIR_STRDUP(vmdef->name, value) < 0)
goto error;


>  if (!vmdef->name && (VIR_STRDUP(vmdef->name, "unnamed") < 0))
> @@ -1051,7 +1057,9 @@ lxcParseConfigString(const char *config,
>  goto error;
>  
>  /* Look for fstab: we shouldn't have it */
> -if (virConfGetValue(properties, "lxc.mount")) {
> +if (virConfGetValue(properties, "lxc.mount") ||
> +/* Legacy config keys were removed after release 3.0. */
> +virConfGetValue(properties, "lxc.mount.fstab")) {
>  virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> _("lxc.mount found, use lxc.mount.entry lines 
> instead"));
>  goto error;
> 

This one's interesting because what does it mean if someone does use 
"lxc.mount.fstab"?
Should they be using lxc.mount.entry instead?  I assume this is what you want:

/* 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.fstab or 

Re: [libvirt] [PATCH v2] lxc: Include support to lxc version 3.0 or higher.

2018-11-05 Thread Pino Toscano
On Monday, 5 November 2018 19:57:24 CET 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.

Shouldn't it be better to check first for the new names of the
configuration strings, using the pre-3.0 ones as fallback option?

-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list