On Thu, 2013-10-03 at 22:54 +0200, Thomas Glanzmann wrote:
> 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?
How about renaming the new attribute to demo_mode_discovery, and using a
default value to 1 + inverting the current checks..?
This would be slightly more descriptive (eg: includes 'demo_mode' +
'discovery'), and does not include 'unauthorized', which conveys that
discovery CHAP authentication is somehow involved. ;)
>
> > 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.
>
Mmm, all fair points above. Thinking about this some more, I'm fine
with taking these patches without further cleanups here, and leave this
as a separate item for now.
> 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.
tpg->tpg_state is protected every place else by tpg_state_lock. Your
right that it's probably overkill for this particular case given the
purely information nature of SendTargets.
>
> > 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?
>
It's possible this could happen, yes. However the initiator receiving a
TargetName without any TargetAddresses should simply ignore this entry.
> 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?
Mmmm.. For LUNs on the same TargetName+TargetPortalGroupTag endpoint, I
don't believe this is possible.
However, splitting up these LUNs across different TargetName
+TargetPortalGroupTag endpoints, and only associating the individual
VLAN interfaces as network portals on these endpoints should provide the
desired effect, no..?
> 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?
>
I'd typically like to avoid doing LUN (or any access control) based upon
IP addresses. But if the same effect really can't be achieved by some
other means, I'm always open to at least a discussion.
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
