On Mon, Jun 29, 2009 at 02:07:29PM +0100, Guido Trotter wrote: > > If an ident member of an IdentKeyVal relationship starts with no_ or -, > handle it the same way we do for a key. Some unittests are added to > check that check_ident_key_val behaves as expected. > > This patch also changes ForceDictType to, for now, fail on such an > entry, and the same to happen when creating an instance or modifying its > nics or disks. > > This behavior will be used later on to allow deletion of os entries in > os parameters.
OK, I understand -debootstrap, but is no_deboostrap different? > > Signed-off-by: Guido Trotter <[email protected]> > --- > lib/cli.py | 16 +++++++++++++--- > lib/cmdlib.py | 5 +++++ > lib/utils.py | 4 ++++ > scripts/gnt-backup | 6 +++++- > scripts/gnt-instance | 6 +++++- > test/ganeti.cli_unittest.py | 17 +++++++++++++++++ > 6 files changed, 49 insertions(+), 5 deletions(-) > > diff --git a/lib/cli.py b/lib/cli.py > index f4f32db..d9bfb59 100644 > --- a/lib/cli.py > +++ b/lib/cli.py > @@ -54,6 +54,8 @@ __all__ = ["DEBUG_OPT", "NOHDR_OPT", "SEP_OPT", > "GenericMain", > "GetOnlineNodes", "JobExecutor", "SYNC_OPT", "CONFIRM_OPT", > ] > > +NO_PREFIX = "no_" > +UN_PREFIX = "-" > > def _ExtractTagsObject(opts, args): > """Extract the tag type object. > @@ -256,8 +258,6 @@ def _SplitKeyVal(opt, data): > @raises errors.ParameterError: if there are duplicate keys > > """ > - NO_PREFIX = "no_" > - UN_PREFIX = "-" > kv_dict = {} > if data: > for elem in data.split(","): > @@ -282,9 +282,19 @@ def check_ident_key_val(option, opt, value): > > """ > if ":" not in value: > - retval = (value, {}) > + ident, rest = value, '' > else: > ident, rest = value.split(":", 1) > + > + if ident.startswith(NO_PREFIX): > + if rest: > + raise errors.ParameterError("Can't list members when negating an > ident") "No options accepted"? We don't list anything here :) > + retval = (ident[len(NO_PREFIX):], False) > + elif ident.startswith(UN_PREFIX): > + if rest: > + raise errors.ParameterError("Can't list members when negating an > ident") Same. > + retval = (ident[len(UN_PREFIX):], None) > + else: > kv_dict = _SplitKeyVal(opt, rest) > retval = (ident, kv_dict) > return retval > diff --git a/lib/cmdlib.py b/lib/cmdlib.py > index 03660c1..fdebd42 100644 > --- a/lib/cmdlib.py > +++ b/lib/cmdlib.py > @@ -5887,6 +5887,9 @@ class LUSetInstanceParams(LogicalUnit): > else: > if not isinstance(disk_op, int): > raise errors.OpPrereqError("Invalid disk index") > + if not isinstance(disk_dict, dict): > + raise errors.OpPrereqError("Invalid disk values dict") "Expected dictionary, got '%s'" % str(disk_dict) (it's not a dict, therefore it can't be an invalid dict) > + > if disk_op == constants.DDM_ADD: > mode = disk_dict.setdefault('mode', constants.DISK_RDWR) > if mode not in constants.DISK_ACCESS_SET: > @@ -5921,6 +5924,8 @@ class LUSetInstanceParams(LogicalUnit): > else: > if not isinstance(nic_op, int): > raise errors.OpPrereqError("Invalid nic index") > + if not isinstance(nic_dict, dict): > + raise errors.OpPrereqError("Invalid nic values dict") same here > > # nic_dict should be a dict > nic_ip = nic_dict.get('ip', None) > diff --git a/lib/utils.py b/lib/utils.py > index 44bd0e7..543073d 100644 > --- a/lib/utils.py > +++ b/lib/utils.py > @@ -388,6 +388,10 @@ def ForceDictType(target, key_types, > allowed_values=None): > if allowed_values is None: > allowed_values = [] > > + if not isinstance(target, dict): > + msg = "Invalid dictionary '%s'" % target > + raise errors.TypeEnforcementError(msg) same here > for key in target: > if key not in key_types: > msg = "Unknown key '%s'" % key > diff --git a/scripts/gnt-backup b/scripts/gnt-backup > index e78abba..d279458 100755 > --- a/scripts/gnt-backup > +++ b/scripts/gnt-backup > @@ -104,6 +104,8 @@ def ImportInstance(opts, args): > nics = [{}] * nic_max > for nidx, ndict in opts.nics.items(): > nidx = int(nidx) > + if not isinstance(ndict, dict): > + raise errors.OpPrereqError("Invalid nic dict for nic %d" % nidx) same here > nics[nidx] = ndict > elif opts.no_nics: > # no nics > @@ -132,7 +134,9 @@ def ImportInstance(opts, args): > disks = [{}] * disk_max > for didx, ddict in opts.disks: > didx = int(didx) > - if "size" not in ddict: > + if not isinstance(ddict, dict): > + raise errors.OpPrereqError("Invalid disk dict for disk %d" % didx) same here > + elif "size" not in ddict: > raise errors.OpPrereqError("Missing size for disk %d" % didx) > try: > ddict["size"] = utils.ParseUnit(ddict["size"]) > diff --git a/scripts/gnt-instance b/scripts/gnt-instance > index 12152e8..e785409 100755 > --- a/scripts/gnt-instance > +++ b/scripts/gnt-instance > @@ -302,6 +302,8 @@ def AddInstance(opts, args): > nics = [{}] * nic_max > for nidx, ndict in opts.nics: > nidx = int(nidx) > + if not isinstance(ndict, dict): > + raise errors.OpPrereqError("Invalid nic dict for nic %d" % nidx) same here > nics[nidx] = ndict > elif opts.no_nics: > # no nics > @@ -330,7 +332,9 @@ def AddInstance(opts, args): > disks = [{}] * disk_max > for didx, ddict in opts.disks: > didx = int(didx) > - if "size" not in ddict: > + if not isinstance(ddict, dict): > + raise errors.OpPrereqError("Invalid disk dict for disk %d" % didx) same here > + elif "size" not in ddict: > raise errors.OpPrereqError("Missing size for disk %d" % didx) > try: > ddict["size"] = utils.ParseUnit(ddict["size"]) > diff --git a/test/ganeti.cli_unittest.py b/test/ganeti.cli_unittest.py > index b86486b..cf3810d 100755 > --- a/test/ganeti.cli_unittest.py > +++ b/test/ganeti.cli_unittest.py > @@ -79,6 +79,23 @@ class TestSplitKeyVal(unittest.TestCase): > """Test how we handle splitting an empty string""" > self.failUnlessEqual(cli._SplitKeyVal("option", ""), {}) > > +class TestIdentKeyVal(unittest.TestCase): > + """Testing case for cli.check_ident_key_val""" > + > + def testIdentKeyVal(self): > + """Test identkeyval""" > + def cikv(value): > + return cli.check_ident_key_val("option", "opt", value) > + > + self.assertEqual(cikv("foo:bar"), ("foo", {"bar": True})) > + self.assertEqual(cikv("foo:bar=baz"), ("foo", {"bar": "baz"})) > + self.assertEqual(cikv("bar:b=c,c=a"), ("bar", {"b": "c", "c": "a"})) > + self.assertEqual(cikv("no_bar"), ("bar", False)) > + self.assertRaises(ParameterError, cikv, "no_bar:foo") > + self.assertRaises(ParameterError, cikv, "no_bar:foo=baz") > + self.assertEqual(cikv("-foo"), ("foo", None)) > + self.assertRaises(ParameterError, cikv, "-foo:a=c") > + iustin
