[dm-devel] [PATCH] This patch filled the host WWNN. when using the command of multipathd to list the isscsi paths informartion, the host WWNN of paths informartion is not exist. like this: multipathd show paths format "%N %R" host WWNN host WWPN [undef] iqn.20

[Martin Wilck]

Hi Sunao,

Thanks for your patch. Next time, please keep the subject line
at a readable length, and provide a full description of the problem,
and the proposed solution, in the message body.

On Tue, 2018-09-18 at 09:09 +0000, Sunao (A) wrote:
> this patch solved this problem and filled the host WWNN.
> like this:
> multipathd show paths format "%N %R"
> host WWNN                           host WWPN
> iqn.1994-05.com.redhat:86329aefccdb iqn.2006-
> 08.com.huawei:oceanstor:21000022a10beb2a::1020002:129.13.100.240
> iqn.1994-05.com.redhat:86329aefccdb iqn.2006-
> 08.com.huawei:oceanstor:21000022a10beb2a::20003:129.13.100.241
> ---
>  libmultipath/print.c | 52 +++++++++++++++++++++++++++++++++++++++---
> ----------
>  1 file changed, 39 insertions(+), 13 deletions(-)

Unfortunately, your patch is malformed and thus doesn't apply. Please
try sending your patch directly with git-send-email or another
program that doesn't break the patch format.

> diff --git a/libmultipath/print.c b/libmultipath/print.c index
> 9da6a77c..e2b90301 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -539,20 +539,39 @@ snprint_host_attr (char * buff, size_t len,
> const struct path * pp, char *attr)
>  	char host_id[32];
>  	const char *value = NULL;
>  	int ret;
> -
> -	if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP)
> +    
> +	if ((pp->sg_id.proto_id != SCSI_PROTOCOL_FCP) && (pp-
> >sg_id.proto_id 
> +!= SCSI_PROTOCOL_ISCSI) )
>  		return snprintf(buff, len, "[undef]");
> -	sprintf(host_id, "host%d", pp->sg_id.host_no);
> -	host_dev = udev_device_new_from_subsystem_sysname(udev,
> "fc_host",
> -							  host_id);
> -	if (!host_dev) {
> -		condlog(1, "%s: No fc_host device for '%s'", pp->dev,
> host_id);
> -		goto out;
> +	if((pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI) && strcmp(buff,
> wwid) 
> +== 0)
> +		
> +    if(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP)
> +    {
> +		sprintf(host_id, "host%d", pp->sg_id.host_no);
> +		host_dev = udev_device_new_from_subsystem_sysname(udev,
> "fc_host",
> +								  host_
> id);
> +		if (!host_dev) {
> +			condlog(1, "%s: No fc_host device for '%s'",
> pp->dev, host_id);
> +			goto out;
> +		}
> +		value = udev_device_get_sysattr_value(host_dev, attr);
> +		if (value)
> +			ret = snprint_str(buff, len, value);
> +		udev_device_unref(host_dev);
> +    }
> +	else if(pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI)
> +	{
> +	    sprintf(host_id, "session%d", pp->sg_id.transport_id);
> +		host_dev = udev_device_new_from_subsystem_sysname(udev,
> "iscsi_session",
> +								  host_
> id);
> +		if (!host_dev) {
> +			condlog(3, "%s: No iscsi_session for '%s'", pp-
> >dev, host_id);
> +			goto out;
> +		}
> +		value = udev_device_get_sysattr_value(host_dev, attr);
> +		if (value)
> +			ret = snprint_str(buff, len, value);
> +		udev_device_unref(host_dev);
>  	}
> -	value = udev_device_get_sysattr_value(host_dev, attr);
> -	if (value)
> -		ret = snprint_str(buff, len, value);
> -	udev_device_unref(host_dev);
>  out:
>  	if (!value)
>  		ret = snprintf(buff, len, "[unknown]"); @@ -562,7
> +581,14 @@ out:
>  int
>  snprint_host_wwnn (char * buff, size_t len, const struct path *
> pp)  {
> -	return snprint_host_attr(buff, len, pp, "node_name");
> +	if(pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI)
> +	{
> +	    return snprint_host_attr(buff, len, pp, "initiatorname");
> +	}
> +	else
> +	{
> +		return snprint_host_attr(buff, len, pp, "node_name");
> +	}
>  }
>  
> could you agree with this solution or have a better solution?

The solution itself looks ok, but I think readability would be better
to with a separate function snprint_iscsi_host_wwnn(). Note that your
patch checks pp->sg_id.proto_id both in snprint_host_attr() and in its
caller, and that the only attribute you retrieve for iscsi is
"initiatorname", so you really need not put this functionality into
snprint_host_attr (which should rather be named snprint_fc_host_attr(),
but that's not your issue).

Note that we also have sysfs_get_tgt_nodename() in discovery.c which
has a lot of similar code. IMO, the Right Thing to do would be to
create a proper abstraction to retrieve the equivalents of "node name"
and "port name" for different SCSI transports. But that could be done
later.

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)