)]}'
{"nova/tests/virt/libvirt/test_libvirt.py":[{"author":{"_account_id":8175,"name":"Rafael Folco","email":"rfolco@redhat.com","username":"rfolco"},"change_message_id":"74df5c4a0b86f7f739b5979363b9708002c5c388","unresolved":false,"context_lines":[{"line_number":1462,"context_line":"                             vconfig.LibvirtConfigGuestVideo)"},{"line_number":1463,"context_line":"            self.assertEqual(cfg.devices[6].type, \u0027vga\u0027)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"    def test_get_guest_config_ppc_through_image_meta_vnc_enabled(self):"},{"line_number":1466,"context_line":"        self.flags(vnc_enabled\u003dTrue)"},{"line_number":1467,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1468,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2BCio%3D","line":1465,"updated":"2014-01-30 18:49:02.000000000","message":"why don\u0027t you create a loop for ppc/ppc64 instead of replicating code?","commit_id":"0249e31b5ed7bea8a4d51faf47f6341015cc195a"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3e80ab838d1da282d7d2c0641590301fc5a26420","unresolved":false,"context_lines":[{"line_number":1462,"context_line":"                             vconfig.LibvirtConfigGuestVideo)"},{"line_number":1463,"context_line":"            self.assertEqual(cfg.devices[6].type, \u0027vga\u0027)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"    def test_get_guest_config_ppc_through_image_meta_vnc_enabled(self):"},{"line_number":1466,"context_line":"        self.flags(vnc_enabled\u003dTrue)"},{"line_number":1467,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1468,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2BCNw%3D","line":1465,"in_reply_to":"AAAAUH%2F%2BCio%3D","updated":"2014-01-30 19:45:47.000000000","message":"Done","commit_id":"0249e31b5ed7bea8a4d51faf47f6341015cc195a"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"d1aa692f81f72e859ce732cb6167fbbbccd5a8ec","unresolved":false,"context_lines":[{"line_number":1462,"context_line":"                             vconfig.LibvirtConfigGuestVideo)"},{"line_number":1463,"context_line":"            self.assertEqual(cfg.devices[6].type, \u0027vga\u0027)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"    def test_get_guest_config_ppc_through_image_meta_vnc_enabled(self):"},{"line_number":1466,"context_line":"        self.flags(vnc_enabled\u003dTrue)"},{"line_number":1467,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1468,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2BCag%3D","line":1465,"in_reply_to":"AAAAUH%2F%2BCio%3D","updated":"2014-01-30 19:10:13.000000000","message":"Good idea, I think that\u0027s the way you did your tests? I\u0027ll reference your patch. Thanks!","commit_id":"0249e31b5ed7bea8a4d51faf47f6341015cc195a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"262d972b78a12b5ac57f9227086b1477aba99e09","unresolved":false,"context_lines":[{"line_number":1464,"context_line":"                                 vconfig.LibvirtConfigGuestVideo)"},{"line_number":1465,"context_line":"                self.assertEqual(cfg.devices[6].type, \u0027vga\u0027)"},{"line_number":1466,"context_line":""},{"line_number":1467,"context_line":"    def test_get_guest_config_ppc64_through_image_meta_spice_enabled(self):"},{"line_number":1468,"context_line":"        self.flags(enabled\u003dTrue,"},{"line_number":1469,"context_line":"                   agent_enabled\u003dTrue,"},{"line_number":1470,"context_line":"                   group\u003d\u0027spice\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAUH%2F9jwU%3D","line":1467,"updated":"2014-02-06 02:32:19.000000000","message":"The only difference I see between this test and the one above it is in the first set of flags set and the cfg.devices index value.  I\u0027d like to see a private common test method used to test both scenarios and then have two methods that call that with the cfg.devices index to assert (would be nice if they were enums/constants), and then the public test methods would also set the appropriate custom flags.\n\nI just really prefer to consolidate this much common test code if possible (which it looks trivial to do in this case) so we avoid maintenance issues later.","commit_id":"b8ba5267223a4680fea1e9eca33443a211bac1ad"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"25aa9221b44beb10786251720e23565d0c3a6890","unresolved":false,"context_lines":[{"line_number":1464,"context_line":"                                 vconfig.LibvirtConfigGuestVideo)"},{"line_number":1465,"context_line":"                self.assertEqual(cfg.devices[6].type, \u0027vga\u0027)"},{"line_number":1466,"context_line":""},{"line_number":1467,"context_line":"    def test_get_guest_config_ppc64_through_image_meta_spice_enabled(self):"},{"line_number":1468,"context_line":"        self.flags(enabled\u003dTrue,"},{"line_number":1469,"context_line":"                   agent_enabled\u003dTrue,"},{"line_number":1470,"context_line":"                   group\u003d\u0027spice\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAUH%2F9eHQ%3D","line":1467,"in_reply_to":"AAAAUH%2F9jwU%3D","updated":"2014-02-06 17:35:13.000000000","message":"Done","commit_id":"b8ba5267223a4680fea1e9eca33443a211bac1ad"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"178d5cecb0c22b6bb0f489b479aecbb011e82f39","unresolved":false,"context_lines":[{"line_number":1463,"context_line":""},{"line_number":1464,"context_line":"    def test_get_guest_config_ppc64_through_image_meta_vnc_enabled(self):"},{"line_number":1465,"context_line":"        self.flags(vnc_enabled\u003dTrue)"},{"line_number":1466,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1467,"context_line":"        self._test_get_guest_config_ppc64(6)"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"    def test_get_guest_config_ppc64_through_image_meta_spice_enabled(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAUH%2F9dCw%3D","line":1466,"updated":"2014-02-06 20:05:53.000000000","message":"This could also go into the common method.","commit_id":"98676beacc590d2fc069a2b4444d171fc031c9b5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"4b38b211b5e17d1eae7f4c4dd448d19a2dbbe0c8","unresolved":false,"context_lines":[{"line_number":1463,"context_line":""},{"line_number":1464,"context_line":"    def test_get_guest_config_ppc64_through_image_meta_vnc_enabled(self):"},{"line_number":1465,"context_line":"        self.flags(vnc_enabled\u003dTrue)"},{"line_number":1466,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1467,"context_line":"        self._test_get_guest_config_ppc64(6)"},{"line_number":1468,"context_line":""},{"line_number":1469,"context_line":"    def test_get_guest_config_ppc64_through_image_meta_spice_enabled(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAUH%2F9dA0%3D","line":1466,"in_reply_to":"AAAAUH%2F9dCw%3D","updated":"2014-02-06 20:10:48.000000000","message":"Nice catch, thanks.","commit_id":"98676beacc590d2fc069a2b4444d171fc031c9b5"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"178d5cecb0c22b6bb0f489b479aecbb011e82f39","unresolved":false,"context_lines":[{"line_number":1470,"context_line":"        self.flags(enabled\u003dTrue,"},{"line_number":1471,"context_line":"                   agent_enabled\u003dTrue,"},{"line_number":1472,"context_line":"                   group\u003d\u0027spice\u0027)"},{"line_number":1473,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1474,"context_line":"        self._test_get_guest_config_ppc64(8)"},{"line_number":1475,"context_line":""},{"line_number":1476,"context_line":"    def test_get_guest_cpu_config_none(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAUH%2F9dCs%3D","line":1473,"updated":"2014-02-06 20:05:53.000000000","message":"ditto","commit_id":"98676beacc590d2fc069a2b4444d171fc031c9b5"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"4b38b211b5e17d1eae7f4c4dd448d19a2dbbe0c8","unresolved":false,"context_lines":[{"line_number":1470,"context_line":"        self.flags(enabled\u003dTrue,"},{"line_number":1471,"context_line":"                   agent_enabled\u003dTrue,"},{"line_number":1472,"context_line":"                   group\u003d\u0027spice\u0027)"},{"line_number":1473,"context_line":"        self.flags(virt_type\u003d\u0027kvm\u0027, group\u003d\u0027libvirt\u0027)"},{"line_number":1474,"context_line":"        self._test_get_guest_config_ppc64(8)"},{"line_number":1475,"context_line":""},{"line_number":1476,"context_line":"    def test_get_guest_cpu_config_none(self):"}],"source_content_type":"text/x-python","patch_set":11,"id":"AAAAUH%2F9dAw%3D","line":1473,"in_reply_to":"AAAAUH%2F9dCs%3D","updated":"2014-02-06 20:10:48.000000000","message":"Done","commit_id":"98676beacc590d2fc069a2b4444d171fc031c9b5"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"90881adcfcaad3ef89899dbdd6e5da5a195f6d1e","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"        # this default. For now, we can detect if we are running nova-compute"},{"line_number":3214,"context_line":"        # on PowerKVM or ppc64 and skip setting the default video emulation."},{"line_number":3215,"context_line":"        # This check should be removed if PowerKVM supports cirrus video"},{"line_number":3216,"context_line":"        # emulation in the future."},{"line_number":3217,"context_line":"        arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3218,"context_line":"        if add_video_driver and arch not in (\"ppc\", \"ppc64\"):"},{"line_number":3219,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BFcE%3D","line":3216,"updated":"2014-01-30 10:00:20.000000000","message":"You can\u0027t simply skip adding the video device entirely just because \u0027cirrus\u0027 doesn\u0027t work. PPC works with \u0027vga\u0027 device and so we should just use that the as the default for that arch, not skip this code.","commit_id":"2664d43a90fc3ed35ca709fcc6f32cc1209124c4"},{"author":{"_account_id":8175,"name":"Rafael Folco","email":"rfolco@redhat.com","username":"rfolco"},"change_message_id":"940ebd3515784dd0b7d7bedfa531eb2e06d0d7f4","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"        # this default. For now, we can detect if we are running nova-compute"},{"line_number":3214,"context_line":"        # on PowerKVM or ppc64 and skip setting the default video emulation."},{"line_number":3215,"context_line":"        # This check should be removed if PowerKVM supports cirrus video"},{"line_number":3216,"context_line":"        # emulation in the future."},{"line_number":3217,"context_line":"        arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3218,"context_line":"        if add_video_driver and arch not in (\"ppc\", \"ppc64\"):"},{"line_number":3219,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BDXo%3D","line":3216,"in_reply_to":"AAAAUH%2F%2BDsI%3D","updated":"2014-01-30 16:43:31.000000000","message":"\u003cvideo\u003e\n      \u003cmodel type\u003d\u0027qxl\u0027\nerror: internal error: process exited while connecting to monitor: qemu-system-ppc64: -device qxl-vga,id\u003dvideo0,ram_size\u003d67108864,vram_size\u003d67108864,bus\u003dpci,addr\u003d0x4: \u0027qxl-vga\u0027 is not a valid device model name","commit_id":"2664d43a90fc3ed35ca709fcc6f32cc1209124c4"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"14a95ffcd063e4284576918be477a2b561d3d4bd","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"        # this default. For now, we can detect if we are running nova-compute"},{"line_number":3214,"context_line":"        # on PowerKVM or ppc64 and skip setting the default video emulation."},{"line_number":3215,"context_line":"        # This check should be removed if PowerKVM supports cirrus video"},{"line_number":3216,"context_line":"        # emulation in the future."},{"line_number":3217,"context_line":"        arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3218,"context_line":"        if add_video_driver and arch not in (\"ppc\", \"ppc64\"):"},{"line_number":3219,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BDqk%3D","line":3216,"in_reply_to":"AAAAUH%2F%2BDsI%3D","updated":"2014-01-30 15:55:39.000000000","message":"Thanks Dan, I tested with \u0027vga\u0027 and that worked. I\u0027m just making a couple tweaks to the unit tests and I\u0027ll have a new patch up. Along the lines of http://paste.openstack.org/show/62180/ \n\nThanks again for the review!","commit_id":"2664d43a90fc3ed35ca709fcc6f32cc1209124c4"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"2dd629adcf3f23bddbd3c49c2433644cc2fc64b6","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"        # this default. For now, we can detect if we are running nova-compute"},{"line_number":3214,"context_line":"        # on PowerKVM or ppc64 and skip setting the default video emulation."},{"line_number":3215,"context_line":"        # This check should be removed if PowerKVM supports cirrus video"},{"line_number":3216,"context_line":"        # emulation in the future."},{"line_number":3217,"context_line":"        arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3218,"context_line":"        if add_video_driver and arch not in (\"ppc\", \"ppc64\"):"},{"line_number":3219,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BDsI%3D","line":3216,"in_reply_to":"AAAAUH%2F%2BDvU%3D","updated":"2014-01-30 15:52:22.000000000","message":"I tested qemu-system-ppc before writing that to confirm it starts up just fine with a \u0027vga\u0027 device model. Likewise I believe that \u0027qxl\u0027 works with PPC too.","commit_id":"2664d43a90fc3ed35ca709fcc6f32cc1209124c4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b9f043ed6028a56f4c2404aede502793f72e77b0","unresolved":false,"context_lines":[{"line_number":3213,"context_line":"        # this default. For now, we can detect if we are running nova-compute"},{"line_number":3214,"context_line":"        # on PowerKVM or ppc64 and skip setting the default video emulation."},{"line_number":3215,"context_line":"        # This check should be removed if PowerKVM supports cirrus video"},{"line_number":3216,"context_line":"        # emulation in the future."},{"line_number":3217,"context_line":"        arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3218,"context_line":"        if add_video_driver and arch not in (\"ppc\", \"ppc64\"):"},{"line_number":3219,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BDvU%3D","line":3216,"in_reply_to":"AAAAUH%2F%2BFcE%3D","updated":"2014-01-30 15:43:53.000000000","message":"Lance was saying that in #openstack-nova yesterday and that\u0027s what he was told internally from the power kvm folks, but for some reason dansmith didn\u0027t think that would work.  Not sure if it was tested or not.","commit_id":"2664d43a90fc3ed35ca709fcc6f32cc1209124c4"},{"author":{"_account_id":8175,"name":"Rafael Folco","email":"rfolco@redhat.com","username":"rfolco"},"change_message_id":"940ebd3515784dd0b7d7bedfa531eb2e06d0d7f4","unresolved":false,"context_lines":[{"line_number":3227,"context_line":""},{"line_number":3228,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3229,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3230,"context_line":"            if guest.os_type \u003d\u003d vm_mode.XEN:"},{"line_number":3231,"context_line":"                video.type \u003d \u0027xen\u0027"},{"line_number":3232,"context_line":"            guest.add_device(video)"},{"line_number":3233,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2BDWU%3D","line":3230,"updated":"2014-01-30 16:43:31.000000000","message":"if CONF.libvirt.virt_type in (\u0027qemu\u0027, \u0027kvm\u0027) and libvirt_utils.get_arch(image_meta) in (\"ppc64\",\"ppc\"): \n    video.type \u003d \u0027vga\u0027","commit_id":"2664d43a90fc3ed35ca709fcc6f32cc1209124c4"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"4d42b19452587f7def4b77c66d5aea3e4ef0e17f","unresolved":false,"context_lines":[{"line_number":3248,"context_line":"            # default set back to \u0027cirrus\u0027."},{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            guest.add_device(video)"},{"line_number":3253,"context_line":""},{"line_number":3254,"context_line":"        # Qemu guest agent only support \u0027qemu\u0027 and \u0027kvm\u0027 hypervisor"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BCp0%3D","line":3251,"updated":"2014-01-30 18:26:56.000000000","message":"Lets put in an explicit else clause here to just clarify things, rather than relying on the config class implicitly. ie\n\n  else:\n     video.type \u003d \u0027cirrus\u0027","commit_id":"858c5a6948dae9d3030e9ba310be966e6b1341b6"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"dcd8562263fbf19f0a6bcb29df0750036951d788","unresolved":false,"context_lines":[{"line_number":3248,"context_line":"            # default set back to \u0027cirrus\u0027."},{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            guest.add_device(video)"},{"line_number":3253,"context_line":""},{"line_number":3254,"context_line":"        # Qemu guest agent only support \u0027qemu\u0027 and \u0027kvm\u0027 hypervisor"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BCn0%3D","line":3251,"in_reply_to":"AAAAUH%2F%2BCp0%3D","updated":"2014-01-30 18:34:21.000000000","message":"Done","commit_id":"858c5a6948dae9d3030e9ba310be966e6b1341b6"},{"author":{"_account_id":8001,"name":"John Warren","email":"jswarren@us.ibm.com","username":"jswarren"},"change_message_id":"d8365a9ed4ce30e6ebf47ad6571c506eb4bada6d","unresolved":false,"context_lines":[{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"},{"line_number":3255,"context_line":"                # of \u0027cirrus\u0027. This should take place when"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F%2BCDY%3D","line":3252,"updated":"2014-01-30 20:12:35.000000000","message":"This seems to be beyond the scope of the commit message.  Also, it seems that if someone breaks LibvirtConfigGuestVideo() so it no longer sets the default to \"cirrus\", this logic would hide that fact; in other words, it looks like you\u0027re preemptively fixing a bug that, if it ever materializes, should be fixed elsewhere.","commit_id":"df7c32d1705a1d3cff3079d80e045ea438d18768"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"3ac1ab360b707e9e56c69e72eb402562a4d8d64d","unresolved":false,"context_lines":[{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"},{"line_number":3255,"context_line":"                # of \u0027cirrus\u0027. This should take place when"}],"source_content_type":"text/x-python","patch_set":5,"id":"AAAAUH%2F%2BCBM%3D","line":3252,"in_reply_to":"AAAAUH%2F%2BCDY%3D","updated":"2014-01-30 20:19:24.000000000","message":"John I see your point, I\u0027ll let danpb weigh in since he mentioned this is a previous patch set.","commit_id":"df7c32d1705a1d3cff3079d80e045ea438d18768"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"5ab87200ef82b8bf6156902f94f1d6a813dbc712","unresolved":false,"context_lines":[{"line_number":3248,"context_line":"            # default set back to \u0027cirrus\u0027."},{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F92mc%3D","line":3251,"updated":"2014-02-03 09:51:13.000000000","message":"This block is in the wrong place in entirely, causing it to override the video type set from the image_meta dictionary. You need to move this whole thing up before the procesing of image_meta.","commit_id":"053c904b254347656c9eb43a15c55e9b4f8cfaee"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1bc829fd6b4aeda0e63572ec1ef3b06b416ba574","unresolved":false,"context_lines":[{"line_number":3248,"context_line":"            # default set back to \u0027cirrus\u0027."},{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9z4M%3D","line":3251,"in_reply_to":"AAAAUH%2F92mc%3D","updated":"2014-02-03 18:14:26.000000000","message":"Done","commit_id":"053c904b254347656c9eb43a15c55e9b4f8cfaee"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"636d695c23c65614f1abe8c74d0cd61f34c076f7","unresolved":false,"context_lines":[{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"},{"line_number":3255,"context_line":"                # of \u0027cirrus\u0027. This should take place when"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F%2BAao%3D","line":3252,"updated":"2014-01-31 02:21:57.000000000","message":"Why is this being added?  As pointed out, this should already be set as the default and if one of the conditions above overrules it, so be it, but not sure why this is needed.  Seems like trying to fix a different bug, or get around some weird logic issue?","commit_id":"053c904b254347656c9eb43a15c55e9b4f8cfaee"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"1bc829fd6b4aeda0e63572ec1ef3b06b416ba574","unresolved":false,"context_lines":[{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"},{"line_number":3255,"context_line":"                # of \u0027cirrus\u0027. This should take place when"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F9z4I%3D","line":3252,"in_reply_to":"AAAAUH%2F%2BAZ8%3D","updated":"2014-02-03 18:14:26.000000000","message":"Done","commit_id":"053c904b254347656c9eb43a15c55e9b4f8cfaee"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"e6fe75709d8439bf2894baea5af0b27c788986e6","unresolved":false,"context_lines":[{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"},{"line_number":3255,"context_line":"                # of \u0027cirrus\u0027. This should take place when"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F%2BAWU%3D","line":3252,"in_reply_to":"AAAAUH%2F%2BAZ8%3D","updated":"2014-01-31 02:55:40.000000000","message":"Yeah, I agree. Before ripping this out I want to make sure Dan is on the same page, and give him time to respond since he suggested it.","commit_id":"053c904b254347656c9eb43a15c55e9b4f8cfaee"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3c50ce04a95f931a7dfcf4513c2790777dcb50c4","unresolved":false,"context_lines":[{"line_number":3249,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3250,"context_line":"            if arch in (\"ppc\", \"ppc64\"):"},{"line_number":3251,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3252,"context_line":"            elif video.type is None:"},{"line_number":3253,"context_line":"                # NOTE(ldbragst): If for some reason video.type isn\u0027t set"},{"line_number":3254,"context_line":"                # at this point, we should make sure it is set to the default"},{"line_number":3255,"context_line":"                # of \u0027cirrus\u0027. This should take place when"}],"source_content_type":"text/x-python","patch_set":6,"id":"AAAAUH%2F%2BAZ8%3D","line":3252,"in_reply_to":"AAAAUH%2F%2BAao%3D","updated":"2014-01-31 02:24:06.000000000","message":"Just read back through the previous patch sets.  I agree with John, but am open to comments from Dan.  I think if this stays in, it has to be called in the commit message at a minimum, or maybe even a follow on patch because this doesn\u0027t seem within the scope of what\u0027s being done or needed here for making this work on ppc*.","commit_id":"053c904b254347656c9eb43a15c55e9b4f8cfaee"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"9ac922c14e31c5a1bdd63b60d67f3e5da1e6f73c","unresolved":false,"context_lines":[{"line_number":3247,"context_line":"                    raise exception.InvalidVideoMode(model\u003dvideo.type)"},{"line_number":3248,"context_line":""},{"line_number":3249,"context_line":"            elif CONF.spice.enabled and arch not in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3250,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3251,"context_line":"            if guest.os_type \u003d\u003d vm_mode.XEN:"},{"line_number":3252,"context_line":"                video.type \u003d \u0027xen\u0027"},{"line_number":3253,"context_line":"            guest.add_device(video)"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F9wfY%3D","line":3250,"updated":"2014-02-04 09:51:16.000000000","message":"I\u0027d suggest that we move this \u0027elsif CONF.spice.enabled\u0027 block up too - make this elif block be associated with your  \"arch in (ppc...)\" block too. That way we don\u0027t need to have a second ppc check here","commit_id":"c596018cbcac93250a231a31c30a8120c3591884"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2b90ae37257baeae10c969f69768a29f7ee49449","unresolved":false,"context_lines":[{"line_number":3247,"context_line":"                    raise exception.InvalidVideoMode(model\u003dvideo.type)"},{"line_number":3248,"context_line":""},{"line_number":3249,"context_line":"            elif CONF.spice.enabled and arch not in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3250,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3251,"context_line":"            if guest.os_type \u003d\u003d vm_mode.XEN:"},{"line_number":3252,"context_line":"                video.type \u003d \u0027xen\u0027"},{"line_number":3253,"context_line":"            guest.add_device(video)"}],"source_content_type":"text/x-python","patch_set":7,"id":"AAAAUH%2F9u2c%3D","line":3250,"in_reply_to":"AAAAUH%2F9wfY%3D","updated":"2014-02-04 15:38:04.000000000","message":"Done","commit_id":"c596018cbcac93250a231a31c30a8120c3591884"},{"author":{"_account_id":8001,"name":"John Warren","email":"jswarren@us.ibm.com","username":"jswarren"},"change_message_id":"98b090f81f30d514f272ede0522dab3d15304f1d","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3240,"context_line":"            if arch in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3241,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3242,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3243,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3244,"context_line":"            meta_prop \u003d image_meta.get(\u0027properties\u0027, {}) if image_meta else {}"},{"line_number":3245,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F9uik%3D","line":3242,"updated":"2014-02-04 16:20:18.000000000","message":"I\u0027m wondering if the code couldn\u0027t be rearranged so that it is abundantly clear which factor \"wins\" in determining the video.type value.  I\u0027m thinking it would be clearer if the overriding condition (guest.os_type\u003d\u003dvm.mode.XEN) occurred first, followed by a series of elif\u0027s in the appropriate order.  It\u0027s potentially confusing to have code set a value at one point only to have later code change the value to something else.  I would: set arch, set meta_prop and then check:\n1. if guest.os_type \u003d\u003d vm_mode.XEN\n2. elif meta_prop.get(\u0027hw_video_model\u0027)\n3. elif arch in (\u0027ppc\u0027, \u0027ppc64\u0027)\n4. elif CONF.spice.enabled\n\nThat way, you have a single if/elif block that covers all possible outcomes, instead of one if/elif and two if\u0027s, all separate.","commit_id":"59debe7c977456059a45779b09c38f304899f7e3"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"73c8209b17bb52241ba5da90d21b7d5efbdc0dc4","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3240,"context_line":"            if arch in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3241,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3242,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3243,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3244,"context_line":"            meta_prop \u003d image_meta.get(\u0027properties\u0027, {}) if image_meta else {}"},{"line_number":3245,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F9uJk%3D","line":3242,"in_reply_to":"AAAAUH%2F9uK8%3D","updated":"2014-02-04 17:22:01.000000000","message":"I think it is clearer to separate out the setup of Nova defaults, from the user specified override of the defaults.","commit_id":"59debe7c977456059a45779b09c38f304899f7e3"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"7e6e01ff8a3f959cf6e8c73c0d331165b62a95a3","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3240,"context_line":"            if arch in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3241,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3242,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3243,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3244,"context_line":"            meta_prop \u003d image_meta.get(\u0027properties\u0027, {}) if image_meta else {}"},{"line_number":3245,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F9uBg%3D","line":3242,"in_reply_to":"AAAAUH%2F9uck%3D","updated":"2014-02-04 17:49:13.000000000","message":"Done","commit_id":"59debe7c977456059a45779b09c38f304899f7e3"},{"author":{"_account_id":8001,"name":"John Warren","email":"jswarren@us.ibm.com","username":"jswarren"},"change_message_id":"23e4b61b456cab792d51c5bda35d237d467368e1","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3240,"context_line":"            if arch in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3241,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3242,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3243,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3244,"context_line":"            meta_prop \u003d image_meta.get(\u0027properties\u0027, {}) if image_meta else {}"},{"line_number":3245,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F9uK8%3D","line":3242,"in_reply_to":"AAAAUH%2F9uck%3D","updated":"2014-02-04 17:18:43.000000000","message":"Why two blocks when only one is necessary?  It seems clearer to have one, in the sense that you have conditions ordered in increasing subordination and you have all possible outcomes in one place, similar to a switch statement.  If a developer needs to add a new condition, it will be clear where in the sequence of checks it should be added.  It arguably also doesn\u0027t make sense to set video.type in the first block only to have the second block potentially overwrite the value.","commit_id":"59debe7c977456059a45779b09c38f304899f7e3"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"7e6e01ff8a3f959cf6e8c73c0d331165b62a95a3","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3240,"context_line":"            if arch in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3241,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3242,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3243,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3244,"context_line":"            meta_prop \u003d image_meta.get(\u0027properties\u0027, {}) if image_meta else {}"},{"line_number":3245,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F9uBk%3D","line":3242,"in_reply_to":"AAAAUH%2F9uik%3D","updated":"2014-02-04 17:49:13.000000000","message":"Done","commit_id":"59debe7c977456059a45779b09c38f304899f7e3"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"715ced15dd93728683816c94d685a597e1b1f2ec","unresolved":false,"context_lines":[{"line_number":3239,"context_line":"            arch \u003d libvirt_utils.get_arch(image_meta)"},{"line_number":3240,"context_line":"            if arch in (\u0027ppc\u0027, \u0027ppc64\u0027):"},{"line_number":3241,"context_line":"                video.type \u003d \u0027vga\u0027"},{"line_number":3242,"context_line":"            elif CONF.spice.enabled:"},{"line_number":3243,"context_line":"                video.type \u003d \u0027qxl\u0027"},{"line_number":3244,"context_line":"            meta_prop \u003d image_meta.get(\u0027properties\u0027, {}) if image_meta else {}"},{"line_number":3245,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"AAAAUH%2F9uck%3D","line":3242,"in_reply_to":"AAAAUH%2F9uik%3D","updated":"2014-02-04 16:33:08.000000000","message":"We basically want 2 blocks. The first block of if/else that sets up the nova defaults based on arch/virt type/features and a second block which deals with user specified override from image meta.","commit_id":"59debe7c977456059a45779b09c38f304899f7e3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"262d972b78a12b5ac57f9227086b1477aba99e09","unresolved":false,"context_lines":[{"line_number":3232,"context_line":"        if add_video_driver:"},{"line_number":3233,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"},{"line_number":3234,"context_line":"            video \u003d vconfig.LibvirtConfigGuestVideo()"},{"line_number":3235,"context_line":"            # NOTE(ldbragst): The following logic sets the video.type"},{"line_number":3236,"context_line":"            # depending on supported defaults given the architecture,"},{"line_number":3237,"context_line":"            # virtualization type, and features. The video.type attribute can"},{"line_number":3238,"context_line":"            # be overridden by the user with image_meta[\u0027properties\u0027], which"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAUH%2F9jwM%3D","line":3235,"updated":"2014-02-06 02:32:19.000000000","message":"Given the discussion in the previous patch set, it might be worth mentioning a re-order of the logic in this patch in the commit message.  I tend to agree with John\u0027s point on the switch-style conditions, but also see Dan\u0027s point and don\u0027t mind how this turned out, it\u0027s kind of ugly either way so we\u0027re stuck with a lesser of two evils kind of decision.","commit_id":"b8ba5267223a4680fea1e9eca33443a211bac1ad"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"25aa9221b44beb10786251720e23565d0c3a6890","unresolved":false,"context_lines":[{"line_number":3232,"context_line":"        if add_video_driver:"},{"line_number":3233,"context_line":"            VALID_VIDEO_DEVICES \u003d (\"vga\", \"cirrus\", \"vmvga\", \"xen\", \"qxl\")"},{"line_number":3234,"context_line":"            video \u003d vconfig.LibvirtConfigGuestVideo()"},{"line_number":3235,"context_line":"            # NOTE(ldbragst): The following logic sets the video.type"},{"line_number":3236,"context_line":"            # depending on supported defaults given the architecture,"},{"line_number":3237,"context_line":"            # virtualization type, and features. The video.type attribute can"},{"line_number":3238,"context_line":"            # be overridden by the user with image_meta[\u0027properties\u0027], which"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAUH%2F9eDs%3D","line":3235,"in_reply_to":"AAAAUH%2F9jwM%3D","updated":"2014-02-06 17:35:13.000000000","message":"Done","commit_id":"b8ba5267223a4680fea1e9eca33443a211bac1ad"}]}