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