Re: [PATCH 03/11] nvmet-fcloop: refactor fcloop_nport_alloc

[Daniel Wagner <dwagner@suse.de>]

On Fri, Feb 28, 2025 at 08:11:11AM +0100, Hannes Reinecke wrote:
> > +	nport = fcloop_nport_lookup(opts->wwnn, opts->wwpn);
> > +	if (nport && ((remoteport && nport->rport) ||
> > +		      (!remoteport && nport->tport))) {
> > +		/* invalid configuration */
> > +		goto out_put_nport;
> > +	}
> > -	spin_lock_irqsave(&fcloop_lock, flags);
> > +	if (!nport) {
> > +		nport = kzalloc(sizeof(*nport), GFP_KERNEL);
> > +		if (!nport)
> > +			goto out_free_opts;
> > -	list_for_each_entry(tmplport, &fcloop_lports, lport_list) {
> > -		if (tmplport->localport->node_name == opts->wwnn &&
> > -		    tmplport->localport->port_name == opts->wwpn)
> > -			goto out_invalid_opts;
> > +		INIT_LIST_HEAD(&nport->nport_list);
> > +		nport->node_name = opts->wwnn;
> > +		nport->port_name = opts->wwpn;
> > +		kref_init(&nport->ref);
> > -		if (tmplport->localport->node_name == opts->lpwwnn &&
> > -		    tmplport->localport->port_name == opts->lpwwpn)
> > -			lport = tmplport;
> > +		spin_lock_irqsave(&fcloop_lock, flags);
> > +		list_add_tail(&nport->nport_list, &fcloop_nports);
> > +		spin_unlock_irqrestore(&fcloop_lock, flags);
> 
> Don't you need to check here if an 'nport' with the same node_name and
> port_name is already present?

There is the existing check which filters out some of the duplicates
(the check is there to allow setting up the target or the remote port
first, so the order doesn't matter), though I am not sure if it would
catch all duplicates. I don't mind adding this, but I'd say it would be
better in a separate patch. I tried to refactor this code without
changing anything else.