)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"257a7e5f47e39c366cec0ac86c7fa3d04697ad13","unresolved":true,"context_lines":[{"line_number":11,"context_line":"we aren\u0027t using the libvirt built in mechanism to enable it"},{"line_number":12,"context_line":"when grabbing the right firmware."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: 1958636"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I890b3021a29fa546d9e36b21b1111e8537cd0020"},{"line_number":17,"context_line":"Signed-off-by: Imran Hussain \u003cih@imranh.co.uk\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"e796f089_1230ff91","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":19},"updated":"2022-01-28 19:29:40.000000000","message":"i still dont like treating this as a nova bug since its enabled by default\nbut i guess we can. there a bug in libvirt however.\neither a docs bug or code bug.\n\nso this is really a workaournd.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1656da72d979e77307cc0681217e7db1b8732588","unresolved":true,"context_lines":[{"line_number":11,"context_line":"we aren\u0027t using the libvirt built in mechanism to enable it"},{"line_number":12,"context_line":"when grabbing the right firmware."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: 1958636"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I890b3021a29fa546d9e36b21b1111e8537cd0020"},{"line_number":17,"context_line":"Signed-off-by: Imran Hussain \u003cih@imranh.co.uk\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"f48e8f7b_7090d46d","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":19},"in_reply_to":"29ca5831_fc4fc407","updated":"2022-01-30 21:59:52.000000000","message":"The error is not being thrown by nova-compute. it is being raided by the libvirt packaged in ubuntu and nova-comptue is just logging that libvirt error,\nlibvirt is not part of openstack,  the libvirt packaged in ubuntu is not confirming to the documentation for libvirt upstream. my understandign is that this was also not required on fedora wehn the feature was first developed. as such it should not need to enabled by nova. nova can workaound this distro differnce but \nupstream libvirt states that SMM is enabled by default.\n\nnova does not override that behavior today by enableing or disabling system managmenet mode. so that is why i state this is not a nova bug. its a  libvirt one. nova can paper over the libvir bug to normalise the behviaor but ubuntu shoudl also fix this in there package.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"e6ade17c8722d7b5fde19544a00c36fb44e5b031","unresolved":true,"context_lines":[{"line_number":11,"context_line":"we aren\u0027t using the libvirt built in mechanism to enable it"},{"line_number":12,"context_line":"when grabbing the right firmware."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: 1958636"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I890b3021a29fa546d9e36b21b1111e8537cd0020"},{"line_number":17,"context_line":"Signed-off-by: Imran Hussain \u003cih@imranh.co.uk\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"29ca5831_fc4fc407","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":19},"in_reply_to":"e796f089_1230ff91","updated":"2022-01-28 20:03:02.000000000","message":"From my perspective I followed the Openstack docs and nove-compute throws errors. The libvirt driver is where the xml that doesn\u0027t work is generated, libvirt driver is in the Nova project.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"21c69d4fb6b2d2d70a5df326fa900265eae896e5","unresolved":false,"context_lines":[{"line_number":11,"context_line":"we aren\u0027t using the libvirt built in mechanism to enable it"},{"line_number":12,"context_line":"when grabbing the right firmware."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Closes-Bug: 1958636"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"Change-Id: I890b3021a29fa546d9e36b21b1111e8537cd0020"},{"line_number":17,"context_line":"Signed-off-by: Imran Hussain \u003cih@imranh.co.uk\u003e"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"16b9404d_0f6b2793","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":19},"in_reply_to":"f48e8f7b_7090d46d","updated":"2022-01-31 10:35:16.000000000","message":"Ack","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"f2f36fa22ddc7e865db16e8df40657267157e9a4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"802f7898_ef6d559b","updated":"2022-01-21 09:58:37.000000000","message":"Alternative https://review.opendev.org/c/openstack/nova/+/825729","commit_id":"11e2629e48006aa5c9476bb79cb1ca9fe56a5a56"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"a049d852f61d4b3b0791cf55dfab255e0547b5fa","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"93f9d4a8_c43c3ab2","updated":"2022-01-20 13:21:34.000000000","message":"I dropped the type checking from the get_loader() func due to not being able to quite figure it out, some suggestions would be appreciated.","commit_id":"11e2629e48006aa5c9476bb79cb1ca9fe56a5a56"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"98a9076459898b1b407da9b8762a0d85a97cad23","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"fa8b187d_bd565e04","updated":"2022-01-20 13:20:28.000000000","message":"Turns out my issue (http://lists.openstack.org/pipermail/openstack-discuss/2022-January/026796.html) is actually straight up a bug in Nova.\n\nHere\u0027s a patch that works on my systems.","commit_id":"11e2629e48006aa5c9476bb79cb1ca9fe56a5a56"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"db907aeadaa2804fb4614c7cc3052f219cf16fe9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"8f6c8f84_4e6df6c6","in_reply_to":"93f9d4a8_c43c3ab2","updated":"2022-01-28 19:53:11.000000000","message":"Done","commit_id":"11e2629e48006aa5c9476bb79cb1ca9fe56a5a56"},{"author":{"_account_id":19234,"name":"Alexey Stupnikov","email":"aleksey.stupnikov@gmail.com","username":"astupnikov"},"change_message_id":"e2a180e015a3ee87c55f6688c1353473ea522e83","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"22353f5c_ad8ab711","updated":"2022-01-21 13:29:45.000000000","message":"IMO it would be great to have more details in commit message about this change. Maybe it also make sense to consider adding release note.","commit_id":"b73c772a57c735a5525ce01549895910bf175155"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"d421fa2634e92933f25a2055bd07c6b8d167a044","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c4c53c4c_abaebb9f","in_reply_to":"22353f5c_ad8ab711","updated":"2022-01-21 13:48:00.000000000","message":"Can do, was ideally waiting for some feedback in the bug (https://bugs.launchpad.net/nova/+bug/1958636) before going ahead and making it more presentable. Will add more details in the commit message and if this approach is agreed upon then can add a release note if deemed necessary.","commit_id":"b73c772a57c735a5525ce01549895910bf175155"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"27a1d4872e6df2544bd51fbf274e74026af74def","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"f5a51f24_1ded0c51","in_reply_to":"c4c53c4c_abaebb9f","updated":"2022-01-31 19:28:13.000000000","message":"Done","commit_id":"b73c772a57c735a5525ce01549895910bf175155"},{"author":{"_account_id":19234,"name":"Alexey Stupnikov","email":"aleksey.stupnikov@gmail.com","username":"astupnikov"},"change_message_id":"48ff61a7b886d870a1d32a2c6ed72a26894ade4e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"37833ad7_91d895ab","updated":"2022-01-28 15:34:32.000000000","message":"Hi Stephen. This problem looks like libvirt bug to me: for some reason smm is not added to list of domains\u0027s features automatically. At the same time I don\u0027t know enough about secure boot support in Nova and may miss something. Could you take a look when you have time?","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":19234,"name":"Alexey Stupnikov","email":"aleksey.stupnikov@gmail.com","username":"astupnikov"},"change_message_id":"32df38e2666b7cc47f85bd5c5e96458d84b77a1f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"0d7f0db1_eea5e4ad","updated":"2022-01-25 15:53:20.000000000","message":"I took some time to analyze the background for this problems and would like to share my findings.\n\nSecure boot support was added following BP https://specs.openstack.org/openstack/nova-specs/specs/wallaby/implemented/allow-secure-boot-for-qemu-kvm-guests.html . Blueprint\u0027s description is very detailed and contains the points similar to what you have expressed: SMM should either be enabled manually or libvirt’s firmware auto-selection feature should add such capabilities. The domain XML syntax for firmware auto-selection seem to be [1].\n\nA number of changes https://review.opendev.org/q/topic:bp/allow-secure-boot-for-qemu-kvm-guests were used to implement secure boot feature in nova. From patches it looks like developers only implemented second way (libvirt’s firmware auto-selection). So there was no option to manually define SMM, but firmware auto-selection should be enough to do the trick.\n\nI also found that libvirt 6.0.0 had related problems (at least when it comes to RHEL https://bugzilla.redhat.com/show_bug.cgi?id\u003d1929357 ). So I think that first step here is to double-check which XML was passed from Nova to libvirt to confirm Nova or libvirt problem.\n\n\n[1]\n  \u003cos firmware\u003d\u0027efi\u0027\u003e\n    \u003cloader secure\u003d\u0027yes\u0027/\u003e\n  \u003c/os\u003e","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"409605fc697393c1f5471650992fbeda5ec1dea3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"961aa34c_d2bdc2f8","updated":"2022-01-28 16:21:10.000000000","message":"This is a good start. We need tests to prevent regressions though. There are a variety of tests in \u0027nova/tests/unit/virt/libvirt/test_driver.py\u0027 that you can build on (search for secure boot, e.g. \u0027test_get_guest_config_with_secure_boot_required\u0027). Also, I have some comments on what\u0027s here","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"af4d81e0c852a4b1d73f30f1e3a9336017b46d40","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"cb5ef4d3_bb01556a","in_reply_to":"0d7f0db1_eea5e4ad","updated":"2022-01-25 16:35:06.000000000","message":"Hi Alexey,\n\nThanks for taking the time to look in to this.\n\nI created a bug with more info and my journey about this stuff at https://bugs.launchpad.net/nova/+bug/1958636, it provides some context for my struggles.\n\nI firmly believe this is a Nova libvirt driver bug. I posted the XML that created by Nova in my mailing lists posts and you can see it doesn\u0027t align with what the linvirt documentation says at https://libvirt.org/formatdomain.html#bios-bootloader.","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"27a1d4872e6df2544bd51fbf274e74026af74def","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5f2d2d6d_dd6c9de8","in_reply_to":"cb5ef4d3_bb01556a","updated":"2022-01-31 19:28:13.000000000","message":"Done","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"3c434e2baf8863f0f5c0b0693397fa989f682095","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"55f165e1_22382b00","updated":"2022-01-28 17:46:43.000000000","message":"Hi Stephen, thanks for the feedback, I\u0027ll look at adding test when I get a moment. Have pushed the quick fixes you suggested though.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"257a7e5f47e39c366cec0ac86c7fa3d04697ad13","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"deb847d4_9d7b83a0","updated":"2022-01-28 19:29:40.000000000","message":"ill abandon my poc in favor of this one.\n\nwe will need test coverage and a release note for this too.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1c0a05fa9f7af1862d5a4bee345db823e9d38ed3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"fba7f052_5e277618","in_reply_to":"55f165e1_22382b00","updated":"2022-01-28 19:16:42.000000000","message":"Great. Looking forward to seeing tests so we can move forward with this and potentially backport it","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"7f4eec95034eb73e4026b701bd0ccc868350e744","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"71a254ef_5fd1a46c","in_reply_to":"fba7f052_5e277618","updated":"2022-01-29 18:44:47.000000000","message":"Added a single test that should cover the cases I think.\n\n`tox -- LibvirtConnTestCase.test_get_guest_config_with_secure_boot_and_smm_required`\n\nLeft a bunch of comments explaining what\u0027s going on to hopefully make my understanding clear.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"ec911d72aabd15fc55dd486acf9efd9821e12c1e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"076c2d7c_8dcc0f3b","updated":"2022-01-31 10:58:11.000000000","message":"(+1 to the main idea; haven\u0027t looked deeply at the test, but only the functional code)","commit_id":"d9f5258ce7050d991b586a94a4a403227dd66078"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8e0cec1957586276aa35d010fd59e9c14b9def3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"428ca6bb_7a71cef2","updated":"2022-01-31 12:54:02.000000000","message":"recheck: over all im +1 on this patch but there are 2 changes i would like to see before im +2.\n1.) can you add a unit test for the get_loaders function to test its behavior independelty form where its used with the driver.\n2.) can you add a release note with a short summary for operators, effectivly we are now support secure boot on platform where libvirt does not enable system management mode by default such as ubuntu.\n\nwith those changes i think i would be happy to merge this patch. i dont think we require functional tests in this case, i am happy that the unit test woudl provide enough coverage for the unit test generation so once you adress 1 i think this is good form a test perspective.","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"8301e9119207474409e618a027f283bb76c17fe8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"198d634f_ed80cad6","in_reply_to":"428ca6bb_7a71cef2","updated":"2022-01-31 15:44:20.000000000","message":"Added both requests.","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c08995938d47aaf2304b00bf503090bec65a6bfd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"94575b91_580415b3","updated":"2022-02-17 11:57:22.000000000","message":"Small typo in the code. Fix that and I\u0027m +2 (Sean: feel free to ninja-approve once fixed)","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"6ee2d1c9e30182e8c3c82215febde24a67476bca","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"77b8f9dd_73661038","updated":"2022-02-16 09:04:31.000000000","message":"What needs to happen next for this patch to land? CR?","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ae6006220de99b8853a8852ce65d96fd59085b5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"f3085dad_949caa08","updated":"2022-02-16 13:41:54.000000000","message":"this should be functionally complete and you have addressed my previous comments so i think we can proceed with this.\n\nsetting review priority to see if we can push this along.","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"18bcf5c871a0d146542f47d39b09aca9de1f97fb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"7de20eaf_2e70a303","updated":"2022-02-17 12:47:11.000000000","message":"Sean \u0026 Stephen,\n\nPut in all your suggestions.","commit_id":"6ad789010043dc4dcf8d1c0f497b6c728d230f45"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d827a37338fea2f3a0d6d66529f7391f8ddd651f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"2c419a39_1b33aef4","updated":"2022-02-17 12:51:19.000000000","message":"fast approving as suggested by stephen.\nin general i think the indentaion will fail pep8 if so please respin\nbut assuming you have already checked that and our pep8/hackeing rules allow\nyou to skip the extra 4 spaces ill approve as is.\n\nthanks for reworking this.","commit_id":"6ad789010043dc4dcf8d1c0f497b6c728d230f45"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"486558d1e28c4c55857a5e9556cfae9f2d349c89","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":11,"id":"2434548e_e35913dc","updated":"2022-02-17 14:44:32.000000000","message":"nova-multi-cell failed because a VM didn\u0027t have connectivity I think\n\n\n```\n### route -n\nKernel IP routing table\nDestination     Gateway         Genmask         Flags Metric Ref    Use Iface\n0.0.0.0         10.10.10.1      0.0.0.0         UG    0      0        0 eth0\n10.10.10.0      0.0.0.0         255.255.255.0   U     0      0        0 eth0\n169.254.169.254 10.10.10.2      255.255.255.255 UGH   0      0        0 eth0\n### cat /etc/resolv.conf\nnameserver 127.0.0.1\n### ping -c 5 10.10.10.1\nPING 10.10.10.1 (10.10.10.1): 56 data bytes\n\n--- 10.10.10.1 ping statistics ---\n5 packets transmitted, 0 packets received, 100% packet loss\n```\n\nIs that me or flaky CI?","commit_id":"6ad789010043dc4dcf8d1c0f497b6c728d230f45"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"535f4bbdb4993aacead32133344cf992fe0561b7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"304db689_3906a143","updated":"2022-02-17 18:16:34.000000000","message":"recheck","commit_id":"6ad789010043dc4dcf8d1c0f497b6c728d230f45"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cedc25bedbe477e9f366a1f727486cf76bf9cc87","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"59d85c7b_ce450670","updated":"2022-02-17 14:38:20.000000000","message":"recheck","commit_id":"6ad789010043dc4dcf8d1c0f497b6c728d230f45"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"2b5848583df406b5244f4f1f1ded05c9ae0a045e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"82deb76e_c77c31dd","updated":"2022-02-17 16:27:05.000000000","message":"recheck","commit_id":"6ad789010043dc4dcf8d1c0f497b6c728d230f45"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8e0cec1957586276aa35d010fd59e9c14b9def3","unresolved":true,"context_lines":[{"line_number":5106,"context_line":"            self.assertTrue(any(isinstance(feature,"},{"line_number":5107,"context_line":"              vconfig.LibvirtConfigGuestFeatureSMM)"},{"line_number":5108,"context_line":"                for feature in cfg.features))"},{"line_number":5109,"context_line":"        elif not requires_smm and any(isinstance(feature,"},{"line_number":5110,"context_line":"          vconfig.LibvirtConfigGuestFeatureSMM) for feature in cfg.features):"},{"line_number":5111,"context_line":"            self.assertRaises("},{"line_number":5112,"context_line":"                exception.FirmwareSMMNotSupported,"},{"line_number":5113,"context_line":"                drvr._get_guest_config,"},{"line_number":5114,"context_line":"                instance_ref, [], image_meta, disk_info)"},{"line_number":5115,"context_line":""},{"line_number":5116,"context_line":"    @ddt.data(True, False)"},{"line_number":5117,"context_line":"    def test_get_guest_config_with_secure_boot_required("}],"source_content_type":"text/x-python","patch_set":7,"id":"0ee1f4c8_c666a686","line":5114,"range":{"start_line":5109,"start_character":8,"end_line":5114,"end_character":56},"updated":"2022-01-31 12:54:02.000000000","message":"so technically this is just asserting that we dont request it and use the libvirt default if we have not enabel secure boot.\n\nhowever its valid to enable SMM when not using secure boot in the future if we have a feature that requires it. so this might get droped in a future patch if that happens but for now its correct","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"8984ddd383ee6ab0e00b89242c70bb890ad4568a","unresolved":true,"context_lines":[{"line_number":5106,"context_line":"            self.assertTrue(any(isinstance(feature,"},{"line_number":5107,"context_line":"              vconfig.LibvirtConfigGuestFeatureSMM)"},{"line_number":5108,"context_line":"                for feature in cfg.features))"},{"line_number":5109,"context_line":"        elif not requires_smm and any(isinstance(feature,"},{"line_number":5110,"context_line":"          vconfig.LibvirtConfigGuestFeatureSMM) for feature in cfg.features):"},{"line_number":5111,"context_line":"            self.assertRaises("},{"line_number":5112,"context_line":"                exception.FirmwareSMMNotSupported,"},{"line_number":5113,"context_line":"                drvr._get_guest_config,"},{"line_number":5114,"context_line":"                instance_ref, [], image_meta, disk_info)"},{"line_number":5115,"context_line":""},{"line_number":5116,"context_line":"    @ddt.data(True, False)"},{"line_number":5117,"context_line":"    def test_get_guest_config_with_secure_boot_required("}],"source_content_type":"text/x-python","patch_set":7,"id":"116e911a_ccc2067e","line":5114,"range":{"start_line":5109,"start_character":8,"end_line":5114,"end_character":56},"in_reply_to":"0ee1f4c8_c666a686","updated":"2022-01-31 15:11:47.000000000","message":"My logic for this is in the code comments, I looked at what ovmf maintainers say about SMM and they said:\n\n`this is supposed to be a security feature, and fallbacks are not allowed`\n\nSo the thinking is, if you enable SMM for security but aren\u0027t doing secure boot then unit test should bomb out, although the current code would actually allow that behaviour.","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"9a3cf5ce3f7ede8359feb9e8ee64f8fc7b4f88e2","unresolved":true,"context_lines":[{"line_number":5106,"context_line":"            self.assertTrue(any(isinstance(feature,"},{"line_number":5107,"context_line":"              vconfig.LibvirtConfigGuestFeatureSMM)"},{"line_number":5108,"context_line":"                for feature in cfg.features))"},{"line_number":5109,"context_line":"        elif not requires_smm and any(isinstance(feature,"},{"line_number":5110,"context_line":"          vconfig.LibvirtConfigGuestFeatureSMM) for feature in cfg.features):"},{"line_number":5111,"context_line":"            self.assertRaises("},{"line_number":5112,"context_line":"                exception.FirmwareSMMNotSupported,"},{"line_number":5113,"context_line":"                drvr._get_guest_config,"},{"line_number":5114,"context_line":"                instance_ref, [], image_meta, disk_info)"},{"line_number":5115,"context_line":""},{"line_number":5116,"context_line":"    @ddt.data(True, False)"},{"line_number":5117,"context_line":"    def test_get_guest_config_with_secure_boot_required("}],"source_content_type":"text/x-python","patch_set":7,"id":"1421cd82_3f1f05d1","line":5114,"range":{"start_line":5109,"start_character":8,"end_line":5114,"end_character":56},"in_reply_to":"116e911a_ccc2067e","updated":"2022-01-31 15:43:17.000000000","message":"the SMM feature is not defiend by OVMF and predates its use for secure boot.\ninfact SMM can be used with bios boot without uefi or secure boot.\n\nall vms created by libvirt 2.1.0+ by default should have SMM mode enable today.\nin recent year the intoduction fo support for uefi and ovmf has complicated matters as usfi and secure boot also have depencices on SMM for the secure boot implemeation but it has other security uses so it not an error to enable it in general wehn not using secure boot.","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"de30dbdb0ce195ccee32ec0c312d1213936c5bca","unresolved":true,"context_lines":[{"line_number":5106,"context_line":"            self.assertTrue(any(isinstance(feature,"},{"line_number":5107,"context_line":"              vconfig.LibvirtConfigGuestFeatureSMM)"},{"line_number":5108,"context_line":"                for feature in cfg.features))"},{"line_number":5109,"context_line":"        elif not requires_smm and any(isinstance(feature,"},{"line_number":5110,"context_line":"          vconfig.LibvirtConfigGuestFeatureSMM) for feature in cfg.features):"},{"line_number":5111,"context_line":"            self.assertRaises("},{"line_number":5112,"context_line":"                exception.FirmwareSMMNotSupported,"},{"line_number":5113,"context_line":"                drvr._get_guest_config,"},{"line_number":5114,"context_line":"                instance_ref, [], image_meta, disk_info)"},{"line_number":5115,"context_line":""},{"line_number":5116,"context_line":"    @ddt.data(True, False)"},{"line_number":5117,"context_line":"    def test_get_guest_config_with_secure_boot_required("}],"source_content_type":"text/x-python","patch_set":7,"id":"de7abe19_bf70823b","line":5114,"range":{"start_line":5109,"start_character":8,"end_line":5114,"end_character":56},"in_reply_to":"1421cd82_3f1f05d1","updated":"2022-01-31 15:49:39.000000000","message":"Thanks for explaining.\n\nI will remove this check then.","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"7084ee41d7e3ef7630ae4593aac3de1308795327","unresolved":false,"context_lines":[{"line_number":5106,"context_line":"            self.assertTrue(any(isinstance(feature,"},{"line_number":5107,"context_line":"              vconfig.LibvirtConfigGuestFeatureSMM)"},{"line_number":5108,"context_line":"                for feature in cfg.features))"},{"line_number":5109,"context_line":"        elif not requires_smm and any(isinstance(feature,"},{"line_number":5110,"context_line":"          vconfig.LibvirtConfigGuestFeatureSMM) for feature in cfg.features):"},{"line_number":5111,"context_line":"            self.assertRaises("},{"line_number":5112,"context_line":"                exception.FirmwareSMMNotSupported,"},{"line_number":5113,"context_line":"                drvr._get_guest_config,"},{"line_number":5114,"context_line":"                instance_ref, [], image_meta, disk_info)"},{"line_number":5115,"context_line":""},{"line_number":5116,"context_line":"    @ddt.data(True, False)"},{"line_number":5117,"context_line":"    def test_get_guest_config_with_secure_boot_required("}],"source_content_type":"text/x-python","patch_set":7,"id":"71894050_27f3d9d9","line":5114,"range":{"start_line":5109,"start_character":8,"end_line":5114,"end_character":56},"in_reply_to":"de7abe19_bf70823b","updated":"2022-01-31 16:00:41.000000000","message":"Done","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c08995938d47aaf2304b00bf503090bec65a6bfd","unresolved":false,"context_lines":[{"line_number":5074,"context_line":"        drvr._host._supports_secure_boot \u003d True"},{"line_number":5075,"context_line":""},{"line_number":5076,"context_line":"        # NOTE(imranh2): Current way of gathering firmwares is inflexible"},{"line_number":5077,"context_line":"        # nova/tests/fixtures/libvirt.py FakeLoaders has requies-smm"},{"line_number":5078,"context_line":"        # defined. do the following to make sure we get this programtically"},{"line_number":5079,"context_line":"        # in the future we should test firmwares that both do and don\u0027t"},{"line_number":5080,"context_line":"        # require smm but the current way firmware is selected doesn\u0027t"}],"source_content_type":"text/x-python","patch_set":10,"id":"cd89d4f8_c156b4e6","line":5077,"range":{"start_line":5077,"start_character":57,"end_line":5077,"end_character":64},"updated":"2022-02-17 11:57:22.000000000","message":"requires","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"}],"nova/virt/libvirt/config.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"409605fc697393c1f5471650992fbeda5ec1dea3","unresolved":true,"context_lines":[{"line_number":2692,"context_line":""},{"line_number":2693,"context_line":"    def __init__(self, **kwargs):"},{"line_number":2694,"context_line":"        super(LibvirtConfigGuestFeatureSMM, self).__init__(\"smm\","},{"line_number":2695,"context_line":"                                                                 **kwargs)"},{"line_number":2696,"context_line":""},{"line_number":2697,"context_line":"    def format_dom(self):"},{"line_number":2698,"context_line":"        root \u003d super(LibvirtConfigGuestFeatureSMM, self).format_dom()"}],"source_content_type":"text/x-python","patch_set":3,"id":"3af0009e_c2526e24","line":2695,"updated":"2022-01-28 16:21:10.000000000","message":"style nit: this can fit on one line","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"3c434e2baf8863f0f5c0b0693397fa989f682095","unresolved":false,"context_lines":[{"line_number":2692,"context_line":""},{"line_number":2693,"context_line":"    def __init__(self, **kwargs):"},{"line_number":2694,"context_line":"        super(LibvirtConfigGuestFeatureSMM, self).__init__(\"smm\","},{"line_number":2695,"context_line":"                                                                 **kwargs)"},{"line_number":2696,"context_line":""},{"line_number":2697,"context_line":"    def format_dom(self):"},{"line_number":2698,"context_line":"        root \u003d super(LibvirtConfigGuestFeatureSMM, self).format_dom()"}],"source_content_type":"text/x-python","patch_set":3,"id":"cb35562c_bd80dcc8","line":2695,"in_reply_to":"3af0009e_c2526e24","updated":"2022-01-28 17:46:43.000000000","message":"Done","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"257a7e5f47e39c366cec0ac86c7fa3d04697ad13","unresolved":true,"context_lines":[{"line_number":6310,"context_line":"                if \u0027requires-smm\u0027 in features:"},{"line_number":6311,"context_line":"                    guest.features.append("},{"line_number":6312,"context_line":"                        vconfig.LibvirtConfigGuestFeatureSMM())"},{"line_number":6313,"context_line":""},{"line_number":6314,"context_line":"            # NOTE(lyarwood): If the machine type isn\u0027t recorded in the stashed"},{"line_number":6315,"context_line":"            # image metadata then record it through the system metadata table."},{"line_number":6316,"context_line":"            # This will allow the host configuration to change in the future"}],"source_content_type":"text/x-python","patch_set":4,"id":"40294b88_a0431fda","line":6313,"updated":"2022-01-28 19:29:40.000000000","message":"i guess this is reasonable to do here although reqiures-smm is not a feature\n\ni would almost prefer do this check in the get_loaders function and retrun a bool.\nsince feature is not needed here in general.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1656da72d979e77307cc0681217e7db1b8732588","unresolved":true,"context_lines":[{"line_number":6310,"context_line":"                if \u0027requires-smm\u0027 in features:"},{"line_number":6311,"context_line":"                    guest.features.append("},{"line_number":6312,"context_line":"                        vconfig.LibvirtConfigGuestFeatureSMM())"},{"line_number":6313,"context_line":""},{"line_number":6314,"context_line":"            # NOTE(lyarwood): If the machine type isn\u0027t recorded in the stashed"},{"line_number":6315,"context_line":"            # image metadata then record it through the system metadata table."},{"line_number":6316,"context_line":"            # This will allow the host configuration to change in the future"}],"source_content_type":"text/x-python","patch_set":4,"id":"a0dbc098_cf3bf861","line":6313,"in_reply_to":"343a12f2_2dd7cc03","updated":"2022-01-30 21:59:52.000000000","message":"logically however a requirement is not a feature. ovmf seam to have misused this filed in the json for to uses. i would like to avoid propagating misuse into nova.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"e6ade17c8722d7b5fde19544a00c36fb44e5b031","unresolved":true,"context_lines":[{"line_number":6310,"context_line":"                if \u0027requires-smm\u0027 in features:"},{"line_number":6311,"context_line":"                    guest.features.append("},{"line_number":6312,"context_line":"                        vconfig.LibvirtConfigGuestFeatureSMM())"},{"line_number":6313,"context_line":""},{"line_number":6314,"context_line":"            # NOTE(lyarwood): If the machine type isn\u0027t recorded in the stashed"},{"line_number":6315,"context_line":"            # image metadata then record it through the system metadata table."},{"line_number":6316,"context_line":"            # This will allow the host configuration to change in the future"}],"source_content_type":"text/x-python","patch_set":4,"id":"343a12f2_2dd7cc03","line":6313,"in_reply_to":"40294b88_a0431fda","updated":"2022-01-28 20:03:02.000000000","message":"My logic: I come at this from a operators perspective, don\u0027t really understand all the inner workings but it \u0027felt\u0027 like a feature of the VM and is also in the \"features\" list of the OVMF json describing the firmware.\n\nNot bothered either way so I guess if yourselves want to come to a conclusion I can change it.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"21c69d4fb6b2d2d70a5df326fa900265eae896e5","unresolved":false,"context_lines":[{"line_number":6310,"context_line":"                if \u0027requires-smm\u0027 in features:"},{"line_number":6311,"context_line":"                    guest.features.append("},{"line_number":6312,"context_line":"                        vconfig.LibvirtConfigGuestFeatureSMM())"},{"line_number":6313,"context_line":""},{"line_number":6314,"context_line":"            # NOTE(lyarwood): If the machine type isn\u0027t recorded in the stashed"},{"line_number":6315,"context_line":"            # image metadata then record it through the system metadata table."},{"line_number":6316,"context_line":"            # This will allow the host configuration to change in the future"}],"source_content_type":"text/x-python","patch_set":4,"id":"e56ed2da_f7557cf9","line":6313,"in_reply_to":"a0dbc098_cf3bf861","updated":"2022-01-31 10:35:16.000000000","message":"Done","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":6962,"name":"Kashyap Chamarthy","email":"kchamart@redhat.com","username":"kashyapc"},"change_message_id":"ec911d72aabd15fc55dd486acf9efd9821e12c1e","unresolved":true,"context_lines":[{"line_number":6311,"context_line":"                if requires_smm:"},{"line_number":6312,"context_line":"                    guest.features.append("},{"line_number":6313,"context_line":"                        vconfig.LibvirtConfigGuestFeatureSMM())"},{"line_number":6314,"context_line":""},{"line_number":6315,"context_line":"            # NOTE(lyarwood): If the machine type isn\u0027t recorded in the stashed"},{"line_number":6316,"context_line":"            # image metadata then record it through the system metadata table."},{"line_number":6317,"context_line":"            # This will allow the host configuration to change in the future"}],"source_content_type":"text/x-python","patch_set":6,"id":"34ba14b5_7d5131bf","line":6314,"updated":"2022-01-31 10:58:11.000000000","message":"Makes sense to explicitly, since we\u0027re not auto-selecting the firmware.  And as you found out, we seem to generate incorrect XML.\n\nThanks for catching the bug and writing this patch.","commit_id":"d9f5258ce7050d991b586a94a4a403227dd66078"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"27a1d4872e6df2544bd51fbf274e74026af74def","unresolved":false,"context_lines":[{"line_number":6311,"context_line":"                if requires_smm:"},{"line_number":6312,"context_line":"                    guest.features.append("},{"line_number":6313,"context_line":"                        vconfig.LibvirtConfigGuestFeatureSMM())"},{"line_number":6314,"context_line":""},{"line_number":6315,"context_line":"            # NOTE(lyarwood): If the machine type isn\u0027t recorded in the stashed"},{"line_number":6316,"context_line":"            # image metadata then record it through the system metadata table."},{"line_number":6317,"context_line":"            # This will allow the host configuration to change in the future"}],"source_content_type":"text/x-python","patch_set":6,"id":"ed756cc6_b6fc8802","line":6314,"in_reply_to":"34ba14b5_7d5131bf","updated":"2022-01-31 19:28:13.000000000","message":"Ack","commit_id":"d9f5258ce7050d991b586a94a4a403227dd66078"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2ae6006220de99b8853a8852ce65d96fd59085b5","unresolved":true,"context_lines":[{"line_number":6292,"context_line":"                    guest.os_loader_secure \u003d False"},{"line_number":6293,"context_line":""},{"line_number":6294,"context_line":"                try:"},{"line_number":6295,"context_line":"                    loader, nvram_template, requires_smm \u003d \\"},{"line_number":6296,"context_line":"                    self._host.get_loader("},{"line_number":6297,"context_line":"                        arch, mach_type,"},{"line_number":6298,"context_line":"                        has_secure_boot\u003dguest.os_loader_secure)"},{"line_number":6299,"context_line":"                except exception.UEFINotSupported as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"85b47663_bbeaacef","line":6296,"range":{"start_line":6295,"start_character":59,"end_line":6296,"end_character":19},"updated":"2022-02-16 13:41:54.000000000","message":"nit: we generally prefer to use () not \\ when we need to use multiple lines.\n\nloader, nvram_template, requires_smm \u003d (\n    self._host.get_loader(...)\n)","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"d4d64157de14a64e4279884fd5a8015097d0dd28","unresolved":false,"context_lines":[{"line_number":6292,"context_line":"                    guest.os_loader_secure \u003d False"},{"line_number":6293,"context_line":""},{"line_number":6294,"context_line":"                try:"},{"line_number":6295,"context_line":"                    loader, nvram_template, requires_smm \u003d \\"},{"line_number":6296,"context_line":"                    self._host.get_loader("},{"line_number":6297,"context_line":"                        arch, mach_type,"},{"line_number":6298,"context_line":"                        has_secure_boot\u003dguest.os_loader_secure)"},{"line_number":6299,"context_line":"                except exception.UEFINotSupported as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"d6a810bf_fa30cc76","line":6296,"range":{"start_line":6295,"start_character":59,"end_line":6296,"end_character":19},"in_reply_to":"3b277e4b_b03e5f8e","updated":"2022-02-17 13:33:33.000000000","message":"I did `tox -e pep8` and it came back fine. Also reran the unit tests related to it as well.","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"18bcf5c871a0d146542f47d39b09aca9de1f97fb","unresolved":false,"context_lines":[{"line_number":6292,"context_line":"                    guest.os_loader_secure \u003d False"},{"line_number":6293,"context_line":""},{"line_number":6294,"context_line":"                try:"},{"line_number":6295,"context_line":"                    loader, nvram_template, requires_smm \u003d \\"},{"line_number":6296,"context_line":"                    self._host.get_loader("},{"line_number":6297,"context_line":"                        arch, mach_type,"},{"line_number":6298,"context_line":"                        has_secure_boot\u003dguest.os_loader_secure)"},{"line_number":6299,"context_line":"                except exception.UEFINotSupported as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"bc5fd78f_125e244b","line":6296,"range":{"start_line":6295,"start_character":59,"end_line":6296,"end_character":19},"in_reply_to":"85b47663_bbeaacef","updated":"2022-02-17 12:47:11.000000000","message":"Done","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d827a37338fea2f3a0d6d66529f7391f8ddd651f","unresolved":false,"context_lines":[{"line_number":6292,"context_line":"                    guest.os_loader_secure \u003d False"},{"line_number":6293,"context_line":""},{"line_number":6294,"context_line":"                try:"},{"line_number":6295,"context_line":"                    loader, nvram_template, requires_smm \u003d \\"},{"line_number":6296,"context_line":"                    self._host.get_loader("},{"line_number":6297,"context_line":"                        arch, mach_type,"},{"line_number":6298,"context_line":"                        has_secure_boot\u003dguest.os_loader_secure)"},{"line_number":6299,"context_line":"                except exception.UEFINotSupported as exc:"}],"source_content_type":"text/x-python","patch_set":10,"id":"3b277e4b_b03e5f8e","line":6296,"range":{"start_line":6295,"start_character":59,"end_line":6296,"end_character":19},"in_reply_to":"bc5fd78f_125e244b","updated":"2022-02-17 12:51:19.000000000","message":"not quite technially you shoudl indent self....\n\nbut assuming this passes pep8 i think its fine i just expect it to fail.","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"}],"nova/virt/libvirt/host.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"409605fc697393c1f5471650992fbeda5ec1dea3","unresolved":true,"context_lines":[{"line_number":1642,"context_line":"        arch: str,"},{"line_number":1643,"context_line":"        machine: str,"},{"line_number":1644,"context_line":"        has_secure_boot: bool,"},{"line_number":1645,"context_line":"    ):"},{"line_number":1646,"context_line":"        \"\"\"Get loader for the specified architecture and machine type."},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"        :returns: A the bootloader executable path and the NVRAM"}],"source_content_type":"text/x-python","patch_set":3,"id":"5c31ed62_1f5ba075","line":1645,"updated":"2022-01-28 16:21:10.000000000","message":"Don\u0027t remove the type hint: update it. What is \u0027features\u0027? I guess a list of strings? If so, say that:\n\n  ty.Tuple[str, str, ty.List[str]]\n\nmypy will tell you if you got it wrong since (iirc) libvirt has typing information now. You can run mypy via tox:\n\n  tox -e mypy","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"3c434e2baf8863f0f5c0b0693397fa989f682095","unresolved":false,"context_lines":[{"line_number":1642,"context_line":"        arch: str,"},{"line_number":1643,"context_line":"        machine: str,"},{"line_number":1644,"context_line":"        has_secure_boot: bool,"},{"line_number":1645,"context_line":"    ):"},{"line_number":1646,"context_line":"        \"\"\"Get loader for the specified architecture and machine type."},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"        :returns: A the bootloader executable path and the NVRAM"}],"source_content_type":"text/x-python","patch_set":3,"id":"6aae7d52_ca26ee47","line":1645,"in_reply_to":"5c31ed62_1f5ba075","updated":"2022-01-28 17:46:43.000000000","message":"Thanks, new to this, that\u0027s useful to know.\n\nI did mention in my original comment I couldn\u0027t work out what it was supposed to be.","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1c0a05fa9f7af1862d5a4bee345db823e9d38ed3","unresolved":false,"context_lines":[{"line_number":1642,"context_line":"        arch: str,"},{"line_number":1643,"context_line":"        machine: str,"},{"line_number":1644,"context_line":"        has_secure_boot: bool,"},{"line_number":1645,"context_line":"    ):"},{"line_number":1646,"context_line":"        \"\"\"Get loader for the specified architecture and machine type."},{"line_number":1647,"context_line":""},{"line_number":1648,"context_line":"        :returns: A the bootloader executable path and the NVRAM"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f2a31b5_9218d4af","line":1645,"in_reply_to":"6aae7d52_ca26ee47","updated":"2022-01-28 19:16:42.000000000","message":"Fair. Sorry if that came across a bit sharp. I just spent a lot of time on type hints so am somewhat protective of them 😊 Thanks for the update","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"409605fc697393c1f5471650992fbeda5ec1dea3","unresolved":true,"context_lines":[{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"            return loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027], \\"},{"line_number":1677,"context_line":"                loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027], \\"},{"line_number":1678,"context_line":"                loader[\u0027features\u0027]"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"        raise exception.UEFINotSupported()"}],"source_content_type":"text/x-python","patch_set":3,"id":"be412223_0f4f567e","line":1678,"updated":"2022-01-28 16:21:10.000000000","message":"style nit: Let\u0027s keep using brackets here\n\n  return (\n      loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],\n      loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027],\n      loader[\u0027features\u0027],\n  )","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"3c434e2baf8863f0f5c0b0693397fa989f682095","unresolved":false,"context_lines":[{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"            return loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027], \\"},{"line_number":1677,"context_line":"                loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027], \\"},{"line_number":1678,"context_line":"                loader[\u0027features\u0027]"},{"line_number":1679,"context_line":""},{"line_number":1680,"context_line":"        raise exception.UEFINotSupported()"}],"source_content_type":"text/x-python","patch_set":3,"id":"66097fbb_f6bde501","line":1678,"in_reply_to":"be412223_0f4f567e","updated":"2022-01-28 17:46:43.000000000","message":"Done","commit_id":"85146e728d325bef27436c12319c767bcc04f7d6"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"257a7e5f47e39c366cec0ac86c7fa3d04697ad13","unresolved":true,"context_lines":[{"line_number":1676,"context_line":"            return ("},{"line_number":1677,"context_line":"                loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],"},{"line_number":1678,"context_line":"                loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027],"},{"line_number":1679,"context_line":"                loader[\u0027features\u0027],"},{"line_number":1680,"context_line":"            )"},{"line_number":1681,"context_line":""},{"line_number":1682,"context_line":"        raise exception.UEFINotSupported()"}],"source_content_type":"text/x-python","patch_set":4,"id":"3cfab400_3012e310","line":1679,"range":{"start_line":1679,"start_character":15,"end_line":1679,"end_character":35},"updated":"2022-01-28 19:29:40.000000000","message":"can we make this a bool and do the requires-smm test here","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"21c69d4fb6b2d2d70a5df326fa900265eae896e5","unresolved":false,"context_lines":[{"line_number":1676,"context_line":"            return ("},{"line_number":1677,"context_line":"                loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],"},{"line_number":1678,"context_line":"                loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027],"},{"line_number":1679,"context_line":"                loader[\u0027features\u0027],"},{"line_number":1680,"context_line":"            )"},{"line_number":1681,"context_line":""},{"line_number":1682,"context_line":"        raise exception.UEFINotSupported()"}],"source_content_type":"text/x-python","patch_set":4,"id":"c0bdba98_4cbb81cd","line":1679,"range":{"start_line":1679,"start_character":15,"end_line":1679,"end_character":35},"in_reply_to":"00a3ddb1_eefd53c5","updated":"2022-01-31 10:35:16.000000000","message":"Done","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"e6ade17c8722d7b5fde19544a00c36fb44e5b031","unresolved":true,"context_lines":[{"line_number":1676,"context_line":"            return ("},{"line_number":1677,"context_line":"                loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],"},{"line_number":1678,"context_line":"                loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027],"},{"line_number":1679,"context_line":"                loader[\u0027features\u0027],"},{"line_number":1680,"context_line":"            )"},{"line_number":1681,"context_line":""},{"line_number":1682,"context_line":"        raise exception.UEFINotSupported()"}],"source_content_type":"text/x-python","patch_set":4,"id":"fffbcf2c_5568ed42","line":1679,"range":{"start_line":1679,"start_character":15,"end_line":1679,"end_character":35},"in_reply_to":"3cfab400_3012e310","updated":"2022-01-28 20:03:02.000000000","message":"My original train of thought was what will be the next thing that needs to be checked for from the \"features\" list so return it back to the driver that configures the guest rather than do it in the host bit.\n\nNot bothered either way so I guess if yourselves want to come to a conclusion I can change it.","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1656da72d979e77307cc0681217e7db1b8732588","unresolved":true,"context_lines":[{"line_number":1676,"context_line":"            return ("},{"line_number":1677,"context_line":"                loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],"},{"line_number":1678,"context_line":"                loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027],"},{"line_number":1679,"context_line":"                loader[\u0027features\u0027],"},{"line_number":1680,"context_line":"            )"},{"line_number":1681,"context_line":""},{"line_number":1682,"context_line":"        raise exception.UEFINotSupported()"}],"source_content_type":"text/x-python","patch_set":4,"id":"00a3ddb1_eefd53c5","line":1679,"range":{"start_line":1679,"start_character":15,"end_line":1679,"end_character":35},"in_reply_to":"fffbcf2c_5568ed42","updated":"2022-01-30 21:59:52.000000000","message":"the requirment need to be checked but the list of feature does not need to be passed back.\n\nso i woudl prefer you to return a requires_smm boolean to comunicate the requirement \n\nafter all we dont reqreuse all the feature that can be enabled we just need to know if SSM is required or not.\n\nso this should ideally be \u0027requires_smm\u0027 in loader[\u0027features\u0027]","commit_id":"a3cd57b0587884c3e21155b1f5593fb4541d3081"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f8e0cec1957586276aa35d010fd59e9c14b9def3","unresolved":true,"context_lines":[{"line_number":1637,"context_line":"        self._loaders \u003d _get_loaders()"},{"line_number":1638,"context_line":"        return self._loaders"},{"line_number":1639,"context_line":""},{"line_number":1640,"context_line":"    def get_loader("},{"line_number":1641,"context_line":"        self,"},{"line_number":1642,"context_line":"        arch: str,"},{"line_number":1643,"context_line":"        machine: str,"}],"source_content_type":"text/x-python","patch_set":7,"id":"29f1eb98_305e11f6","line":1640,"range":{"start_line":1640,"start_character":8,"end_line":1640,"end_character":18},"updated":"2022-01-31 12:54:02.000000000","message":"can you add or update the unit test coverage for get_loader so that you assert the change in behavior below.\nideally we shoudl have a testcase wehre requires-smm is true and false.","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"8984ddd383ee6ab0e00b89242c70bb890ad4568a","unresolved":false,"context_lines":[{"line_number":1637,"context_line":"        self._loaders \u003d _get_loaders()"},{"line_number":1638,"context_line":"        return self._loaders"},{"line_number":1639,"context_line":""},{"line_number":1640,"context_line":"    def get_loader("},{"line_number":1641,"context_line":"        self,"},{"line_number":1642,"context_line":"        arch: str,"},{"line_number":1643,"context_line":"        machine: str,"}],"source_content_type":"text/x-python","patch_set":7,"id":"6c5818d8_26264c96","line":1640,"range":{"start_line":1640,"start_character":8,"end_line":1640,"end_character":18},"in_reply_to":"29f1eb98_305e11f6","updated":"2022-01-31 15:11:47.000000000","message":"Extended the existing test.\n\n`$ tox -- HostTestCase.test_get_loader`","commit_id":"7e945996010d4ac22358be41abcf1d9ec4bdb787"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c08995938d47aaf2304b00bf503090bec65a6bfd","unresolved":true,"context_lines":[{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"            # by default assume we don\u0027t need SMM unless the json says we do"},{"line_number":1677,"context_line":"            if \u0027requires-smm\u0027 in loader[\u0027features\u0027]:"},{"line_number":1678,"context_line":"                requies_smm \u003d True"},{"line_number":1679,"context_line":"            else:"},{"line_number":1680,"context_line":"                requies_smm \u003d False"},{"line_number":1681,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"223ac970_9fd0e9a7","line":1678,"range":{"start_line":1678,"start_character":16,"end_line":1678,"end_character":27},"updated":"2022-02-17 11:57:22.000000000","message":"requires_smm (you\u0027re missing the second \u0027r\u0027)","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":34462,"name":"Imran Hussain","email":"ih@imranh.co.uk","username":"imranh2"},"change_message_id":"18bcf5c871a0d146542f47d39b09aca9de1f97fb","unresolved":false,"context_lines":[{"line_number":1675,"context_line":""},{"line_number":1676,"context_line":"            # by default assume we don\u0027t need SMM unless the json says we do"},{"line_number":1677,"context_line":"            if \u0027requires-smm\u0027 in loader[\u0027features\u0027]:"},{"line_number":1678,"context_line":"                requies_smm \u003d True"},{"line_number":1679,"context_line":"            else:"},{"line_number":1680,"context_line":"                requies_smm \u003d False"},{"line_number":1681,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"ab416d37_ae3f07dd","line":1678,"range":{"start_line":1678,"start_character":16,"end_line":1678,"end_character":27},"in_reply_to":"223ac970_9fd0e9a7","updated":"2022-02-17 12:47:11.000000000","message":"Done","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c08995938d47aaf2304b00bf503090bec65a6bfd","unresolved":false,"context_lines":[{"line_number":1677,"context_line":"            if \u0027requires-smm\u0027 in loader[\u0027features\u0027]:"},{"line_number":1678,"context_line":"                requies_smm \u003d True"},{"line_number":1679,"context_line":"            else:"},{"line_number":1680,"context_line":"                requies_smm \u003d False"},{"line_number":1681,"context_line":""},{"line_number":1682,"context_line":"            return ("},{"line_number":1683,"context_line":"                loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],"}],"source_content_type":"text/x-python","patch_set":10,"id":"f67c102f_6d81f1f4","line":1680,"updated":"2022-02-17 11:57:22.000000000","message":"nit:\n\n  requires_smm \u003d \u0027requires-smm\u0027 in loader[\u0027features\u0027]\n\nor, better, just return this below\n\n  return (\n      loader[\u0027mapping\u0027][\u0027executable\u0027][\u0027filename\u0027],\n      loader[\u0027mapping\u0027][\u0027nvram-template\u0027][\u0027filename\u0027],\n      \u0027requires-smm\u0027 in loader[\u0027features\u0027],\n  )","commit_id":"82595dbed83ff876ece0290ae39128bea2d7bb52"}]}