Xen devel - Feb 2013 - [PATCH] tools/xend: Only add cpuid and cpuid_check to sexpr once

# HG changeset patch
# User Jim Fehlig <jfehlig@suse.com>
# Date 1360861948 -3600
# Node ID 0f9c7503650fa1b1103b769e1129d66ff614b2ad
# Parent  cffb489a6df37d8d114e7d2d53a7a85d14e8f968
tools/xend: Only add cpuid and cpuid_check to sexpr once

When converting a XendConfig object to sexpr, cpuid and cpuid_check
were being emitted twice in the resulting sexpr.  The first conversion
writes incorrect sexpr, causing parsing of the sexpr to fail when xend
is restarted and domain sexpr files in /var/lib/xend/domains/<dom-uuid>
are read and parsed.

This patch skips the first conversion, and uses only the custom
cpuid{_check} conversion methods called later.  It is not pretty, but
is the least invasive fix in this complex code.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r cffb489a6df3 -r 0f9c7503650f tools/python/xen/xend/XendConfig.py
--- a/tools/python/xen/xend/XendConfig.py
+++ b/tools/python/xen/xend/XendConfig.py
@@ -1124,6 +1124,10 @@ class XendConfig(dict):
         else:
             for name, typ in XENAPI_CFG_TYPES.items():
                 if name in self and self[name] not in (None, []):
+                    # Skip cpuid and cpuid_check.  Custom conversion
+                    # methods for these are called below.
+                    if name in ("cpuid", "cpuid_check"):
+                        continue
                     if typ == dict:
                         s = self[name].items()
                     elif typ == list:

Olaf Hering writes ("[Xen-devel] [PATCH] tools/xend: Only add cpuid and
cpuid_check to sexpr once"):
> tools/xend: Only add cpuid and cpuid_check to sexpr once
> 
> When converting a XendConfig object to sexpr, cpuid and cpuid_check
> were being emitted twice in the resulting sexpr.  The first conversion
> writes incorrect sexpr, causing parsing of the sexpr to fail when xend
> is restarted and domain sexpr files in
/var/lib/xend/domains/<dom-uuid>
> are read and parsed.
> 
> This patch skips the first conversion, and uses only the custom
> cpuid{_check} conversion methods called later.  It is not pretty, but
> is the least invasive fix in this complex code.
We do intend to fix bugs in xend, but I''m worried that this change
might break something.  I don''t know the code at all so forgive me if
I''m asking stupid questions, but why does the first conversion emit an
invalid sexpr ?
>              for name, typ in XENAPI_CFG_TYPES.items():
>                  if name in self and self[name] not in (None, []):
> +                    # Skip cpuid and cpuid_check.  Custom conversion
> +                    # methods for these are called below.
> +                    if name in ("cpuid",
"cpuid_check"):
> +                        continue
I do agree that this looks plausible because as your comment says, the
cpuid and cpuid_check values are converted explicitly later.

Thanks,
Ian.

On Tue, Feb 19, Ian Jackson wrote:
> Olaf Hering writes ("[Xen-devel] [PATCH] tools/xend: Only add cpuid
and cpuid_check to sexpr once"):
> > tools/xend: Only add cpuid and cpuid_check to sexpr once
> > 
> > When converting a XendConfig object to sexpr, cpuid and cpuid_check
> > were being emitted twice in the resulting sexpr.  The first conversion
> > writes incorrect sexpr, causing parsing of the sexpr to fail when xend
> > is restarted and domain sexpr files in
/var/lib/xend/domains/<dom-uuid>
> > are read and parsed.
> > 
> > This patch skips the first conversion, and uses only the custom
> > cpuid{_check} conversion methods called later.  It is not pretty, but
> > is the least invasive fix in this complex code.
> 
> We do intend to fix bugs in xend, but I''m worried that this change
> might break something.  I don''t know the code at all so forgive me
if
> I''m asking stupid questions, but why does the first conversion
emit an
> invalid sexpr ?
The result of having cpuid= in the domU.cfg and doing a rcxend restart is
that the sxp string is not properly converted into a list and dict(?),
instead the string is partly passed as is. xend will reject the sxp and
the domU disappears until its readded with xm new.

This change has been used almost a year now in production, no errors
were reported.

Olaf

Olaf Hering writes ("Re: [Xen-devel] [PATCH] tools/xend: Only add cpuid and
cpuid_check to sexpr once"):
> The result of having cpuid= in the domU.cfg and doing a rcxend restart is
> that the sxp string is not properly converted into a list and dict(?),
> instead the string is partly passed as is. xend will reject the sxp and
> the domU disappears until its readded with xm new.
> 
> This change has been used almost a year now in production, no errors
> were reported.
Thanks for the reassurance, I have applied the patch.

Ian.