| Message ID | 20230830092019.9846-1-dwagner@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce nvmet target setup/cleanup helpers | expand |
On Aug 30, 2023 / 11:20, Daniel Wagner wrote: > Almost all fabric tests have the identically code for > setting up and cleaning up the target side. Introduce > two new helpers. > > Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> > Signed-off-by: Daniel Wagner <dwagner@suse.de> Thanks Daniel for this v4 series. All patches looks good, except one thing in this patch. [...] > diff --git a/tests/nvme/018 b/tests/nvme/018 > index 68729c3cb070..19e439f3f3e0 100755 > --- a/tests/nvme/018 > +++ b/tests/nvme/018 > @@ -21,16 +21,9 @@ test() { > > _setup_nvmet > > - local port > local nvmedev > > - truncate -s "${nvme_img_size}" "${def_file_path}" > - > - _create_nvmet_subsystem "${def_subsysnqn}" "${def_file_path}" \ > - "${def_subsys_uuid}" > - port="$(_create_nvmet_port "${nvme_trtype}")" > - _add_nvmet_subsys_to_port "${port}" "${def_subsysnqn}" > - _create_nvmet_host "${def_subsysnqn}" "${def_hostnqn}" > + _nvmet_target_setup As I noted for v3, I think the line above should be, _nvmet_target_setup --blkdev file If the change is ok for you, I'll add this fix up and apply the series. > > _nvme_connect_subsys "${nvme_trtype}" "${def_subsysnqn}" > > @@ -48,12 +41,7 @@ test() { > > _nvme_disconnect_subsys "${def_subsysnqn}" > > - _remove_nvmet_subsystem_from_port "${port}" "${def_subsysnqn}" > - _remove_nvmet_subsystem "${def_subsysnqn}" > - _remove_nvmet_port "${port}" > - _remove_nvmet_host "${def_hostnqn}" > - > - rm "${def_file_path}" > + _nvmet_target_cleanup > > echo "Test complete" > }
On Wed, Aug 30, 2023 at 01:38:03PM +0000, Shinichiro Kawasaki wrote: > > diff --git a/tests/nvme/018 b/tests/nvme/018 > > index 68729c3cb070..19e439f3f3e0 100755 > > --- a/tests/nvme/018 > > +++ b/tests/nvme/018 > > @@ -21,16 +21,9 @@ test() { > > > > _setup_nvmet > > > > - local port > > local nvmedev > > > > - truncate -s "${nvme_img_size}" "${def_file_path}" > > - > > - _create_nvmet_subsystem "${def_subsysnqn}" "${def_file_path}" \ > > - "${def_subsys_uuid}" > > - port="$(_create_nvmet_port "${nvme_trtype}")" > > - _add_nvmet_subsys_to_port "${port}" "${def_subsysnqn}" > > - _create_nvmet_host "${def_subsysnqn}" "${def_hostnqn}" > > + _nvmet_target_setup > > As I noted for v3, I think the line above should be, > > _nvmet_target_setup --blkdev file > > If the change is ok for you, I'll add this fix up and apply the > series. Sorry, I didn't see it in the response. Sure thing, no objection from my side.
On Aug 30, 2023 / 11:20, Daniel Wagner wrote: > Updated the series according the last round of feedback and retested it. In > order to pass the 'make check' check this version depends on the upcoming revert > of 26664dff17b6 ("Do not suppress any shellcheck warnings") as discussed in the > v3 thread. > > original cover letter: > > Introduce helpers to setup nvmet targets. This is spin off from the refactoring > patches and the allowed_host patches [1]. > > Sagi suggested to record all resources allocated by nvmet_target_setup and then > later clean them up in nvmet_target_cleanup. I opted to figure out in > nvmet_target_cleanup what was allocated via the newly introdcuded _get_nvmet_ports > helper. The reason being, Hannes told me offline that he would like to add ANA > tests which will add some more ports to the subsystem. I hope with this > the code is more future proof. > > BTW, while looking at this I saw that the passthru code is using the awkward > return value port when calling nvmet_passthru_target_setup. It seems some > more refactoring is in order... > > [1] https://lore.kernel.org/linux-nvme/5h333eqhtw252sjw6axjewlb5bbb5ze7awekczxe3kie2lnhw6@manyer42khct/ I've applied the series with the fix noted for the 3rd patch. I also reverted the commit 26664dff17b6 together. Thanks again for this clean up :)
>> [1] https://lore.kernel.org/linux-nvme/5h333eqhtw252sjw6axjewlb5bbb5ze7awekczxe3kie2lnhw6@manyer42khct/ > > I've applied the series with the fix noted for the 3rd patch. I also reverted > the commit 26664dff17b6 together. Thanks again for this clean up :) Seriously thanks again, it was due long time ago. -ck
Updated the series according the last round of feedback and retested it. In order to pass the 'make check' check this version depends on the upcoming revert of 26664dff17b6 ("Do not suppress any shellcheck warnings") as discussed in the v3 thread. original cover letter: Introduce helpers to setup nvmet targets. This is spin off from the refactoring patches and the allowed_host patches [1]. Sagi suggested to record all resources allocated by nvmet_target_setup and then later clean them up in nvmet_target_cleanup. I opted to figure out in nvmet_target_cleanup what was allocated via the newly introdcuded _get_nvmet_ports helper. The reason being, Hannes told me offline that he would like to add ANA tests which will add some more ports to the subsystem. I hope with this the code is more future proof. BTW, while looking at this I saw that the passthru code is using the awkward return value port when calling nvmet_passthru_target_setup. It seems some more refactoring is in order... [1] https://lore.kernel.org/linux-nvme/5h333eqhtw252sjw6axjewlb5bbb5ze7awekczxe3kie2lnhw6@manyer42khct/ changes v4: - introduced _cleanup_blkdev helper - fixed --blkdev arguments passing - added unknown argument warning to _nvmet_target_setup argument parser - added rb tag v3: - rebased/retested - use the default with _nvmet_target_setup - https://lore.kernel.org/linux-nvme/20230822083812.24612-1-dwagner@suse.de/ v2: - drop local subsys variable in passthru tests - do not use port as handle in passthru tests - free port after unregistering from subsys - https://lore.kernel.org/linux-nvme/20230818141537.22332-1-dwagner@suse.de/ v1: - https://lore.kernel.org/linux-nvme/20230818095744.24619-1-dwagner@suse.de/ Daniel Wagner (3): nvme/{033,034,035,036}: use default subsysnqn variable directly nvme/{033,034,035,036,37}: drop port handle between passthru target setup and cleanup nvme: introduce nvmet_target_{setup/cleanup} common code tests/nvme/003 | 14 ++----- tests/nvme/004 | 21 ++-------- tests/nvme/005 | 20 +--------- tests/nvme/006 | 19 +-------- tests/nvme/007 | 14 +------ tests/nvme/008 | 21 +--------- tests/nvme/009 | 16 +------- tests/nvme/010 | 21 +--------- tests/nvme/011 | 16 +------- tests/nvme/012 | 21 +--------- tests/nvme/013 | 16 +------- tests/nvme/014 | 21 +--------- tests/nvme/015 | 16 +------- tests/nvme/018 | 16 +------- tests/nvme/019 | 21 +--------- tests/nvme/020 | 16 +------- tests/nvme/021 | 16 +------- tests/nvme/022 | 16 +------- tests/nvme/023 | 21 +--------- tests/nvme/024 | 16 +------- tests/nvme/025 | 16 +------- tests/nvme/026 | 16 +------- tests/nvme/027 | 17 ++------ tests/nvme/028 | 17 ++------ tests/nvme/029 | 21 +--------- tests/nvme/033 | 10 ++--- tests/nvme/034 | 10 ++--- tests/nvme/035 | 10 ++--- tests/nvme/036 | 12 +++--- tests/nvme/037 | 5 +-- tests/nvme/040 | 19 +-------- tests/nvme/041 | 18 +-------- tests/nvme/042 | 17 +------- tests/nvme/043 | 17 +------- tests/nvme/044 | 19 ++------- tests/nvme/045 | 18 ++------- tests/nvme/047 | 21 +--------- tests/nvme/048 | 17 +------- tests/nvme/rc | 103 ++++++++++++++++++++++++++++++++++++++++++++++--- 39 files changed, 188 insertions(+), 553 deletions(-)