Hello Nab,
* Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx> [2013-09-16 22:29]:
I'm open to accepting a patch for this.. However, I'd prefer to keep
the default action of being able to perform sendtargets discovery of
all TargetNames, regardless of these changes.
done. Please let me know if you like the attribute name
hide_from_unauthorized or if I should change it?
So that said, I'm thinking the patch should include a new TPG
attribute that allows the endpoint to be hidden from sendtargets
discovery unless a valid NodeACL exists for the connected
InitiatorName. This TPG attribute will be disabled by default, and
can be enabled by admin on a endpoint by endpoint basis.
done.
If enabled + generate_node_acls=0 (eg: demo mode dislabed), the
discovery logic should walk through the list of NodeACLs for a given
TargetName+TargetPortalGroupTag endpoint, looking for match. If a
match is found then TargetName + Portals will be returned.
done.
FYI, iscsit_build_sendtargets_response() is already a bit convoluted
as is, so I'll expect a patch to add this type of functionality to
pretty up the existing code as well.
I thought about this several hours, but need advice from you. I thought
about cleaning up this function the following ways but discarded it for
the reasons below:
- Split up the scenarios:
- Normal
- IFC_SENDTARGETS_SINGLE
- hide_from_unauthorized
I discarded this option because it results in a lot of
duplicated code.
- Make the function shorter by:
- Putting if (cmd->cmd_flags & IFC_SENDTARGETS_SINGLE) {
text_ptr = strchr(text_in, '=');
...
}
in a seperate function. And put the sprintf ...
payload_len += len; parts in a seperate function.
Make the function shorter but not much nicer.
- Now that I put my thoughts in writing I should have probably
done both.
Please give me advice on howto pretty up the existing code.
That being said, I have a few pending questions:
spin_lock(&tpg->tpg_state_lock);
if ((tpg->tpg_state == TPG_STATE_FREE) ||
(tpg->tpg_state == TPG_STATE_INACTIVE)) {
spin_unlock(&tpg->tpg_state_lock);
continue;
}
spin_unlock(&tpg->tpg_state_lock);
Is the spinlock necessary? I think it is not because the tpg_state is an
enum that is mapped to a byte, int or long int. The access to it is
atomic. So I don't think we need it.
if (! target_name_printed) {
len = sprintf(buf, "TargetName=%s",
tiqn->tiqn);
len += 1;
if ((len + payload_len) > buffer_len) {
spin_unlock(&tpg->tpg_np_lock);
spin_unlock(&tiqn->tiqn_tpg_lock);
end_of_buf = 1;
goto eob;
}
memcpy(payload + payload_len, buf, len);
payload_len += len;
target_name_printed = 1;
}
len = sprintf(buf, "TargetAddress="
"%s:%hu,%hu",
(inaddr_any == false) ?
np->np_ip : conn->local_ip,
(inaddr_any == false) ?
np->np_port : conn->local_port,
tpg->tpgt);
len += 1;
if ((len + payload_len) > buffer_len) {
spin_unlock(&tpg->tpg_np_lock);
spin_unlock(&tiqn->tiqn_tpg_lock);
end_of_buf = 1;
goto eob;
}
memcpy(payload + payload_len, buf, len);
payload_len += len;
With the current code path it is possible that TargetName is printed but
TargetAddress is not because the payload buffer runs out of space. Is
this okay, or should we change it?
I often have a configuration where I export multiple demo mode LUNs
in different VLANs but I only want initiators from same VLAN access that
LUN. To my knowledge that is currently not possible, is it? Would you
accept another patch which only exposes demo mode LUNs if a portal
exists for that LUN for the same ip address as the discovery is
targeted or another patch which gives me same functionality but maybe
does it another way?
Cheers,
Thomas
