[Kimchi-devel] [PATCH] Addition of custom mac address field for interfaces v3

[Crístian Viana]

Hi Brent,

We have a set of commands to check whether the code has the right syntax
according to our standards (e.g. Python PEP8, etc.). You can run it by
typing "make check-local". I tried it after applying your patch and I
got the following error:


bad i18n string formatting:
  KCHVMIF0010E: The custom MAC address %(value) is already in use on this guest


You need to provide a formatting mask (e.g. "%(value)s") when using a
variable inside an internationalized string.

I also launched Kimchi after applying this patch and I tried to add a
few new NICs with custom MAC addresses. I tried "52:54:00:41:65:a1" and
it worked fine. However, when I tried "ab:cd:ef:ab:cd:ef", an unexpected
exception was raised:


[01/Oct/2014:16:14:47] HTTP Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 656, in respond
    response.body = self.handler()
  File "/usr/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 188, in __call__
    self.body = self.oldhandler(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/cherrypy/_cpdispatch.py", line 34, in __call__
    return self.callable(*self.args, **self.kwargs)
  File "/home/vianac/LTC/kimchi/src/kimchi/control/base.py", line 310, in index
    return self.create(parse_request(), *args)
  File "/home/vianac/LTC/kimchi/src/kimchi/control/base.py", line 242, in create
    name = create(*args)
  File "/home/vianac/LTC/kimchi/src/kimchi/model/vmifaces.py", line 88, in create
    validMac(mac)
  File "/home/vianac/LTC/kimchi/src/kimchi/model/vmifaces.py", line 61, in validMac
    raise InvalidParameter("KCHVMIF0011E", {'custommac': mac})
  File "/home/vianac/LTC/kimchi/src/kimchi/exception.py", line 49, in __init__
    msg = unicode(msg, 'utf-8') % args
KeyError: u'value'


The exception above is also raised when we run our test suite (i.e. by
typing "sudo make check").

And I have one last comment about the code, which is below:

On Qua, 2014-10-01 at 10:03 -0500, bbaude at redhat.com wrote:

> +            mac = inst.vmifaces_create('kimchi-ifaces', iface_args)
> +            iface = inst.vmiface_lookup('kimchi-ifaces', mac)
> +            self.assertEquals("52:54:00:17:cd:e7", iface['mac'])
> +            self.assertRaises(InvalidParameter, inst.vmifaces_create,
> +                              'kimchi-ifaces', iface_args)
> +            rollback.prependDefer(inst.vmiface_delete, 'kimchi-ifaces', mac)


It's a good practice to add a delete rollback action as soon as the
referred object is created. That means that the call to "prependDefer"
would be better placed right after "vmifaces_create". Using the code
above, however, if the call to "vmiface_lookup" somehow fails, the
network interface won't be removed by the transaction.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/kimchi-devel/attachments/20141001/8196a7e1/attachment.html>