[ovs-dev] [python idl 12/16] ovs.ovsuuid: Get rid of ovs.ovsuuid.UUID class.

Reid Price reid at nicira.com
Tue Sep 20 23:55:36 UTC 2011


Some notes inline.  One typo.

On Mon, Sep 19, 2011 at 11:18 AM, Ben Pfaff <blp at nicira.com> wrote:

> This class only caused unnecessary confusion.  This commit changes all of
> its methods into top-level functions.
> ---
>  python/ovs/db/data.py  |    6 ++--
>  python/ovs/db/idl.py   |    4 +-
>  python/ovs/db/types.py |    2 +-
>  python/ovs/ovsuuid.py  |   80
> +++++++++++++++++++++--------------------------
>  tests/test-ovsdb.py    |    2 +-
>  5 files changed, 43 insertions(+), 51 deletions(-)
>
> diff --git a/python/ovs/db/data.py b/python/ovs/db/data.py
> index c07d8bf..15a7151 100644
> --- a/python/ovs/db/data.py
> +++ b/python/ovs/db/data.py
> @@ -96,7 +96,7 @@ class Atom(object):
>             or (type_ == ovs.db.types.StringType and type(json) in [str,
> unicode])):
>             atom = Atom(type_, json)
>         elif type_ == ovs.db.types.UuidType:
> -            atom = Atom(type_, ovs.ovsuuid.UUID.from_json(json, symtab))
> +            atom = Atom(type_, ovs.ovsuuid.from_json(json, symtab))
>         else:
>             raise error.Error("expected %s" % type_.to_string(), json)
>         atom.check_constraints(base)
> @@ -147,7 +147,7 @@ class Atom(object):
>
>     def to_json(self):
>         if self.type == ovs.db.types.UuidType:
> -            return self.value.to_json()
> +            return ovs.ovsuuid.to_json(self.value)
>         else:
>             return self.value
>
> @@ -165,7 +165,7 @@ class Atom(object):
>             return ['%s.string = xstrdup("%s");'
>                     % (var, escapeCString(self.value))]
>         elif self.type == ovs.db.types.UuidType:
> -            return self.value.cInitUUID(var)
> +            return ovs.ovsuuid.to_c_assignment(self.value, var)
>
>     def toEnglish(self, escapeLiteral=returnUnchanged):
>         if self.type == ovs.db.types.IntegerType:
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 65f6838..7841a89 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -194,12 +194,12 @@ class Idl:
>                                   'an object' % table_name, table_update)
>
>             for uuid_string, row_update in table_update.iteritems():
> -                if not ovs.ovsuuid.UUID.is_valid_string(uuid_string):
> +                if not ovs.ovsuuid.is_valid_string(uuid_string):
>                     raise error.Error('<table-update> for table "%s" '
>                                       'contains bad UUID "%s" as member '
>                                       'name' % (table_name, uuid_string),
>                                       table_update)
> -                uuid = ovs.ovsuuid.UUID.from_string(uuid_string)
> +                uuid = ovs.ovsuuid.from_string(uuid_string)
>

I'm getting enough deja vu to know I've said this before =/
If it wasn't for your dastardly-ly informative error messages, this would be
better-written as

  uuid = ovs.<...>.from_string(uuid_string)

and letting that function do the raising.  Fine as-is, of course.


