)]}'
{"nova/tests/test_libvirt.py":[{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":263,"context_line":"        vol \u003d {\u0027id\u0027: 1, \u0027name\u0027: name}"},{"line_number":264,"context_line":"        connection_info \u003d self.sheepdog_connection(vol)"},{"line_number":265,"context_line":"        conf \u003d libvirt_driver.connect_volume(connection_info,"},{"line_number":266,"context_line":"                                             \"virtio\", \"vde\", \"disk\")"},{"line_number":267,"context_line":"        tree \u003d conf.format_dom()"},{"line_number":268,"context_line":"        self.assertEqual(tree.get(\u0027type\u0027), \u0027network\u0027)"},{"line_number":269,"context_line":"        self.assertEqual(tree.find(\u0027./source\u0027).get(\u0027protocol\u0027), \u0027sheepdog\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX1Q%3D","line":266,"updated":"2013-01-25 00:41:30.000000000","message":"perhaps we should pass the info descriptor directly in - the dict with bus/dev/type in it? Keeps it together, avoids room for loops that only reassign one leg and other such bugs.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":704,"context_line":"        try:"},{"line_number":705,"context_line":"            virt_dom \u003d self._lookup_by_name(instance[\u0027name\u0027])"},{"line_number":706,"context_line":"            xml \u003d virt_dom.XMLDesc(0)"},{"line_number":707,"context_line":"        except exception.InstanceNotFound:"},{"line_number":708,"context_line":"            disk_mapping \u003d self.get_guest_disk_mapping(instance,"},{"line_number":709,"context_line":"                                                       block_device_info)"},{"line_number":710,"context_line":"            xml \u003d self.to_xml(instance, network_info, disk_mapping,"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX1o%3D","line":707,"updated":"2013-01-25 00:41:30.000000000","message":"This code path looks like it will break because get_guest_disk_mapping requires 3 parameters but you only pass in two. Please add a test to confirm that I\u0027m right, then fix :)","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":713,"context_line":""},{"line_number":714,"context_line":"    def detach_volume(self, connection_info, instance, mountpoint):"},{"line_number":715,"context_line":"        instance_name \u003d instance[\u0027name\u0027]"},{"line_number":716,"context_line":"        disk_dev \u003d mountpoint.rpartition(\"/\")[2]"},{"line_number":717,"context_line":"        try:"},{"line_number":718,"context_line":"            virt_dom \u003d self._lookup_by_name(instance_name)"},{"line_number":719,"context_line":"            xml \u003d self._get_disk_xml(virt_dom.XMLDesc(0), disk_dev)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX3g%3D","line":716,"updated":"2013-01-25 00:41:30.000000000","message":"Thought: the mountpoint.rpartition(\"/\")[2] fragment is a little opaque (and repeated) - perhaps extract a method for that?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":713,"context_line":""},{"line_number":714,"context_line":"    def detach_volume(self, connection_info, instance, mountpoint):"},{"line_number":715,"context_line":"        instance_name \u003d instance[\u0027name\u0027]"},{"line_number":716,"context_line":"        disk_dev \u003d mountpoint.rpartition(\"/\")[2]"},{"line_number":717,"context_line":"        try:"},{"line_number":718,"context_line":"            virt_dom \u003d self._lookup_by_name(instance_name)"},{"line_number":719,"context_line":"            xml \u003d self._get_disk_xml(virt_dom.XMLDesc(0), disk_dev)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXZQ%3D","line":716,"in_reply_to":"AAAAM3%2F%2FX3g%3D","updated":"2013-01-25 11:57:01.000000000","message":"Well this is pre-existing code, so it would be inappropriate to mix such a change with this patch. It is a job for a separate cleanup","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1243,"context_line":"        if os.path.exists(console_log):"},{"line_number":1244,"context_line":"            libvirt_utils.chown(console_log, os.getuid())"},{"line_number":1245,"context_line":""},{"line_number":1246,"context_line":"    def has_guest_disk_dev(self, mapping, disk_dev):"},{"line_number":1247,"context_line":"        \"\"\"Determine if a disk device name has already been used.\"\"\""},{"line_number":1248,"context_line":"        for disk in mapping:"},{"line_number":1249,"context_line":"            info \u003d mapping[disk]"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX3Q%3D","line":1246,"updated":"2013-01-25 00:41:30.000000000","message":"This has no reference to self - perhaps it should be a standalone function for use anywhere, or even a method on mapping?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":1243,"context_line":"        if os.path.exists(console_log):"},{"line_number":1244,"context_line":"            libvirt_utils.chown(console_log, os.getuid())"},{"line_number":1245,"context_line":""},{"line_number":1246,"context_line":"    def has_guest_disk_dev(self, mapping, disk_dev):"},{"line_number":1247,"context_line":"        \"\"\"Determine if a disk device name has already been used.\"\"\""},{"line_number":1248,"context_line":"        for disk in mapping:"},{"line_number":1249,"context_line":"            info \u003d mapping[disk]"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXYo%3D","line":1246,"in_reply_to":"AAAAM3%2F%2FX3Q%3D","updated":"2013-01-25 11:57:01.000000000","message":"Hmm, for that matter, I should probably move all of this code out into a standalone module in libvirt/blockinfo.py since it has no reason to be directly associated with the driver module code.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1251,"context_line":"                return True"},{"line_number":1252,"context_line":"        return False"},{"line_number":1253,"context_line":""},{"line_number":1254,"context_line":"    def find_disk_dev(self, mapping, bus, last_device\u003dFalse):"},{"line_number":1255,"context_line":"        disk_prefix \u003d self.get_guest_disk_prefix_for_bus(bus)"},{"line_number":1256,"context_line":"        max_dev \u003d self.get_guest_max_dev_for_bus(bus)"},{"line_number":1257,"context_line":"        if last_device:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX3M%3D","line":1254,"updated":"2013-01-25 00:41:30.000000000","message":"This is either a) really two different methods, or can be combined down to be quite a bit narrower. I say this because 80% of the method is an if/else split on last_device: there are two different use cases here.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":1251,"context_line":"                return True"},{"line_number":1252,"context_line":"        return False"},{"line_number":1253,"context_line":""},{"line_number":1254,"context_line":"    def find_disk_dev(self, mapping, bus, last_device\u003dFalse):"},{"line_number":1255,"context_line":"        disk_prefix \u003d self.get_guest_disk_prefix_for_bus(bus)"},{"line_number":1256,"context_line":"        max_dev \u003d self.get_guest_max_dev_for_bus(bus)"},{"line_number":1257,"context_line":"        if last_device:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXZI%3D","line":1254,"in_reply_to":"AAAAM3%2F%2FX3M%3D","updated":"2013-01-25 11:57:01.000000000","message":"I guess we could do\n\n if last_device\n    devs \u003d [max_dev-1]\n else\n   devs \u003d range(max_dev)\n\nand thus only have one loop to deal with","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1271,"context_line":"            _(\"No more free disk device names for prefix \u0027%s\u0027\"),"},{"line_number":1272,"context_line":"            disk_prefix)"},{"line_number":1273,"context_line":""},{"line_number":1274,"context_line":"    def get_guest_disk_bus(self, device_type\u003d\"disk\"):"},{"line_number":1275,"context_line":"        \"\"\"Determine the best disk bus to use for a device type"},{"line_number":1276,"context_line":"        and a given instance\"\"\""},{"line_number":1277,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX3I%3D","line":1274,"updated":"2013-01-25 00:41:30.000000000","message":"This has no reference to internal state - I think it would be better as a standalone function.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1273,"context_line":""},{"line_number":1274,"context_line":"    def get_guest_disk_bus(self, device_type\u003d\"disk\"):"},{"line_number":1275,"context_line":"        \"\"\"Determine the best disk bus to use for a device type"},{"line_number":1276,"context_line":"        and a given instance\"\"\""},{"line_number":1277,"context_line":""},{"line_number":1278,"context_line":"        if CONF.libvirt_type \u003d\u003d \"uml\":"},{"line_number":1279,"context_line":"            if device_type \u003d\u003d \"disk\":"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX2s%3D","line":1276,"updated":"2013-01-25 00:41:30.000000000","message":"PEP8 - first line should be a single sentence. Its long partly because it marks a false claim (it does not look at instance, only device type).\nPerhaps \"\"\"Find the right bus for device_type based on libvirt_type.\n\n:param device_type: One of \u0027disk\u0027, \u0027cdrom\u0027. Other types are ignored and None will be returned.\n:return: None if any bus can be used, otherwise a bus name - one of \"uml\", \"ide\", \"xen\", \"virtio\".\n\"\"\"\n\nIf I got the description wrong - please fix :)","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1291,"context_line":""},{"line_number":1292,"context_line":"        return None"},{"line_number":1293,"context_line":""},{"line_number":1294,"context_line":"    def get_guest_disk_prefix_for_bus(self, disk_bus):"},{"line_number":1295,"context_line":"        if disk_bus \u003d\u003d \"ide\":"},{"line_number":1296,"context_line":"            return \"hd\""},{"line_number":1297,"context_line":"        elif disk_bus \u003d\u003d \"virtio\":"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX2Y%3D","line":1294,"updated":"2013-01-25 00:41:30.000000000","message":"Similarly to get_bus_for... needs docs, should be a separate function not a method IMO. I note that this raises where the other one returns None; this is inconsistent and harder for folk to predict - can we pick one and do that for both ?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1311,"context_line":"                _(\"Unable to determine disk prefix for %s\") %"},{"line_number":1312,"context_line":"                disk_bus)"},{"line_number":1313,"context_line":""},{"line_number":1314,"context_line":"    def get_guest_max_dev_for_bus(self, disk_bus):"},{"line_number":1315,"context_line":"        if disk_bus \u003d\u003d \"ide\":"},{"line_number":1316,"context_line":"            return 4"},{"line_number":1317,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX2Q%3D","line":1314,"updated":"2013-01-25 00:41:30.000000000","message":"Wants to be a separate function I think. Or perhaps promote bus to an object?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1317,"context_line":"        else:"},{"line_number":1318,"context_line":"            return 26"},{"line_number":1319,"context_line":""},{"line_number":1320,"context_line":"    def get_guest_disk_bus_for_dev(self, disk_dev):"},{"line_number":1321,"context_line":"        if disk_dev[:2] \u003d\u003d \u0027hd\u0027:"},{"line_number":1322,"context_line":"            return \"ide\""},{"line_number":1323,"context_line":"        elif disk_dev[:2] \u003d\u003d \u0027sd\u0027:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX2M%3D","line":1320,"updated":"2013-01-25 00:41:30.000000000","message":"And - same line of comments here as well.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1341,"context_line":""},{"line_number":1342,"context_line":"    def get_guest_next_disk_dev(self, mapping, disk_bus,"},{"line_number":1343,"context_line":"                                device_type\u003d\u0027disk\u0027,"},{"line_number":1344,"context_line":"                                last_device\u003dFalse):"},{"line_number":1345,"context_line":"        disk_dev \u003d self.find_disk_dev(mapping,"},{"line_number":1346,"context_line":"                                      disk_bus,"},{"line_number":1347,"context_line":"                                      last_device)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX2I%3D","line":1344,"updated":"2013-01-25 00:41:30.000000000","message":"The method name suggests it returns a dev, but it returns a dict describing bus + dev + type, perhaps \u0027get_guest_next_disk_info\u0027 ?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1352,"context_line":"    def get_guest_disk_mapping(self, instance,"},{"line_number":1353,"context_line":"                               disk_bus, cdrom_bus,"},{"line_number":1354,"context_line":"                               block_device_info\u003dNone,"},{"line_number":1355,"context_line":"                               image_meta\u003dNone, rescue\u003dFalse):"},{"line_number":1356,"context_line":"        \"\"\"Determine how to map default disks to the virtual machine."},{"line_number":1357,"context_line":"        This is about figuring out whether the default \u0027disk\u0027,"},{"line_number":1358,"context_line":"        \u0027disk.local\u0027, \u0027disk.swap\u0027 and \u0027disk.config\u0027 images have"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX14%3D","line":1355,"updated":"2013-01-25 00:41:30.000000000","message":"This is a super long function, and it has some very different code paths (e.g. rescue) - can we split it into a bunch of smaller targeted functions?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":1352,"context_line":"    def get_guest_disk_mapping(self, instance,"},{"line_number":1353,"context_line":"                               disk_bus, cdrom_bus,"},{"line_number":1354,"context_line":"                               block_device_info\u003dNone,"},{"line_number":1355,"context_line":"                               image_meta\u003dNone, rescue\u003dFalse):"},{"line_number":1356,"context_line":"        \"\"\"Determine how to map default disks to the virtual machine."},{"line_number":1357,"context_line":"        This is about figuring out whether the default \u0027disk\u0027,"},{"line_number":1358,"context_line":"        \u0027disk.local\u0027, \u0027disk.swap\u0027 and \u0027disk.config\u0027 images have"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXZA%3D","line":1355,"in_reply_to":"AAAAM3%2F%2FX14%3D","updated":"2013-01-25 11:57:01.000000000","message":"I actually think it is right for this to be all in one function. The logic for assigning disk devices really does need to be considered as a whole, not treated as independent pieces. Splitting up into separate functions would hurt actually hurt understanding of this code \u0026 lead to greater potential for screwups in the future IMHO.\n\nI don\u0027t want to split off the rescue stuff either. The current approach to rescue mode is fundamentally flawed, because it is only considering the root disk and not providing any of the volume mappings which the admin may well need in order to unfsck their guest. The rescue code actually needs to fixed follow exactly the same logic as non-rescue mode, the only difference being that you\u0027d boot off the disk.rescue. This is something I want to address in a subsequent patch.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1787,"context_line":"        return cpu"},{"line_number":1788,"context_line":""},{"line_number":1789,"context_line":"    def get_guest_disk_config(self, instance, name, disk_mapping,"},{"line_number":1790,"context_line":"                              image_type\u003dNone):"},{"line_number":1791,"context_line":"        image \u003d self.image_backend.image(instance,"},{"line_number":1792,"context_line":"                                         name,"},{"line_number":1793,"context_line":"                                         image_type)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX1g%3D","line":1790,"updated":"2013-01-25 00:41:30.000000000","message":"I think the next call will all fit on one line.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":1911,"context_line":"        guest.cpu \u003d self.get_guest_cpu_config()"},{"line_number":1912,"context_line":""},{"line_number":1913,"context_line":"        if \u0027root\u0027 in disk_mapping:"},{"line_number":1914,"context_line":"            root_device_name \u003d \"/dev/\" + disk_mapping[\u0027root\u0027][\u0027dev\u0027]"},{"line_number":1915,"context_line":"        else:"},{"line_number":1916,"context_line":"            root_device_name \u003d None"},{"line_number":1917,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX1c%3D","line":1914,"updated":"2013-01-25 00:41:30.000000000","message":"This branch and the \u0027if root_device_name\u0027 if below can be combined AFAICT, as root_device_name is not passed in.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":1911,"context_line":"        guest.cpu \u003d self.get_guest_cpu_config()"},{"line_number":1912,"context_line":""},{"line_number":1913,"context_line":"        if \u0027root\u0027 in disk_mapping:"},{"line_number":1914,"context_line":"            root_device_name \u003d \"/dev/\" + disk_mapping[\u0027root\u0027][\u0027dev\u0027]"},{"line_number":1915,"context_line":"        else:"},{"line_number":1916,"context_line":"            root_device_name \u003d None"},{"line_number":1917,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXYw%3D","line":1914,"in_reply_to":"AAAAM3%2F%2FX1c%3D","updated":"2013-01-25 11:57:01.000000000","message":"Hmm, I\u0027m not sure about that. I think this might be an indication that I mistakenly changed something I shouldn\u0027t have. Previously the code would only update the DB instance if root_device_name was not specified in the block_device_info. Now it appears to update it unconditionally,  which I think is possibly wrong. I will investigate further.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1812,"name":"p-draigbrady","email":"P@draigBrady.com","username":"p-draigbrady"},"change_message_id":"24107ac57f3b9e7f5e77ac00803910f81f559ff8","unresolved":false,"context_lines":[{"line_number":1031,"context_line":"                                            instance,"},{"line_number":1032,"context_line":"                                            block_device_info)"},{"line_number":1033,"context_line":"        xml \u003d self.to_xml(instance, network_info, disk_info,"},{"line_number":1034,"context_line":"                          write_to_disk\u003dTrue)"},{"line_number":1035,"context_line":"        self._create_domain_and_network(xml, instance, network_info,"},{"line_number":1036,"context_line":"                                        block_device_info)"},{"line_number":1037,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAM3%2F%2FH9U%3D","line":1034,"updated":"2013-02-06 13:59:13.000000000","message":"I notice to_xml() no longer gets block_device_info here.\nJust a comment in case this was unintended.","commit_id":"8080953fe4544d3aa5d6f1da833dc5c763f63c5a"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"68cf39c3db39b570c372aed54c3c7b7afa86816b","unresolved":false,"context_lines":[{"line_number":1031,"context_line":"                                            instance,"},{"line_number":1032,"context_line":"                                            block_device_info)"},{"line_number":1033,"context_line":"        xml \u003d self.to_xml(instance, network_info, disk_info,"},{"line_number":1034,"context_line":"                          write_to_disk\u003dTrue)"},{"line_number":1035,"context_line":"        self._create_domain_and_network(xml, instance, network_info,"},{"line_number":1036,"context_line":"                                        block_device_info)"},{"line_number":1037,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAM3%2F%2FH8s%3D","line":1034,"in_reply_to":"AAAAM3%2F%2FH9U%3D","updated":"2013-02-06 14:11:16.000000000","message":"Nice catch - this is indeed a mistake","commit_id":"8080953fe4544d3aa5d6f1da833dc5c763f63c5a"}],"nova/virt/libvirt/imagebackend.py":[{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":23,"context_line":"from nova.openstack.common import excutils"},{"line_number":24,"context_line":"from nova.openstack.common import fileutils"},{"line_number":25,"context_line":"from nova.openstack.common import lockutils"},{"line_number":26,"context_line":"from nova.openstack.common import log as logging"},{"line_number":27,"context_line":"from nova import utils"},{"line_number":28,"context_line":"from nova.virt.disk import api as disk"},{"line_number":29,"context_line":"from nova.virt.libvirt import config as vconfig"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX1U%3D","line":26,"updated":"2013-01-25 00:41:30.000000000","message":"This import is not used.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":23,"context_line":"from nova.openstack.common import excutils"},{"line_number":24,"context_line":"from nova.openstack.common import fileutils"},{"line_number":25,"context_line":"from nova.openstack.common import lockutils"},{"line_number":26,"context_line":"from nova.openstack.common import log as logging"},{"line_number":27,"context_line":"from nova import utils"},{"line_number":28,"context_line":"from nova.virt.disk import api as disk"},{"line_number":29,"context_line":"from nova.virt.libvirt import config as vconfig"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXZU%3D","line":26,"in_reply_to":"AAAAM3%2F%2FX1U%3D","updated":"2013-01-25 11:57:01.000000000","message":"Leftover from debugging...","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"}],"nova/virt/libvirt/volume.py":[{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":52,"context_line":"    def __init__(self, connection):"},{"line_number":53,"context_line":"        self.connection \u003d connection"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def connect_volume(self, connection_info, disk_bus, disk_dev, device_type):"},{"line_number":56,"context_line":"        \"\"\"Connect the volume. Returns xml for libvirt.\"\"\""},{"line_number":57,"context_line":"        conf \u003d vconfig.LibvirtConfigGuestDisk()"},{"line_number":58,"context_line":"        conf.source_type \u003d \"block\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX1E%3D","line":55,"updated":"2013-01-25 00:41:30.000000000","message":"There is a lot of duplicate boilerplate in these routines. Can it be reduced while you\u0027re here, rather than exacerbating it?","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"21dd1e0d55ebf198e0afed52120a4379d07b8405","unresolved":false,"context_lines":[{"line_number":52,"context_line":"    def __init__(self, connection):"},{"line_number":53,"context_line":"        self.connection \u003d connection"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"    def connect_volume(self, connection_info, disk_bus, disk_dev, device_type):"},{"line_number":56,"context_line":"        \"\"\"Connect the volume. Returns xml for libvirt.\"\"\""},{"line_number":57,"context_line":"        conf \u003d vconfig.LibvirtConfigGuestDisk()"},{"line_number":58,"context_line":"        conf.source_type \u003d \"block\""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FXZc%3D","line":55,"in_reply_to":"AAAAM3%2F%2FX1E%3D","updated":"2013-01-25 11:57:01.000000000","message":"There is definitely room to refactor this whole module, but that should be done as a separate patch. I\u0027ll see about adding such a patch to this series","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":4190,"name":"lifeless","email":"robertc@robertcollins.net","username":"lifeless"},"change_message_id":"0e2b50e86b0b7dd0161fbf0ca9665dd6ed437649","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        return self._run_iscsiadm(iscsi_properties, iscsi_command, **kwargs)"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    @lockutils.synchronized(\u0027connect_volume\u0027, \u0027nova-\u0027)"},{"line_number":145,"context_line":"    def connect_volume(self, connection_info, disk_bus, disk_dev, device_type):"},{"line_number":146,"context_line":"        \"\"\"Attach the volume to instance_name.\"\"\""},{"line_number":147,"context_line":"        iscsi_properties \u003d connection_info[\u0027data\u0027]"},{"line_number":148,"context_line":"        # NOTE(vish): If we are on the same host as nova volume, the"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAM3%2F%2FX08%3D","line":145,"updated":"2013-01-25 00:41:30.000000000","message":"I think it would definitely be a good idea to pass disk_bus, disk_dev and device_type as a dict or tuple, they are tightly coupled together from what I can see.","commit_id":"44d46c29bb88f3556c7fb52924156cbf7da0305f"},{"author":{"_account_id":1812,"name":"p-draigbrady","email":"P@draigBrady.com","username":"p-draigbrady"},"change_message_id":"24107ac57f3b9e7f5e77ac00803910f81f559ff8","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                raise exception.NovaException(_(\"iSCSI device not found at %s\")"},{"line_number":221,"context_line":"                                              % (host_device))"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"            LOG.warn(_(\"ISCSI volume not yet found at: %(disk_dev)s. \""},{"line_number":224,"context_line":"                       \"Will rescan \u0026 retry.  Try number: %(tries)s\") %"},{"line_number":225,"context_line":"                     locals())"},{"line_number":226,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAM3%2F%2FH94%3D","line":223,"updated":"2013-02-06 13:59:13.000000000","message":"is disk_dev defined in this function?","commit_id":"8080953fe4544d3aa5d6f1da833dc5c763f63c5a"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"68cf39c3db39b570c372aed54c3c7b7afa86816b","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                raise exception.NovaException(_(\"iSCSI device not found at %s\")"},{"line_number":221,"context_line":"                                              % (host_device))"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"            LOG.warn(_(\"ISCSI volume not yet found at: %(disk_dev)s. \""},{"line_number":224,"context_line":"                       \"Will rescan \u0026 retry.  Try number: %(tries)s\") %"},{"line_number":225,"context_line":"                     locals())"},{"line_number":226,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"AAAAM3%2F%2FH8o%3D","line":223,"in_reply_to":"AAAAM3%2F%2FH94%3D","updated":"2013-02-06 14:11:16.000000000","message":"Opps, yes, that needs changing","commit_id":"8080953fe4544d3aa5d6f1da833dc5c763f63c5a"}]}