Re: [PATCH 6.19/scsi-queue] scsi: target: core: Add emulation for REPORT_IDENTIFICATION_INFORMATION

[John Garry <john.g.garry@oracle.com>]

>>> +
>>> +   len = strscpy(buf, page, sizeof(buf));
>>> +   if (len > 0) {
>>
>> Can you just return early if len is too short?
> You mean to return before we check "if (len > 0)"?

I'm not sure. It's just that the checks seem odd to me. Firstly could 
you do a E2BIG check after strscpy (by checking len < 0)?

I think that logically the following is the same, but it is slightly 
neater. Please check it.

	if (strscpy(buf, page, sizeof(buf) < 0)
		return -EOVERFLOW;
	stripped = strstrip(buf);
	if (strlen(stripped) >= PD_TEXT_ID_INFO_LEN)
		return -EOVERFLOW;

> 
>>
>>> +           /* Strip any newline added from userspace. */
>>> +           stripped = strstrip(buf);
>>> +           len = strlen(stripped);
>>> +   }
>>> +
>>> +   if (len < 0 || len >= PD_TEXT_ID_INFO_LEN) {
>>> +           pr_err("Emulated peripheral device text id info exceeds"
>>> +                   " PD_TEXT_ID_INFO_LEN: "
>> __stringify(PD_TEXT_ID_INFO_LEN)
>>> +                   "\n");
>>
>> I think that this can all be a single line.
>>
> Actually here I followed the existing code style. Mike also wanted to restrict the columns per line to be 80.
> Let me know if you want me to change this.

The 80 char limit does not apply to strings, but you are using 
__stringify (so a grey area). Please disregard my comment if you like.

>>> +           return -EOVERFLOW;
>>> +   }
>>> +
>>> +   /*
>>> +    * Check to see if any active exports exist.  If they do exist, fail
>>> +    * here as changing this information on the fly (underneath the
>>> +    * initiator side OS dependent multipath code) could cause negative
>>> +    * effects.
>>> +    */
>>> +   if (dev->export_count) {

...

>>> +static sense_reason_t
>>> +spc_fill_pd_text_id_info(struct se_cmd *cmd, u8 *cdb) {
>>> +   struct se_device *dev = cmd->se_dev;
>>> +   unsigned char *buf;
>>> +   unsigned char *rbuf;
>>> +   u32 header_len = 4;
>>> +   u32 actual_data_len;
>>> +   u32 buf_len;
>>> +   u16 id_len;
>>> +
>>> +   buf_len = get_unaligned_be32(&cdb[6]);
>>> +   if (buf_len < header_len)
>>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>
>> isn't there a limit of 512 bytes for code 10b?
> Earlier I kept this as 512 bytes as the userspace is sending the size as 512, but Mike pointed out the range mentioned in the specification.
> So, If we see the Table 61 (Table 61 — Identifying information types) of "spc4r37", it says 0-256 bytes for "Peripheral device text identifying information".
> Let me know your comments.

ok, I was checking Peripheral device identifying information.

>>
>>   > +> +       id_len = strlen(dev->t10_wwn.pd_text_id_info);
>>> +   if (id_len > 0)
>>> +           /* trailing null */
>>> +           id_len += 1;
>>> +
>>> +   actual_data_len = id_len + header_len;
>>> +
>>> +   if (actual_data_len < buf_len)
>>> +           buf_len = actual_data_len;
>>> +
>>> +   buf = kzalloc(buf_len, GFP_KERNEL);
>>> +   if (!buf) {
>>> +           pr_err("Unable to allocate response buffer for IDENTITY
>> INFO\n");
>>> +           return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>> +   }
>>> +
>>> +   scnprintf(&buf[header_len], buf_len - header_len, "%s",
>>> +            dev->t10_wwn.pd_text_id_info);
>>> +
>>> +   put_unaligned_be16(id_len, &buf[2]);
>>> +
>>> +   rbuf = transport_kmap_data_sg(cmd);
>>
>> is it really ok if this returns NULL?
> I think its ok to return NULL.  We just don't send any information. I kept it like this because I don't see any information defined by protocol when the memory mapping fails.

If this fails, then we don't supply the pd_text_id_info but we are 
setting the len in the put_unaligned_be16(id_len, &buf[2]) call, which 
seems improper.

> Mike, Can you please correct me if I am missing anything?
>>
>>> +   if (rbuf) {
>>> +           memcpy(rbuf, buf, buf_len);
>>> +           transport_kunmap_data_sg(cmd);
>>> +   }
>>> +   kfree(buf);
>>> +
>>> +   target_complete_cmd_with_length(cmd, SAM_STAT_GOOD, buf_len);
>>> +   return TCM_NO_SENSE;
>>> +}
>>> +
>>> +static sense_reason_t
>>> +spc_emulate_report_id_info(struct se_cmd *cmd) {
>>> +   u8 *cdb = cmd->t_task_cdb;
>>> +   sense_reason_t rc;
>>> +
>>> +   switch ((cdb[10] >> 1)) {
>>> +   case 2:
>>> +           rc = spc_fill_pd_text_id_info(cmd, cdb);
>>> +           break;
>>> +   default:
>>> +           return TCM_UNSUPPORTED_SCSI_OPCODE;
>>> +   }
>>> +
>>> +   return rc;
>>
>> I don't know why default clause is required and not return result from
>> spc_fill_pd_text_id_info() directly?
> 
> Default case is to handle the incorrect information type. As of now we are using the 10b code but in future, we may need to handle others.

ok...