)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"80cd778a563faeb4e35a5676c72ad5bbd474b2d7","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Set the default bus to scsi when the guest architecture is ppc64"},{"line_number":10,"context_line":"and the hypervisor is kvm|qemu. Checks the arch from image_meta."},{"line_number":11,"context_line":"Fallback to host arch if arch is not present in the meta_image."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: I62d1490ee8c9ba45a087f934342f14386ddb7cb2"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"AAAAUH%2F%2BhbA%3D","line":11,"updated":"2014-01-24 19:44:35.000000000","message":"Assuming you meant image_meta here at the end.","commit_id":"38bbdea8d82fbc9679ad646cc5353148565daaac"},{"author":{"_account_id":6062,"name":"jichenjc","email":"jichenjc@cn.ibm.com","username":"jichenjc"},"change_message_id":"a19a8efc728ddb2b3d19bd4a44e6b07b6f573990","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Set the default bus to scsi when the guest architecture is ppc64"},{"line_number":10,"context_line":"and the hypervisor is kvm|qemu. Checks the arch from image_meta."},{"line_number":11,"context_line":"Fallback to host arch if arch is not present in the meta_image."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Change-Id: I62d1490ee8c9ba45a087f934342f14386ddb7cb2"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"AAAAUH%2F%2Bp6c%3D","line":12,"updated":"2014-01-23 14:20:43.000000000","message":"a bp or bug name is recommended","commit_id":"38bbdea8d82fbc9679ad646cc5353148565daaac"}],"nova/tests/virt/libvirt/test_libvirt_blockinfo.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"80cd778a563faeb4e35a5676c72ad5bbd474b2d7","unresolved":false,"context_lines":[{"line_number":591,"context_line":""},{"line_number":592,"context_line":"        with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":593,"context_line":"                               \u0027get_arch\u0027,"},{"line_number":594,"context_line":"                               return_value\u003d\u0027ppc64\u0027):"},{"line_number":595,"context_line":"            bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027)"},{"line_number":596,"context_line":"            self.assertEqual(bus, \u0027scsi\u0027)"},{"line_number":597,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2BhY8%3D","line":594,"updated":"2014-01-24 19:44:35.000000000","message":"Suggestion: your code checks for ppc and ppc64, but your test only covers pp64.  Since you have two checks here, I\u0027d suggest making one of them use \u0027ppc\u0027 and the other use \u0027ppc64\u0027 so both cases are covered.","commit_id":"38bbdea8d82fbc9679ad646cc5353148565daaac"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2f19219a72bd9f00608c4cb70d680add8cce107d","unresolved":false,"context_lines":[{"line_number":595,"context_line":"            bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027)"},{"line_number":596,"context_line":"            self.assertEqual(bus, \u0027scsi\u0027)"},{"line_number":597,"context_line":""},{"line_number":598,"context_line":"        with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":599,"context_line":"                               \u0027get_arch\u0027,"},{"line_number":600,"context_line":"                               return_value\u003d\u0027ppc\u0027):"},{"line_number":601,"context_line":"            bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F%2BhNg%3D","line":598,"updated":"2014-01-24 20:11:43.000000000","message":"Duplicating this wasn\u0027t really what I was thinking, but probably works too.  However, could you move this into a for loop and just iterate over a list of the values and set those in your context?  Then we don\u0027t have the copy/paste.","commit_id":"ab931dc17951df72087ea7ed4de97bcbc36365bf"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1dbd7c17e73a4aefa5713f56ba51f72602bebaf7","unresolved":false,"context_lines":[{"line_number":559,"context_line":"            with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":560,"context_line":"                                   \u0027get_arch\u0027,"},{"line_number":561,"context_line":"                                   return_value\u003darch):"},{"line_number":562,"context_line":"                if dev is not None:"},{"line_number":563,"context_line":"                    bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027,"},{"line_number":564,"context_line":"                            device_type\u003ddev)"},{"line_number":565,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F%2BK7s%3D","line":562,"updated":"2014-01-29 13:42:34.000000000","message":"If you just default to \u0027disk\u0027 rather than None in your nested tuples above, you don\u0027t need this condition block since \u0027disk\u0027 is the default value for device_type in get_disk_bus_for_device_type - might make things a tad cleaner here in the logic.","commit_id":"d473340f0eb4dab1ad247d71638700e5fc128eaa"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2eac033a6ec0c0044a0d3fae7c2537e1bd0ba39a","unresolved":false,"context_lines":[{"line_number":559,"context_line":"            with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":560,"context_line":"                                   \u0027get_arch\u0027,"},{"line_number":561,"context_line":"                                   return_value\u003darch):"},{"line_number":562,"context_line":"                if dev is not None:"},{"line_number":563,"context_line":"                    bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027,"},{"line_number":564,"context_line":"                            device_type\u003ddev)"},{"line_number":565,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F%2BK20%3D","line":562,"in_reply_to":"AAAAUH%2F%2BK5Q%3D","updated":"2014-01-29 14:04:44.000000000","message":"I could go either way - if you don\u0027t provide device_type it still defaults to \u0027disk\u0027 so you get it either way.  If someone changed the default they\u0027d probably have to change this code anyway.\n\nI\u0027d prefer to have clean logic in the test if it\u0027s a wash either way.\n\nMaybe Dan can be the tie-breaker here? :)","commit_id":"d473340f0eb4dab1ad247d71638700e5fc128eaa"},{"author":{"_account_id":8175,"name":"Rafael Folco","email":"rfolco@redhat.com","username":"rfolco"},"change_message_id":"1e83d22a3b8ffe9d2225dd36f78f862c3fbdf884","unresolved":false,"context_lines":[{"line_number":559,"context_line":"            with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":560,"context_line":"                                   \u0027get_arch\u0027,"},{"line_number":561,"context_line":"                                   return_value\u003darch):"},{"line_number":562,"context_line":"                if dev is not None:"},{"line_number":563,"context_line":"                    bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027,"},{"line_number":564,"context_line":"                            device_type\u003ddev)"},{"line_number":565,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F%2BK5Q%3D","line":562,"in_reply_to":"AAAAUH%2F%2BK7s%3D","updated":"2014-01-29 13:55:12.000000000","message":"This has been done on purpose to confirm the functions returns what is expected without giving device_type argument. If this particular case (do not set any device_type as argument) is not needed, I\u0027ll remove the if block. Otherwise, I think it\u0027s better to keep this way.","commit_id":"d473340f0eb4dab1ad247d71638700e5fc128eaa"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"7d82409e88784ddb525ab1c0907d34dcf17445f4","unresolved":false,"context_lines":[{"line_number":559,"context_line":"            with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":560,"context_line":"                                   \u0027get_arch\u0027,"},{"line_number":561,"context_line":"                                   return_value\u003darch):"},{"line_number":562,"context_line":"                if dev is not None:"},{"line_number":563,"context_line":"                    bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027,"},{"line_number":564,"context_line":"                            device_type\u003ddev)"},{"line_number":565,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F%2BK7g%3D","line":562,"in_reply_to":"AAAAUH%2F%2BK7s%3D","updated":"2014-01-29 13:43:59.000000000","message":"Yeah, that would make sense.","commit_id":"d473340f0eb4dab1ad247d71638700e5fc128eaa"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a8ed9b1edf558ddda25d6ef16e0402d2125c8d82","unresolved":false,"context_lines":[{"line_number":559,"context_line":"            with mock.patch.object(blockinfo.libvirt_utils,"},{"line_number":560,"context_line":"                                   \u0027get_arch\u0027,"},{"line_number":561,"context_line":"                                   return_value\u003darch):"},{"line_number":562,"context_line":"                bus \u003d blockinfo.get_disk_bus_for_device_type(\u0027kvm\u0027,"},{"line_number":563,"context_line":"                            device_type\u003ddev)"},{"line_number":564,"context_line":"                self.assertEqual(bus, res)"},{"line_number":565,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAUH%2F%2BJsI%3D","line":562,"updated":"2014-01-29 17:10:36.000000000","message":"Hmm, maybe not for this patch since it wasn\u0027t in here originally, but the virt_type in the condition is really (\u0027qemu\u0027, \u0027kvm\u0027) so we should have the expected tuple nested adjusted with the virt_type for both cases?  I\u0027d be OK with that in it\u0027s own patch though.","commit_id":"0d66018ad3c4c2f69daf6abc5d79d7dee3b64834"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a52244553669bf97f7e4b1c905f2d5138d8626db","unresolved":false,"context_lines":[{"line_number":553,"context_line":"                (\u0027x86_64\u0027, \u0027cdrom\u0027, \u0027ide\u0027),"},{"line_number":554,"context_line":"                (\u0027x86_64\u0027, \u0027floppy\u0027, \u0027fdc\u0027),"},{"line_number":555,"context_line":"                (\u0027ppc\u0027, \u0027cdrom\u0027, \u0027scsi\u0027),"},{"line_number":556,"context_line":"                (\u0027ppc64\u0027, \u0027cdrom\u0027, \u0027scsi\u0027)"},{"line_number":557,"context_line":"                )"},{"line_number":558,"context_line":"        for arch, dev, res in expected:"},{"line_number":559,"context_line":"            with mock.patch.object(blockinfo.libvirt_utils,"}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAUH%2F%2BDww%3D","line":556,"updated":"2014-01-30 15:41:45.000000000","message":"Since the last patch set you changed the code, so I think you should also cover two more cases here to avoid regression:\n\n(\u0027ppc\u0027, \u0027disk\u0027, \u0027virtio\u0027),\n\n(\u0027ppc64\u0027, \u0027disk\u0027, \u0027virtio\u0027),","commit_id":"a5e9ecfbca88ac7499242cc0e9ad9c0978a9d9c0"}],"nova/virt/libvirt/blockinfo.py":[{"author":{"_account_id":8175,"name":"Rafael Folco","email":"rfolco@redhat.com","username":"rfolco"},"change_message_id":"7778aa41c1767cdf59180c4c9eeac6355c0225a0","unresolved":false,"context_lines":[{"line_number":253,"context_line":"                return \"scsi\""},{"line_number":254,"context_line":"            else:"},{"line_number":255,"context_line":"                return \"ide\""},{"line_number":256,"context_line":"        elif device_type \u003d\u003d \"disk\":"},{"line_number":257,"context_line":"            return \"virtio\""},{"line_number":258,"context_line":"        elif device_type \u003d\u003d \"floppy\":"},{"line_number":259,"context_line":"            return \"fdc\""}],"source_content_type":"text/x-python","patch_set":12,"id":"AAAAUH%2F%2BEdo%3D","line":256,"updated":"2014-01-30 13:39:35.000000000","message":"libvirt for Power sets \u0027scsi\u0027 as the default bus for both disk and cdrom. However, the ppc disk bus supports virtio drivers to improve I/O performance.","commit_id":"a5e9ecfbca88ac7499242cc0e9ad9c0978a9d9c0"}],"nova/virt/libvirt/utils.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"d013942081b29ce1fba5e484f9d7bfd22f726628","unresolved":false,"context_lines":[{"line_number":648,"context_line":"    the hypervisor."},{"line_number":649,"context_line":"    \"\"\""},{"line_number":650,"context_line":"    if image_meta:"},{"line_number":651,"context_line":"        return image_meta.get(\u0027properties\u0027, {}).get(\u0027architecture\u0027)"},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"    return None"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2FNpY%3D","line":651,"updated":"2014-01-16 10:03:12.000000000","message":"If no \"architecture\" key is present you should be falling back to using the host architecture. This falls back to \"none\"","commit_id":"cd10244c533b132eca1c8efb994032df4c9a9fbe"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"dd8c04d8498c8e57c5b19aaf8655877e9e0868b2","unresolved":false,"context_lines":[{"line_number":648,"context_line":"    the hypervisor."},{"line_number":649,"context_line":"    \"\"\""},{"line_number":650,"context_line":"    if image_meta:"},{"line_number":651,"context_line":"        return image_meta.get(\u0027properties\u0027, {}).get(\u0027architecture\u0027)"},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"    return None"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2FMnc%3D","line":651,"in_reply_to":"AAAAUH%2F%2FMoU%3D","updated":"2014-01-16 14:50:05.000000000","message":"I meant fallback to using   platform.processor() when no image property is set.","commit_id":"cd10244c533b132eca1c8efb994032df4c9a9fbe"},{"author":{"_account_id":8175,"name":"Rafael Folco","email":"rfolco@redhat.com","username":"rfolco"},"change_message_id":"ee90e6ef849bb5e0b98536af6082e8db92b8d5c2","unresolved":false,"context_lines":[{"line_number":648,"context_line":"    the hypervisor."},{"line_number":649,"context_line":"    \"\"\""},{"line_number":650,"context_line":"    if image_meta:"},{"line_number":651,"context_line":"        return image_meta.get(\u0027properties\u0027, {}).get(\u0027architecture\u0027)"},{"line_number":652,"context_line":""},{"line_number":653,"context_line":"    return None"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2FMoU%3D","line":651,"in_reply_to":"AAAAUH%2F%2FNpY%3D","updated":"2014-01-16 14:46:23.000000000","message":"Is CONF.libvirt.cpu_model acceptable ? Even using custom cpu_mode or host-model would do the job here. Othwerwise, I\u0027d need to get this info from driver.py (_caps.host.cpu.arch), which requires changes in many functions. Unless I am missing something here.\n\nThere is no easy way to get the host arch on blockinfo.py. I would need to add one more argument (host_arch) to the get_disk_bus_for_device_type and this would affect more 3 functions: get_disk_info, get_disk_mapping, and get_info_from_bdm.\nAnd that\u0027s what I am trying to avoid.\n\nThis seems to be much overhead for a fall back plan in this little change.","commit_id":"cd10244c533b132eca1c8efb994032df4c9a9fbe"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"8dc9834b96a9fd45c3152699c65a27e1013640a3","unresolved":false,"context_lines":[{"line_number":657,"context_line":""},{"line_number":658,"context_line":"    #fallback [1]: get cpu_model from libvirt"},{"line_number":659,"context_line":"    if CONF.libvirt.cpu_model is not None:"},{"line_number":660,"context_line":"        return CONF.libvirt.cpu_model"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    #fallback [2]: get arch from nova-compute"},{"line_number":663,"context_line":"    return platform.processor()"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2B7DM%3D","line":660,"updated":"2014-01-20 18:01:55.000000000","message":"CPU model names !\u003d  architecture names.\n\neg CONF.libvirt.cpu_model will be something like Opteron_G3, Westmere or other codenames.","commit_id":"ddc2ea455c6a1d32ff4c4a63cc051e8fbf3d412a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e012c7b67fb314633152c40936372ebec918f79a","unresolved":false,"context_lines":[{"line_number":664,"context_line":"    This method determines the CPU architecture that must be supported by"},{"line_number":665,"context_line":"    the hypervisor."},{"line_number":666,"context_line":"    \"\"\""},{"line_number":667,"context_line":"    #get guest cpu arch from image_meta"},{"line_number":668,"context_line":"    if image_meta:"},{"line_number":669,"context_line":"        arch \u003d image_meta.get(\u0027properties\u0027, {}).get(\u0027architecture\u0027)"},{"line_number":670,"context_line":"        if arch is not None:"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F%2BJ7o%3D","line":667,"updated":"2014-01-29 16:30:39.000000000","message":"Does this comment really add anything? I don\u0027t think it does. Plus, it\u0027s not really in correct comment format.","commit_id":"50b9eaa7ef96078d198a6b8f920357a300902208"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e012c7b67fb314633152c40936372ebec918f79a","unresolved":false,"context_lines":[{"line_number":670,"context_line":"        if arch is not None:"},{"line_number":671,"context_line":"            return arch"},{"line_number":672,"context_line":""},{"line_number":673,"context_line":"    #fallback: get arch from nova-compute"},{"line_number":674,"context_line":"    return platform.processor()"}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F%2BJ7U%3D","line":673,"updated":"2014-01-29 16:30:39.000000000","message":"Same here. I think all the details of these two comments should be expressed in the docstring as they indicate the behavior of the function to the caller.","commit_id":"50b9eaa7ef96078d198a6b8f920357a300902208"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a8ed9b1edf558ddda25d6ef16e0402d2125c8d82","unresolved":false,"context_lines":[{"line_number":660,"context_line":""},{"line_number":661,"context_line":"def get_arch(image_meta):"},{"line_number":662,"context_line":"    \"\"\"Determine the guest architecture based on image_meta."},{"line_number":663,"context_line":""},{"line_number":664,"context_line":"    This method determines the CPU architecture that must be supported by"},{"line_number":665,"context_line":"    the hypervisor."},{"line_number":666,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAUH%2F%2BJsg%3D","line":663,"updated":"2014-01-29 17:10:36.000000000","message":"As Dan pointed out in the previous patch set, the comments should be integrated with the docstrings to explain what the caller can expect if architecture is not defined in the image_meta or if it\u0027s not provded, what they can get back (native value from the platform).","commit_id":"0d66018ad3c4c2f69daf6abc5d79d7dee3b64834"}]}