Hi Mike,
>>>> Add acls/{ACL}/attrib/authentication attribute that controls authentication
>>>> for the particular ACL. By default, this attribute inherits a value of
>>>> authentication attribute of the target port group to keep backward
>>>> compatibility.
>>>>
>>>> authentication attribute has 3 states:
>>>> "0" - authentication is turned off for this ACL
>>>> "1" - authentication is required for this ACL
>>>> "" - authentication is inherited from TPG
>>>
>>>
>>> Why the empty string for this value? Maybe 2 or -1?
>> That was design decision by logic that since that attribute has a precedence
>> to clear that precedence we must clear the attribute, i.e. set to the empty value.
>>
>>>>
>>>> Reviewed-by: Roman Bolshakov <r.bolshakov@xxxxxxxxx>
>>>> Reviewed-by: Konstantin Shelekhin <k.shelekhin@xxxxxxxxx>
>>>> Signed-off-by: Dmitry Bogdanov <d.bogdanov@xxxxxxxxx>
>>>> ---
>>>> drivers/target/iscsi/iscsi_target_configfs.c | 41 +++++++++++++++++++
>>>> drivers/target/iscsi/iscsi_target_nego.c | 8 +++-
>>>> .../target/iscsi/iscsi_target_nodeattrib.c | 1 +
>>>> include/target/iscsi/iscsi_target_core.h | 2 +
>>>> 4 files changed, 51 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
>>>> index e3750b64cc0c..2d70de342408 100644
>>>> --- a/drivers/target/iscsi/iscsi_target_configfs.c
>>>> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
>>>> @@ -314,6 +314,46 @@ ISCSI_NACL_ATTR(random_datain_pdu_offsets);
>>>> ISCSI_NACL_ATTR(random_datain_seq_offsets);
>>>> ISCSI_NACL_ATTR(random_r2t_offsets);
>>>>
>>>> +static ssize_t iscsi_nacl_attrib_authentication_show(struct config_item *item,
>>>> + char *page)
>>>> +{
>>>> + struct se_node_acl *se_nacl = attrib_to_nacl(item);
>>>> + struct iscsi_node_acl *nacl = to_iscsi_nacl(se_nacl);
>>>> +
>>>> + if (nacl->node_attrib.authentication == NA_AUTHENTICATION_INHERITED) {
>>>> + struct iscsi_portal_group *tpg = to_iscsi_tpg(se_nacl->se_tpg);
>>>> +
>>>> + return sprintf(page, "%u (inherited)\n",
>>>> + tpg->tpg_attrib.authentication);
>>>
>>>
>>> I think we want a value of -1 or 2 for inherited then here it should print
>>> that value.
>>
>> We decided to hide the internal value from userspace and represent it similar to
>> tpg.authentication to have the same handling there.
>
> I'm not sure what you meant by representing it similar to tpg.authentication. That
> attrib, and I think every attrib, prints 1 value.
>
> The problem with above is that this works by accident for rtslib based apps which
> read in the attribs, stores them, then on restore writes them to the kernel. On the
> read/save stage we get "0 (inherited)". Then on the restore stage we try to write
> that back to the kernel and get an error. rtslib/targetcli just spits out an error
> and ignores it, so it still works since the kernel used the default. We don't
> really want the error spit out and I don't think we want it to work by accident like
> this.
I missed that fact that rtslib saves/restores all attributes. You are right then, I need to
report some effective value (I think '-1'). Will send new patchset soon.
BR,
Dmitry
