On Tue, Oct 8, 2013 at 12:48 PM, Klaus Aehlig <[email protected]> wrote:

> On Tue, Oct 08, 2013 at 12:24:34PM +0200, Helga Velroyen wrote:
> > This downgrades the cluster's hypervisor parameters for the
> > xen hypervisors with respect to the 'xen_cmd' attribute.
> > A unit tests is provided.
> >
> > Signed-off-by: Helga Velroyen <[email protected]>
> > ---
> >  test/py/cfgupgrade_unittest.py | 13 +++++++++++++
> >  tools/cfgupgrade               | 15 +++++++++++++++
> >  2 files changed, 28 insertions(+)
>
>
> > +def DowngradeHvparams(config_data):
> > +  """Downgrade the cluster's hypervisor parameters."""
> > +  cluster = config_data["cluster"]
> > +  if "hvparams" in cluster:
> > +    hvparams = cluster["hvparams"]
> > +    xen_params = None
> > +    for xen_variant in [constants.HT_XEN_PVM, constants.HT_XEN_HVM]:
> > +      if xen_variant in hvparams:
> > +        xen_params = hvparams[xen_variant]
> > +      # 'xen_cmd' was introduced in 2.9
> > +      if constants.HV_XEN_CMD in xen_params:
> > +        del xen_params[constants.HV_XEN_CMD]
>
> Here I'm a bit puzzled about the logic. I would have expected to
> only inspect the  xen_params if you found them for the xen_variant
> you're looking at. I mean, it works (ans xen_params is properly
> initialized), but still, wouldn't it be nices, if this last if-statement
> was indent one level more?
>

Oh, right, I think I had it right before, but then messed up the
indentation at some point. Interdiff: (for the whole series, I will
distribute the change on the two last patches)

diff --git a/tools/cfgupgrade b/tools/cfgupgrade
index b4bcfc1..a7be848 100755
--- a/tools/cfgupgrade
+++ b/tools/cfgupgrade
@@ -426,12 +426,12 @@ def DowngradeHvparams(config_data):
     for xen_variant in [constants.HT_XEN_PVM, constants.HT_XEN_HVM]:
       if xen_variant in hvparams:
         xen_params = hvparams[xen_variant]
-      # 'xen_cmd' was introduced in 2.9
-      if constants.HV_XEN_CMD in xen_params:
-        del xen_params[constants.HV_XEN_CMD]
-      # 'vif_script' was introducted in 2.9
-      if constants.HV_VIF_SCRIPT in xen_params:
-        del xen_params[constants.HV_VIF_SCRIPT]
+        # 'xen_cmd' was introduced in 2.9
+        if constants.HV_XEN_CMD in xen_params:
+          del xen_params[constants.HV_XEN_CMD]
+        # 'vif_script' was introducted in 2.9
+        if constants.HV_VIF_SCRIPT in xen_params:
+          del xen_params[constants.HV_VIF_SCRIPT]


 def DowngradeAll(config_data):

Cheers,
helga



>
> Rest LGTM.
>
> --
> Klaus Aehlig
> Google Germany GmbH, Dienerstr. 12, 80331 Muenchen
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores
>



-- 
-- 
Helga Velroyen | Software Engineer | [email protected] |

Google Germany GmbH
Dienerstr. 12
80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

Reply via email to