On 05/18/2018 06:27 PM, Lee Duncan wrote:
>
> On 05/18/2018 02:53 AM, Bodo Stroesser wrote:
> > @@ -621,6 +622,7 @@ static void gather_data_area(struct tcmu
> > }
> > copy_bytes = min_t(size_t, sg_remaining,
> > block_remaining);
> > + copy_bytes = min_t(size_t, copy_bytes, len_remaining);
>
> It may just be me, but I don't like this consecutive use of min_t() to
> set copy_bytes. I'd prefer adding something like:
>
> if (len_remaining < copy_bytes)
> copy_bytes = len_remaining;
>
> but maybe I'm old-fashioned.
I trusted in the compiler to remove the useless part of the code ...
But you are right, I'll change it.
> > @@ -947,6 +953,8 @@ static void tcmu_handle_completion(struc
> > {
> > struct se_cmd *se_cmd = cmd->se_cmd;
> > struct tcmu_dev *udev = cmd->tcmu_dev;
> > + bool read_len_valid = false;
> > + uint32_t read_len = 0xffffffff;
>
> Why do you set read_len? Is it to make the compiler happy? Because it
> only gets used if read_len_valid is set, in which case it is also set,
> right?
I changed gather_data_area() to copy data from uio buffer until either the
sg list is exhausted or read_len is reached.
gather_data_area() then writes back to read_len the length of the data that
really was transferred.
So, if a userspace program defines a too big read_len, it is truncated to
what is defined in the sg list.
Initializing read_len to a very high value avoids truncation in case the user
did not set an explicit read_len.
So gather_data_area() can be called with the pointer to read_len regardless
of read_len_valid being true or false.
I also thought about initializing read_len to se_cmd->data_length instead of
0xffffffff.
But honestly I wasn't sure whether this would be right in all cases. So I
stayed with the limitation to the sg list defined length as used by
gather_data_area().
If sg_cmd->data_length would be a correct limit for the transfer length also,
the check for too long read_length could be done in tcmu_handle_completion().
Then gather_data_area() would no longer need to write back read_len.
Maybe that would be the clearer code. What do you think?
> > + read_len_valid = (entry->hdr.uflags & TCMU_UFLAG_READ_LEN) &&
> > + entry->rsp.read_len;
> > + if (read_len_valid)
> > + read_len = entry->rsp.read_len;
>
> Again, perhaps it's just style, but this seems harder to read than:
>
> if (uflags & READ_LEN && read_len) {
> read_len_valid = true;
> read_len = entry...read_len;
> }
Yes, your code looks clearer to me also. I'll change it.
> > + if (read_len_valid) {
> > + pr_debug("read_len = %d\n", read_len);
> > + target_complete_cmd_with_length(cmd->se_cmd,
> > + entry->rsp.scsi_status, read_len);
> > + }
> > + else
>
> This is just wrong. don't put the else on a separate line please.
Oh, yes, I'll change it. Maybe this time I was a bit old-fashioned.
Thank you for the hints and corrections. I'll send an updated patch.
Bodo
--
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
