| Message ID | 20230811093614.28005-1-dwagner@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Switch to allowed_host | expand |
On 8/11/23 11:36, Daniel Wagner wrote: > Addressed the comments from v2. I also added cleanup code to _nvmet_cleanup() to > make sure we do not leak resources when something goes wrong. I run into this > while testing and all tests after the first failure failed then. > > changes: > v3: > - added new patch: "nvme/043: Use hostnqn to generate DHCAP key" > - removed unused variable in "nvme/rc: Add helper for adding/removing to allow list" > - added cleanup code to _nvmet_cleanup(). > > v2: > - updated commit messages > - moved the removal of subsys_name to the right patch > - added _nvmet_target_{setup|cleanup} helpers > this addresses also the 'appears unused' warning by ShellCheck > - https://lore.kernel.org/linux-nvme/20230810111317.25273-1-dwagner@suse.de/ > > v1: > - initial version > https://lore.kernel.org/linux-nvme/20230726124644.12619-1-dwagner@suse.de/ > Looks good. You can add: Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
> Addressed the comments from v2. I also added cleanup code to _nvmet_cleanup() to > make sure we do not leak resources when something goes wrong. I run into this > while testing and all tests after the first failure failed then. The name of the patch series suggest that it switches to allowed_hosts where it does that in 2 patches 11+12 out of 13 patches. The rest are just bug fixes and unifications. It's true that any series will include fixes, cleanups and prep patches, but this is too far :) I'll let Shinichiro accept as he wish though. The cleanups look fine to me.
On Sun, Aug 13, 2023 at 05:59:53PM +0300, Sagi Grimberg wrote: > > > Addressed the comments from v2. I also added cleanup code to _nvmet_cleanup() to > > make sure we do not leak resources when something goes wrong. I run into this > > while testing and all tests after the first failure failed then. > > The name of the patch series suggest that it switches to allowed_hosts > where it does that in 2 patches 11+12 out of 13 patches. The rest are > just bug fixes and unifications. It's true that any series will include > fixes, cleanups and prep patches, but this is too far :) I see your point. The whole series started smaller, but just grew over time. I suppose if we agree with the general direction we could just get the first part done (bug fixes and refactoring). > I'll let Shinichiro accept as he wish though. I am fine either way, just let me know what you prefer. > The cleanups look fine to me. Thanks for reviewing!
On Aug 16, 2023 / 11:31, Daniel Wagner wrote: > On Sun, Aug 13, 2023 at 05:59:53PM +0300, Sagi Grimberg wrote: > > > > > Addressed the comments from v2. I also added cleanup code to _nvmet_cleanup() to > > > make sure we do not leak resources when something goes wrong. I run into this > > > while testing and all tests after the first failure failed then. > > > > The name of the patch series suggest that it switches to allowed_hosts > > where it does that in 2 patches 11+12 out of 13 patches. The rest are > > just bug fixes and unifications. It's true that any series will include > > fixes, cleanups and prep patches, but this is too far :) > > I see your point. The whole series started smaller, but just grew over > time. I suppose if we agree with the general direction we could just get > the first part done (bug fixes and refactoring). > > > I'll let Shinichiro accept as he wish though. > > I am fine either way, just let me know what you prefer. I think the 13th patch worth spending some more time, and other 12 patches from 1 to 12 have consensus. If there is no other voice, I will apply the patches from 1 to 12 tomorrow.
On Aug 16, 2023 / 21:18, Shin'ichiro Kawasaki wrote: [...] > I think the 13th patch worth spending some more time, and other 12 patches > from 1 to 12 have consensus. If there is no other voice, I will apply the > patches from 1 to 12 tomorrow. I've applied the patches from 1 to 12. Of note is that I added "export" to the new, three def_* variables in nvme/rc to avoid shellcheck warnings. Anyway, thank you for the clean up and fixes!
On Thu, Aug 17, 2023 at 02:58:58AM +0000, Shinichiro Kawasaki wrote: > On Aug 16, 2023 / 21:18, Shin'ichiro Kawasaki wrote: > [...] > > I think the 13th patch worth spending some more time, and other 12 patches > > from 1 to 12 have consensus. If there is no other voice, I will apply the > > patches from 1 to 12 tomorrow. > > I've applied the patches from 1 to 12. Of note is that I added "export" to the > new, three def_* variables in nvme/rc to avoid shellcheck warnings. Thanks. This one of the reasons why I included patch #13, to use these newly introduced def_* variables. > Anyway, thank you for the clean up and fixes! You're welcome!
Addressed the comments from v2. I also added cleanup code to _nvmet_cleanup() to make sure we do not leak resources when something goes wrong. I run into this while testing and all tests after the first failure failed then. changes: v3: - added new patch: "nvme/043: Use hostnqn to generate DHCAP key" - removed unused variable in "nvme/rc: Add helper for adding/removing to allow list" - added cleanup code to _nvmet_cleanup(). v2: - updated commit messages - moved the removal of subsys_name to the right patch - added _nvmet_target_{setup|cleanup} helpers this addresses also the 'appears unused' warning by ShellCheck - https://lore.kernel.org/linux-nvme/20230810111317.25273-1-dwagner@suse.de/ v1: - initial version https://lore.kernel.org/linux-nvme/20230726124644.12619-1-dwagner@suse.de/ *** BLURB HERE *** Daniel Wagner (13): nvme/{003,004,005,013,046,049}: Group all variables declarations nvme: Reorganize test preamble code section nvme/043: Use hostnqn to generate DHCAP key nvme/rc: Add common subsystem nqn define nvme: Use def_subsysnqn variable instead local variable nvme/{041,042,043,044,045,048}: Remove local variable hostnqn and hostid nvme/rc: Add common file_path name define nvme: Use def_file_path variable instead local variable nvme/rc: Add common def_subsys_uuid define nvme: Use def_subsys_uuid variable nvme/rc: Add helper for adding/removing to allow list nvme: Add explicitly host to allow_host list nvme: Introduce nvmet_target_{setup/cleanup} common code tests/nvme/003 | 12 ++----- tests/nvme/004 | 23 ++++-------- tests/nvme/005 | 22 +++--------- tests/nvme/006 | 21 ++--------- tests/nvme/007 | 19 ++-------- tests/nvme/008 | 26 +++----------- tests/nvme/009 | 21 +++-------- tests/nvme/010 | 26 +++----------- tests/nvme/011 | 22 +++--------- tests/nvme/012 | 26 +++----------- tests/nvme/013 | 22 +++--------- tests/nvme/014 | 26 +++----------- tests/nvme/015 | 21 +++-------- tests/nvme/016 | 17 +++++---- tests/nvme/017 | 26 ++++++-------- tests/nvme/018 | 21 +++-------- tests/nvme/019 | 26 +++----------- tests/nvme/020 | 21 +++-------- tests/nvme/021 | 21 +++-------- tests/nvme/022 | 21 +++-------- tests/nvme/023 | 26 +++----------- tests/nvme/024 | 21 +++-------- tests/nvme/025 | 21 +++-------- tests/nvme/026 | 21 +++-------- tests/nvme/027 | 20 +++-------- tests/nvme/028 | 20 +++-------- tests/nvme/029 | 26 +++----------- tests/nvme/030 | 19 +++++----- tests/nvme/031 | 14 ++++---- tests/nvme/033 | 9 ++--- tests/nvme/034 | 9 ++--- tests/nvme/035 | 9 ++--- tests/nvme/036 | 9 ++--- tests/nvme/037 | 8 ++--- tests/nvme/038 | 6 ++-- tests/nvme/039 | 4 +-- tests/nvme/040 | 28 +++++---------- tests/nvme/041 | 49 ++++++++----------------- tests/nvme/042 | 55 ++++++++++------------------ tests/nvme/043 | 52 +++++++++------------------ tests/nvme/044 | 71 ++++++++++++++---------------------- tests/nvme/045 | 62 ++++++++++++-------------------- tests/nvme/046 | 1 + tests/nvme/047 | 30 ++++------------ tests/nvme/048 | 42 +++++++--------------- tests/nvme/049 | 1 + tests/nvme/rc | 97 +++++++++++++++++++++++++++++++++++++++++++++++--- 47 files changed, 404 insertions(+), 766 deletions(-)