On 07/19/2018 10:37 AM, Bart Van Assche wrote: > On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: >> + if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) { >> + len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n", >> + se_sess->se_node_acl->initiatorname, >> + &se_sess->sess_bin_isid); >> + } else { >> + len = snprintf(page, PAGE_SIZE, "%s\n", >> + se_sess->se_node_acl->initiatorname); >> + } > > Hello Mike, > > The general recommendation for configfs is that each attribute contains a > single value, just like for sysfs. Patch 11/15 exports two values through > a single attribute. Have you considered to split the above into two What about just making it the initiator port transport id so it aligns with the get_initiator_port_transport_id() comment for the other patch. For iscsi it would be 1 value with the format from SPC/SAM "target_name,i,0x,isid"? > attributes, namely the initiator name and the ISID? Can the initiator name > be changed into a soft link to the se_node_acl configfs directory to make > it easy for shell scripts to retrieve additional initiator configuration > information? Because the kernel is creating the session instead of it being driven from a mkdir, there are no existing functions for this. I do not know configfs code well, but I think making a function to do this is possible though. What about the dynamic_acl case? Just make those a normal file? Just to make sure we are on the same page too. The initiator name is always the name of the acl right? It looked like that from target_fabric_make_nodeacl but I was wondering if you are asking for the symlink because there are some fabric module quirks where that is not the case. If it's the same names, then you know the acl already from the initiator name file. > > Thanks, > > Bart. >