Code review comment for lp:~yamahata/nova/boot-from-volume-0

On Wed, Jun 08, 2011 at 06:27:26PM -0000, Rick Harris wrote:
> Review: Needs Fixing
> Very impressive work! Just a few small nits:
>
>
> Received a test failure:
>
> ======================================================================
> FAIL: test_stop_with_attached_volume (nova.tests.test_cloud.CloudTestCase)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/rick/openstack/nova/boot-from-volume-0/nova/tests/test_cloud.py", line 691, in test_stop_with_attached_volume
> self._assert_volume_attached(vol, instance_id, '/dev/vdc')
> File "/home/rick/openstack/nova/boot-from-volume-0/nova/tests/test_cloud.py", line 582, in _assert_volume_attached
> self.assertEqual(vol['mountpoint'], mountpoint)
> AssertionError: u'\\/dev\\/vdc' != '/dev/vdc'

Hmm, the test passes for me. I'm using sqlite3 for unittest.
I can't find the code to escape '/' into '\\/'. MySQL?

> > 315 + block_device_mapping=[]):
>
> Usually not a good idea to use a list as a default argument. This is because
> the list-object is created at /function definition/ time and the same list
> object will be re-used on each invocation--probably not what you wanted.
>
> Instead, it's better to default to None and initialize a new list in the
> function's body:
>
> block_device_mapping=None):
>
> block_device_mapping = block_device_mapping or []
>
> OR....
>
> if not block_device_mapping:
> block_device_mapping = []

Okay, fixed.
During the fixes, I found other suspicious code.
Since I'm not sure they are intentional or not at a glance,
so please review the attached patch.

> > 393 + if not _is_able_to_shutdown(instance, instance_id):
> > 394 + return
>
> Should we log here that we weren't able to shutdown, something like:
>
> LOG.warn(_("Unable to shutdown server...."))

Yes, _is_able_to_shutdown() itself does.

=== modified file 'nova/objectstore/s3server.py'
--- nova/objectstore/s3server.py 2011-03-24 23:38:31 +0000
+++ nova/objectstore/s3server.py 2011-06-15 05:54:21 +0000
@@ -155,7 +155,8 @@ class BaseRequestHandler(wsgi.Controller
         self.finish('<?xml version="1.0" encoding="UTF-8"?>\n' +
                     ''.join(parts))

- def _render_parts(self, value, parts=[]):
+ def _render_parts(self, value, parts=None):
+ parts = parts or []
         if isinstance(value, basestring):
             parts.append(utils.xhtml_escape(value))
         elif isinstance(value, int) or isinstance(value, long):

=== modified file 'tools/ajaxterm/qweb.py'
--- tools/ajaxterm/qweb.py 2010-09-18 02:08:22 +0000
+++ tools/ajaxterm/qweb.py 2011-06-15 05:57:36 +0000
@@ -726,7 +726,7 @@ class QWebHtml(QWebXml):
 #----------------------------------------------------------
 # QWeb Simple Controller
 #----------------------------------------------------------
-def qweb_control(self,jump='main',p=[]):
+def qweb_control(self,jump='main',p=None):
     """ qweb_control(self,jump='main',p=[]):
     A simple function to handle the controler part of your application. It
     dispatch the control to the jump argument, while ensuring that prefix
@@ -739,6 +739,7 @@ def qweb_control(self,jump='main',p=[]):
     name1_name2_name3

     """
+ p = p or []
     jump=jump.replace('/','_').strip('_')
     if not hasattr(self,jump):
         return 0

=== modified file 'tools/esx/guest_tool.py'
--- tools/esx/guest_tool.py 2011-04-18 20:53:09 +0000
+++ tools/esx/guest_tool.py 2011-06-15 05:57:11 +0000
@@ -275,7 +275,8 @@ def _filter_duplicates(all_entries):
     return final_list

-def _set_rhel_networking(network_details=[]):
+def _set_rhel_networking(network_details=None):
+ network_details = network_details or []
     all_dns_servers = []
     for network_detail in network_details:
         mac_address, ip_address, subnet_mask, gateway, broadcast,\

--
yamahata