* Michele Tartara <[email protected]> [2014-02-19 15:51:56 +0100]: > On Sun, Feb 16, 2014 at 12:33 AM, Dimitris Aragiorgis <[email protected]> wrote: > > * Michele Tartara <[email protected]> [2014-02-13 16:20:22 +0100]: > > > >> On Thu, Feb 13, 2014 at 3:46 PM, Dimitris Aragiorgis <[email protected]> > >> wrote: > >> > During backup import/export SafeConfigParser() is used to > >> > save/restore instance's configuration. There is a possibility if an > >> > export is done with a different Ganeti version, a specific value not > >> > to be saved during export (e.g. the NIC/Disk name) but still > >> > requested during import. > >> > > >> > With this patch we override the get() method of SafeConfigParser() > >> > and catch NoOptionError if raised and return None. Additionally we > >> > translate "None" values read from .ini file into None. > >> > > >> > Signed-off-by: Dimitris Aragiorgis <[email protected]> > >> > --- > >> > lib/objects.py | 15 ++++++++++++++- > >> > 1 file changed, 14 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/lib/objects.py b/lib/objects.py > >> > index 358f108..6aca6fc 100644 > >> > --- a/lib/objects.py > >> > +++ b/lib/objects.py > >> > @@ -2159,7 +2159,8 @@ class Network(TaggableObject): > >> > return obj > >> > > >> > > >> > -class SerializableConfigParser(ConfigParser.SafeConfigParser): > >> > +# need to inherit object in order to use super() > >> > +class SerializableConfigParser(ConfigParser.SafeConfigParser, object): > >> > """Simple wrapper over ConfigParse that allows serialization. > >> > > >> > This class is basically ConfigParser.SafeConfigParser with two > >> > @@ -2181,6 +2182,18 @@ class > >> > SerializableConfigParser(ConfigParser.SafeConfigParser): > >> > cfp.readfp(buf) > >> > return cfp > >> > > >> > + def get(self, section, option, **kwargs): > >> > + value = None > >> > + try: > >> > + value = super(SerializableConfigParser, self).get(section, option, > >> > + **kwargs) > >> > + if value.lower() == constants.VALUE_NONE: > >> > + value = None > >> > + except ConfigParser.NoOptionError: > >> > + pass > >> > + > >> > + return value > >> > + > >> > > >> > class LvmPvInfo(ConfigObject): > >> > """Information about an LVM physical volume (PV). > >> > -- > >> > 1.7.10.4 > >> > > >> > >> I think we should at least log that an error happened. Silently > >> accepting it seems to me extremely dangerous. > >> > > > > Sure. Maybe "extremely" is too much here, but ok :) > > > >> Also, I'm not confident enough about this part of the code to say that > >> all the values can be optional if not found. So, what about > >> specifically checking whether we have been looking for the disk name > >> and nick name and ONLY in that case returning None instead of the > >> exception? > > > > Well I think this can get kinda ugly. What if another option gets added or > > removed in the future? Are we going to add another if: then: else? > > Yes. Actually I was thinking more of a: > if option in ["disk", "nic", ...]: > pass > else: > raise again the exception >
How about this interdiff:
diff --git a/lib/objects.py b/lib/objects.py
index 6aca6fc..8b38e81 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -2190,7 +2190,12 @@ class
SerializableConfigParser(ConfigParser.SafeConfigParser, object):
if value.lower() == constants.VALUE_NONE:
value = None
except ConfigParser.NoOptionError:
- pass
+ r = re.compile(r"(disk|nic)\d+_name")
+ match = r.match(option)
+ if match:
+ pass
+ else:
+ raise
return value
> >
> >> This way, we know that we are modifying only the behavior related to
> >> the new configuration fields.
> >>
> >
> > What is actually new? Name is a NIC/Disk parameter since 2.7...
>
> Of course. I meant it as "newly exported". It's not a new parameter per se.
>
> >
> > How about adding just the logging?
>
> Right now we have a really strict checking on the options. Allowing a
> specific set of them to be optional is ok, but suddenly turning off
> any check on the options is not a good thing. Also, it's not like we
> are going to have hundreds of optional ones, so the list is still
> going to be really small and updated only rarely.
>
>
> >
> > Thanks,
> > dimara
> >
> >> Cheers,
> >> Michele
> >>
>
> Thanks,
> Michele
>
> --
> 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
signature.asc
Description: Digital signature