>
>                 if type(row_update) != dict:
>                     raise error.Error('<table-update> for table "%s" '
> diff --git a/python/ovs/db/types.py b/python/ovs/db/types.py
> index dc19f85..9563211 100644
> --- a/python/ovs/db/types.py
> +++ b/python/ovs/db/types.py
> @@ -56,7 +56,7 @@ IntegerType = AtomicType("integer", 0)
>  RealType = AtomicType("real", 0.0)
>  BooleanType = AtomicType("boolean", False)
>  StringType = AtomicType("string", "")
> -UuidType = AtomicType("uuid", ovs.ovsuuid.UUID.zero())
> +UuidType = AtomicType("uuid", ovs.ovsuuid.zero())
>
>  ATOMIC_TYPES = [VoidType, IntegerType, RealType, BooleanType, StringType,
>                 UuidType]
> diff --git a/python/ovs/ovsuuid.py b/python/ovs/ovsuuid.py
> index 5d2785c..0776dd5 100644
> --- a/python/ovs/ovsuuid.py
> +++ b/python/ovs/ovsuuid.py
> @@ -18,57 +18,49 @@ import uuid
>  from ovs.db import error
>  import ovs.db.parser
>
> -class UUID(uuid.UUID):
> -    x = "[0-9a-fA-f]"
> -    uuidRE = re.compile("^(%s{8})-(%s{4})-(%s{4})-(%s{4})-(%s{4})(%s{8})$"
> -                        % (x, x, x, x, x, x))
> +uuidRE = re.compile("^(%s{8})-(%s{4})-(%s{4})-(%s{4})-(%s{4})(%s{8})$"
> +                    % (("[0-9a-fA-f]",) * 6))
>

Typo, second F should be capitalized.  Could also use the dictionary-style
string formatting, though this seems reasonable as-is.

  "^(%(hex)s{8})-(%(hex)s{4})- ... " % {'hex': "[0-9a-fA-F]"}


>
> -    def __init__(self, s):
> -        uuid.UUID.__init__(self, hex=s)
> +def zero():
> +    return uuid.UUID(int=0)
>
> -    @staticmethod
> -    def zero():
> -        return UUID('00000000-0000-0000-0000-000000000000')
> +def is_zero(uuid_):
> +    return uuid_.int == 0
>
> -    def is_zero(self):
> -        return self.int == 0
> +def is_valid_string(s):
> +    return uuidRE.match(s) is not None
>
> -    @staticmethod
> -    def is_valid_string(s):
> -        return UUID.uuidRE.match(s) != None
> +def from_string(s):
> +    if not is_valid_string(s):
> +        raise error.Error("%s is not a valid UUID" % s)
> +    return uuid.UUID(s)
>

I have a preference for the OO organizational style [I think u.is_zero()
makes more sense than ovs.ovsuuid.is_zero(u)], but I agree that these
changes are better than having a half-classed version that is mixing and
matching elsewhere.  If the long string were the problem, you can always do
"UUID = ovs.ovsuuid.UUID" up at the top.

Wanted to note that in general the errors provided by UUID itself are fairly
informative, though they lack the object, e.g.:

>>> import uuid
>>> u = uuid.uuid4()
>>> u
UUID('7862e9f1-6cd5-4922-9d21-6132b6c2bd23')
>>> str(u)
'7862e9f1-6cd5-4922-9d21-6132b6c2bd23'
>>> uuid.UUID(str(u))
UUID('7862e9f1-6cd5-4922-9d21-6132b6c2bd23')
>>> uuid.UUID(str(u)[:4])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/uuid.py", line 134, in __init__
    raise ValueError('badly formed hexadecimal UUID string')
ValueError: badly formed hexadecimal UUID string
>>> uuid.UUID(u)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/uuid.py", line 131, in __init__
    hex = hex.replace('urn:', '').replace('uuid:', '')
AttributeError: 'UUID' object has no attribute 'replace'



>
> -    @staticmethod
> -    def from_string(s):
> -        if not UUID.is_valid_string(s):
> -            raise error.Error("%s is not a valid UUID" % s)
> -        return UUID(s)
> -
> -    @staticmethod
> -    def from_json(json, symtab=None):
> +def from_json(json, symtab=None):
> +    try:
> +        s = ovs.db.parser.unwrap_json(json, "uuid", unicode)
> +        if not uuidRE.match(s):
> +            raise error.Error("\"%s\" is not a valid UUID" % s, json)
> +        return uuid.UUID(s)
> +    except error.Error, e:
> +        if not symtab:
> +            raise e
>         try:
> -            s = ovs.db.parser.unwrap_json(json, "uuid", unicode)
> -            if not UUID.uuidRE.match(s):
> -                raise error.Error("\"%s\" is not a valid UUID" % s, json)
> -            return UUID(s)
> -        except error.Error, e:
> -            if not symtab:
> -                raise e
> -            try:
> -                name = ovs.db.parser.unwrap_json(json, "named-uuid",
> unicode)
> -            except error.Error:
> -                raise e
> +            name = ovs.db.parser.unwrap_json(json, "named-uuid", unicode)
> +        except error.Error:
> +            raise e
>
> -            if name not in symtab:
> -                symtab[name] = uuid4()
> -            return symtab[name]
> +        if name not in symtab:
> +            symtab[name] = uuid4()
> +        return symtab[name]
>
> -    def to_json(self):
> -        return ["uuid", str(self)]
> +def to_json(uuid_):
> +    return ["uuid", str(uuid_)]
>
> -    def cInitUUID(self, var):
> -        m = UUID.uuidRE.match(str(self))
> -        return ["%s.parts[0] = 0x%s;" % (var, m.group(1)),
> -                "%s.parts[1] = 0x%s%s;" % (var, m.group(2), m.group(3)),
> -                "%s.parts[2] = 0x%s%s;" % (var, m.group(4), m.group(5)),
> -                "%s.parts[3] = 0x%s;" % (var, m.group(6))]
> +def to_c_assignment(uuid_, var):
> +    """Returns an array of strings, each of which contain a C statement.
>  The
> +    statements assign 'uuid_' to a "struct uuid" as defined in Open
> vSwitch
> +    lib/uuid.h."""
>
> +    hex = uuid_.hex
> +    return ["%s.parts[%d] = 0x%s;" % (var, x, hex[x * 8:(x + 1) * 8])
> +            for x in range(4)]
>

hex is a (rather convenient) builtin you might want to avoid shadowing, and
might even be able to use here (or just "0x%x"), didn't look beyond patch
context though.


> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
> index ea45f9a..1774dd9 100644
> --- a/tests/test-ovsdb.py
> +++ b/tests/test-ovsdb.py
> @@ -163,7 +163,7 @@ def substitute_uuids(json, symtab):
>     return json
>
>  def parse_uuids(json, symtab):
> -    if type(json) in [str, unicode] and
> ovs.ovsuuid.UUID.is_valid_string(json):
> +    if type(json) in [str, unicode] and ovs.ovsuuid.is_valid_string(json):
>         name = "#%d#" % len(symtab)
>         sys.stderr.write("%s = %s\n" % (name, json))
>         symtab[name] = json


  -Reid


> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20110920/5e20c5f1/attachment-0003.html>


More information about the dev mailing list