[PATCH blktests v2 04/13] nvme: introduce wwnn/wwpn helper functions

[Hannes Reinecke]

On 10/30/24 13:20, Shinichiro Kawasaki wrote:
> On Oct 30, 2024 / 08:21, Hannes Reinecke wrote:
>> On 10/30/24 07:51, Shin'ichiro Kawasaki wrote:
>>> Commit "nvme: sanitize transport parameter in _create_nvmet_port()"
>>> modified the wwnn and wwpn names for fc transport to calculate from the
>>> port ID. It also moved the timing to set the global variable
>>> def_host_traddr based on the wwnn and wwpn from _setup_nvmet() to
>>> _create_nvmet_port(). This move made _nvme_connect_subsys() fail
>>> to refer to the def_host_traddr variable, and caused nvme/003 test case
>>> failure.
>>>
>>> To avoid the failure, do the wwnn and wwpn calculations both in
>>> _create_nvmet_port() and _nvme_connect_subsys(). Introduce helper
>>> functions to calculate wwnn and wwpn.
>>>
>>> Reviewed-by: Daniel Wagner <dwagner at suse.de>
>>> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki at wdc.com>
>>> ---
>>>    common/nvme | 63 +++++++++++++++++++++++++++++++++++++----------------
>>>    1 file changed, 44 insertions(+), 19 deletions(-)
>>>
>> Hmm. This was not what my patchset did.
>>
>> In my patchset I changed the 'logical' FC port setup in using _always_
>> the same local wwpn/wwnn, and only have the remote wwpn/wwnn modified with
>> the port id.
>> That way the host_traddr can be kept global throughout the code, and
>> we can calculate the remote wwpn/wwnn when creating the FC ports.
> 
> Hannes, I think you mean the patch titled "nvmet: rework fcloop" [1] in the
> comment above. The patch is not in this series. I once tried to include it in
> this series and moved its commit position, but it caused rather heavy conflicts.
> So I'm preparing to include the patch in one of the following series. I hope it
> is okay for you to apply this series as it is, and later on we revisit the
> patch "nvmet: rework fcloop" in the following series.
> 
> In the following series, conflicts of the patch are easier for me to resolve.
> The patch I modified looks like this [2]. Your original patch [1] gets
> remote_wwnn/wwpn for port removal from addr_traddr, but after my modification,
> it gets remote_wwnn/wwpn from the port itself, because I think its simpler.
> 
> [1] https://github.com/osandov/blktests/pull/146/commits/5545881f11769832100a517d29ff059435a1fbbf
> [2] https://github.com/kawasaki/blktests/commit/93227f4c41f52032da3de1be1c3b485e639d9663
> 
Ah, ok, that's fine then.

Reviewed-by: Hannes Reinecke <hare at suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare at suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich