)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"0355f3cf7fbd57a08b35db5072ad945cf11724ed","unresolved":false,"context_lines":[{"line_number":5,"context_line":"CommitDate: 2016-09-05 17:08:24 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Clean up libvirt guest code"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Libvirt\u0027s function blockJobInfo returns structure with info about operation on"},{"line_number":10,"context_line":"block device. In this structure there current and final value to indicate"},{"line_number":11,"context_line":"current and final cursor position for operation, value of bandwidth for"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a89bdaa_ab4502b4","line":8,"updated":"2016-09-06 10:28:44.000000000","message":"Also, does this summary sound any more accurate?\n\n\"libvirt: Clean up block device job status reporting code\"","commit_id":"ccf9db18e090e7f164d0f222da1b980983ecd79e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6cdc831b86fb0e30c5236f9700366b02417fbc8c","unresolved":false,"context_lines":[{"line_number":5,"context_line":"CommitDate: 2016-09-05 17:08:24 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Clean up libvirt guest code"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Libvirt\u0027s function blockJobInfo returns structure with info about operation on"},{"line_number":10,"context_line":"block device. In this structure there current and final value to indicate"},{"line_number":11,"context_line":"current and final cursor position for operation, value of bandwidth for"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a89bdaa_a119a37b","line":8,"in_reply_to":"9a89bdaa_ab4502b4","updated":"2016-09-06 10:54:01.000000000","message":"+1","commit_id":"ccf9db18e090e7f164d0f222da1b980983ecd79e"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"93a5bb5ff1152620ed8b9b69f305e2373689077b","unresolved":false,"context_lines":[{"line_number":5,"context_line":"CommitDate: 2016-09-05 17:08:24 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Clean up libvirt guest code"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Libvirt\u0027s function blockJobInfo returns structure with info about operation on"},{"line_number":10,"context_line":"block device. In this structure there current and final value to indicate"},{"line_number":11,"context_line":"current and final cursor position for operation, value of bandwidth for"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a89bdaa_68c1332e","line":8,"in_reply_to":"9a89bdaa_ab4502b4","updated":"2016-09-06 13:36:35.000000000","message":"Done","commit_id":"ccf9db18e090e7f164d0f222da1b980983ecd79e"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"5b85f478305f39b0a578a89b8e45c64f81c4f180","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Libvirt\u0027s function blockJobInfo returns structure with info about operation on"},{"line_number":10,"context_line":"block device. In this structure there current and final value to indicate"},{"line_number":11,"context_line":"current and final cursor position for operation, value of bandwidth for"},{"line_number":12,"context_line":"operation and type of operation. All of those values are integer values and"},{"line_number":13,"context_line":"can\u0027t be unset. Because of that there is no need to get those values with"},{"line_number":14,"context_line":"specified defaults in BlockDevice.get_job_info() method."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a89bdaa_2ba9f2df","line":12,"updated":"2016-09-06 10:21:09.000000000","message":"Nit, I might rephrase the sentence:\n\n\"In this structure there current and final value to indicate current and final cursor position for operation, value of bandwidth for operation and type of operation.\"\n\nWith:\n\n\"In this structure, there are four fields: \u0027cur\u0027 and \u0027end\u0027 to indicate current and final cursor position for a live block operation; \u0027bandwidth\u0027 to set bandwidth limit for the active block operation; finally, \u0027type\u0027 to specify the type of block operation (e.g. blockCommit(), blockRebase(), etc).\"","commit_id":"ccf9db18e090e7f164d0f222da1b980983ecd79e"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6cdc831b86fb0e30c5236f9700366b02417fbc8c","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Libvirt\u0027s function blockJobInfo returns structure with info about operation on"},{"line_number":10,"context_line":"block device. In this structure there current and final value to indicate"},{"line_number":11,"context_line":"current and final cursor position for operation, value of bandwidth for"},{"line_number":12,"context_line":"operation and type of operation. All of those values are integer values and"},{"line_number":13,"context_line":"can\u0027t be unset. Because of that there is no need to get those values with"},{"line_number":14,"context_line":"specified defaults in BlockDevice.get_job_info() method."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a89bdaa_81357fdb","line":12,"in_reply_to":"9a89bdaa_2ba9f2df","updated":"2016-09-06 10:54:01.000000000","message":"+1\n\nBut don\u0027t sweat either of these.","commit_id":"ccf9db18e090e7f164d0f222da1b980983ecd79e"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"93a5bb5ff1152620ed8b9b69f305e2373689077b","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Libvirt\u0027s function blockJobInfo returns structure with info about operation on"},{"line_number":10,"context_line":"block device. In this structure there current and final value to indicate"},{"line_number":11,"context_line":"current and final cursor position for operation, value of bandwidth for"},{"line_number":12,"context_line":"operation and type of operation. All of those values are integer values and"},{"line_number":13,"context_line":"can\u0027t be unset. Because of that there is no need to get those values with"},{"line_number":14,"context_line":"specified defaults in BlockDevice.get_job_info() method."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9a89bdaa_48c62f49","line":12,"in_reply_to":"9a89bdaa_2ba9f2df","updated":"2016-09-06 13:36:35.000000000","message":"Done","commit_id":"ccf9db18e090e7f164d0f222da1b980983ecd79e"}],"nova/tests/unit/virt/libvirt/test_guest.py":[{"author":{"_account_id":13431,"name":"Evgeny Antyshev","email":"eantyshev@gmail.com","username":"eantyshev"},"change_message_id":"b57fc21042691290263d37e02ca45ccee10f5428","unresolved":false,"context_lines":[{"line_number":651,"context_line":""},{"line_number":652,"context_line":"    def test_wait_for_job_clean(self):"},{"line_number":653,"context_line":"        self.domain.blockJobInfo.return_value \u003d {"},{"line_number":654,"context_line":"            \"type\": 0,"},{"line_number":655,"context_line":"            \"bandwidth\": 0,"},{"line_number":656,"context_line":"            \"cur\": 100,"},{"line_number":657,"context_line":"            \"end\": 100}"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a8bc5a2_caf8bbc7","line":654,"updated":"2016-09-28 09:44:57.000000000","message":"type is never 0 for real job,\nand this value is hardcoded in wait_for_job currently,\nso this is a bad choice for unit test.\n\nIf you intend to test here whether wait_for_job(wait_for_job_clean\u003dTrue) is true if cur\u003d\u003dend but job still exists, then you should change assertFalse to assertTrue below.","commit_id":"969c0f6644ee90768c85ebd44da95fd2849d441a"}],"nova/virt/libvirt/guest.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ffbc9d0c286cc4c61147461c0191a06f6600e972","unresolved":false,"context_lines":[{"line_number":650,"context_line":"        :returns: BlockDeviceJobInfo or None"},{"line_number":651,"context_line":"        \"\"\""},{"line_number":652,"context_line":"        status \u003d self._guest._domain.blockJobInfo(self._disk, flags\u003d0)"},{"line_number":653,"context_line":"        if status !\u003d -1:"},{"line_number":654,"context_line":"            return BlockDeviceJobInfo("},{"line_number":655,"context_line":"                job\u003dstatus[\"type\"],"},{"line_number":656,"context_line":"                bandwidth\u003dstatus[\"bandwidth\"],"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a8bc5a2_df09d285","line":653,"updated":"2016-09-26 14:44:37.000000000","message":"Not strictly related to this change, but lets take the opportunity to turn this into a fix for bug 1627134.\n\nLooking at this check more critically, this is clearly nonsense: blockJobInfo has never returned an integer.\n\nThe call is documented here: http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetBlockJobInfo\n\nIt can return:\n\n-1: An error occurred\n 0: The job does not exists\n 1: The job exists, and we are returning info about it.\n\nLooking (yet) again at libvirt-python, it returns:\n\n-1: None\n 0: {}\n 1: A fully populated dict\n\nSo the return from blockJobInfo is None if there was an error, an empty dict if the blockJobInfo wasn\u0027t found, or a fully populated dict if it was found.\n\nIoW there are still no circumstances under which a partially populated dict can be returned, so the primary purpose of this cleanup is still good.\n\nHowever, if we let python do a falsey check above (eurgh, I hate those), i.e.:\n\nif status:\n  ...\n\nThen I think we\u0027re good to go. Slightly concerned that we\u0027re swallowing an error case here (None), but if it doesn\u0027t raise a usable Exception I\u0027m not sure what we can do with it anyway.","commit_id":"ba1957608f32ef45bd092e5532b91d34cbd152e1"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"56df9d773d29a522b445d7541be5e154c47f225b","unresolved":false,"context_lines":[{"line_number":650,"context_line":"        :returns: BlockDeviceJobInfo or None"},{"line_number":651,"context_line":"        \"\"\""},{"line_number":652,"context_line":"        status \u003d self._guest._domain.blockJobInfo(self._disk, flags\u003d0)"},{"line_number":653,"context_line":"        if status !\u003d -1:"},{"line_number":654,"context_line":"            return BlockDeviceJobInfo("},{"line_number":655,"context_line":"                job\u003dstatus[\"type\"],"},{"line_number":656,"context_line":"                bandwidth\u003dstatus[\"bandwidth\"],"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a8bc5a2_17e10cff","line":653,"in_reply_to":"5a8bc5a2_df09d285","updated":"2016-09-27 20:27:16.000000000","message":"Yes, I also found that it is not really properly handled here and I changed it.\nI hope it will be better now","commit_id":"ba1957608f32ef45bd092e5532b91d34cbd152e1"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0422aa0d493ac4c1576afef50985e6de4ab421ce","unresolved":false,"context_lines":[{"line_number":655,"context_line":"            LOG.warning(_LW(\"Failed to get block job info on %(disk)s in \""},{"line_number":656,"context_line":"                            \"%(guest)s\"),"},{"line_number":657,"context_line":"                        {\u0027disk\u0027: self._disk, \u0027guest\u0027: self._guest.uuid})"},{"line_number":658,"context_line":"            return -1"},{"line_number":659,"context_line":""},{"line_number":660,"context_line":"        if len(status) \u003d\u003d 0:"},{"line_number":661,"context_line":"            LOG.debug(\"No any block job on %(disk)s in %(guest)s\","}],"source_content_type":"text/x-python","patch_set":5,"id":"5a8bc5a2_04f29c9a","line":658,"updated":"2016-09-28 09:12:00.000000000","message":"I was thinking that you\u0027d retain the original behaviour of returning None, but calling this out explicitly is better.\n\nHowever, you shouldn\u0027t return -1 here. You should raise your exception from line 724 below here instead, and not handle it below at all.","commit_id":"969c0f6644ee90768c85ebd44da95fd2849d441a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0422aa0d493ac4c1576afef50985e6de4ab421ce","unresolved":false,"context_lines":[{"line_number":660,"context_line":"        if len(status) \u003d\u003d 0:"},{"line_number":661,"context_line":"            LOG.debug(\"No any block job on %(disk)s in %(guest)s\","},{"line_number":662,"context_line":"                      {\u0027disk\u0027: self._disk, \u0027guest\u0027: self._guest.uuid})"},{"line_number":663,"context_line":"            return 0"},{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        return BlockDeviceJobInfo("},{"line_number":666,"context_line":"            job\u003dstatus[\"type\"],"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a8bc5a2_647d20e7","line":663,"updated":"2016-09-28 09:12:00.000000000","message":"Again, calling this out is great, but given that this function returns a BlockDeviceJobInfo object, returning an integer as an alternative is surprising[1]. You should return None here.\n\n[1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment if you don\u0027t get the reference :)","commit_id":"969c0f6644ee90768c85ebd44da95fd2849d441a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4d9098547b02cdc316ab050fad8a9206cfd8c819","unresolved":false,"context_lines":[{"line_number":725,"context_line":"            return False"},{"line_number":726,"context_line":""},{"line_number":727,"context_line":"        if status \u003d\u003d 0:"},{"line_number":728,"context_line":"            return False"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"        if wait_for_job_clean:"},{"line_number":731,"context_line":"            job_ended \u003d status.job \u003d\u003d 0"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a8bc5a2_502a9ee8","line":728,"updated":"2016-09-28 10:06:53.000000000","message":"You need to incorporate this into the if/else block below.","commit_id":"969c0f6644ee90768c85ebd44da95fd2849d441a"},{"author":{"_account_id":13431,"name":"Evgeny Antyshev","email":"eantyshev@gmail.com","username":"eantyshev"},"change_message_id":"b57fc21042691290263d37e02ca45ccee10f5428","unresolved":false,"context_lines":[{"line_number":728,"context_line":"            return False"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"        if wait_for_job_clean:"},{"line_number":731,"context_line":"            job_ended \u003d status.job \u003d\u003d 0"},{"line_number":732,"context_line":"        else:"},{"line_number":733,"context_line":"            # NOTE(slaweq): because of bug in libvirt, which is described in"},{"line_number":734,"context_line":"            # http://www.redhat.com/archives/libvir-list/2016-September/msg00017.html"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a8bc5a2_c73996da","line":731,"updated":"2016-09-28 09:44:57.000000000","message":"This line will not be reached providing your change to get_job_info() applied.","commit_id":"969c0f6644ee90768c85ebd44da95fd2849d441a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4d9098547b02cdc316ab050fad8a9206cfd8c819","unresolved":false,"context_lines":[{"line_number":728,"context_line":"            return False"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"        if wait_for_job_clean:"},{"line_number":731,"context_line":"            job_ended \u003d status.job \u003d\u003d 0"},{"line_number":732,"context_line":"        else:"},{"line_number":733,"context_line":"            # NOTE(slaweq): because of bug in libvirt, which is described in"},{"line_number":734,"context_line":"            # http://www.redhat.com/archives/libvir-list/2016-September/msg00017.html"}],"source_content_type":"text/x-python","patch_set":5,"id":"5a8bc5a2_708cfa35","line":731,"in_reply_to":"5a8bc5a2_c73996da","updated":"2016-09-28 10:06:53.000000000","message":"The only way we would ever have previously returned type of 0 here is if blockJobInfo had returned {}, meaning the job no longer exists. In that case we\u0027re proposing that get_job_info() returns None instead, so we just need to update this accordingly.","commit_id":"969c0f6644ee90768c85ebd44da95fd2849d441a"}]}