On Aug 21, 2024 / 14:58, Nilay Shroff wrote:
>
>
> On 8/21/24 12:51, Shinichiro Kawasaki wrote:
[...]
> > diff --git a/tests/nvme/052 b/tests/nvme/052
> > index cf6061a..22e0bf5 100755
> > --- a/tests/nvme/052
> > +++ b/tests/nvme/052
> > @@ -20,23 +20,35 @@ set_conditions() {
> > _set_nvme_trtype "$@"
> > }
> >
> > +find_nvme_ns() {
> > + if [[ "$2" == removed ]]; then
> > + _find_nvme_ns "$1" 2> /dev/null
> > + else
> > + _find_nvme_ns "$1"
> > + fi
> > +}
> > +
> > +# Wait for the namespace with specified uuid to fulfill the specified condtion,
> > +# "created" or "removed".
> > nvmf_wait_for_ns() {
> > local ns
> > local timeout="5"
> > local uuid="$1"
> > + local condition="$2"
> >
> > - ns=$(_find_nvme_ns "${uuid}")
> > + ns=$(find_nvme_ns "${uuid}" "${condition}")
> >
> > start_time=$(date +%s)
> > - while [[ -z "$ns" ]]; do
> > + while [[ -z "$ns" && "$condition" == created ]] ||
> > + [[ -n "$ns" && "$condition" == removed ]]; do
> > sleep 1
> > end_time=$(date +%s)
> > if (( end_time - start_time > timeout )); then
> > echo "namespace with uuid \"${uuid}\" not " \
> > - "found within ${timeout} seconds"
> > + "${condition} within ${timeout} seconds"
> > return 1
> > fi
> > - ns=$(_find_nvme_ns "${uuid}")
> > + ns=$(find_nvme_ns "${uuid}" "${condition}")
> > done
> >
> > return 0
> > @@ -63,7 +75,7 @@ test() {
> > _create_nvmet_ns "${def_subsysnqn}" "${i}" "$(_nvme_def_file_path).$i" "${uuid}"
> >
> > # wait until async request is processed and ns is created
> > - nvmf_wait_for_ns "${uuid}"
> > + nvmf_wait_for_ns "${uuid}" created
> > if [ $? -eq 1 ]; then
> > echo "FAIL"
> > rm "$(_nvme_def_file_path).$i"
> > @@ -71,6 +83,10 @@ test() {
> > fi
> >
> > _remove_nvmet_ns "${def_subsysnqn}" "${i}"
> > +
> > + # wait until async request is processed and ns is removed
> > + nvmf_wait_for_ns "${uuid}" removed
> > +
> As nvme_wait_for_ns() returns either 0 (success) or 1 (failure), I think we
> should check the return status of nvme_wait_for_ns() here and bail out in case
> it returns failure same as what we do above while creating namespace.
Agreed, I will add the check to v2.
>
> Another point: I think, we may always suppress error from _find_nvme_ns() irrespective
> of it's being called while "creating" or "removing" the namespace assuming we always
> check the return status of nvme_wait_for_ns() in the main loop. So ideally we shall
> invoke _find_nvme_ns() from nvme_wait_for_ns() as below:
>
> ns=$(_find_nvme_ns "${uuid}" 2>/dev/null)
It doesn't sound correct to suppress the _find_nvme_ns errors always. We found
the issue since _find_nvme_ns reported the error at after _create_nvmet_ns()
call. If we suppress it, I can not be confident that this fix avoids the error.
>
> On a cosmetic note: Maybe we can use readonly constants to make "created" and "removed"
> parameters looks more elegant/readable.
>
> # Define constants
> readonly NS_ADD="added"
> readonly NS_DEL="deleted"
>
> Now we may reuse above constants instead of "created" and "removed". You may rename
> constant name if you don't like the name I used above :)
This sounds too much for me. "created" and "removed" are constant strings, so
I'm not sure about the merit of introducing the constant variables. Number of
characters will not change much either.
>
> Thanks,
> --Nilay
