[stgt] [PATCH 02/13] mem_copy_n32: safe memcpy, accumulates copied count, tracks remaining space

[FUJITA Tomonori]

On Mon, 11 Feb 2013 10:33:40 +0200
Alexander Nezhinsky <nezhinsky at gmail.com> wrote:

> On Mon, Feb 11, 2013 at 2:07 AM, FUJITA Tomonori
> <fujita.tomonori at lab.ntt.co.jp> wrote:
> 
>>> From: Alexander Nezhinsky <nezhinsky at gmail.com>
>>>
>>> + * Copy up to src_len bytes from src,
>>> + * not exceeding *remain_len bytes available at dst.
>>> + * Return actually copied length.
>>> + * Reflect decreased space in *remain_len (mandatory).
>>> + * Accumulate copied length in *avail_len (optional).
>>> + */
>>> +int mem_copy_n32(uint8_t *dst, uint8_t *src, uint32_t src_len,
>>> +              uint32_t *avail_len, uint32_t *remain_len)
> 
>> Something like the following looks more reasonable?
>> scsi_memcpy(dst, str_len, src, src_len)
>>
>> I'm not sure about 'avail_len'. src_len is const so it doesn't make
>> sense to calculate avail_len inside this function.
> 
> avail_len (or its equivalent under alternatve name) is necessary, as it is
> sometimes used to accumulate the available buffer length through a
> sequence of nested calls.
> For example, please see build_mode_page() usage pattern.

I already read that code. Why you can't simply do something like:

actual_len = mem_copy_n32(data, hdr, hdr_size,
	                  remain_len);
*avail_len += hdr_size;

mem_copy_n32 unconditionally adds src_len to avail_len. I don't see
any point of doing that in mem_copy_n32().


> The source of confusion (also referred to from a couple of your other RE: mails)
> seems to be the parameter and variable names: avail_len passed as the value
> of src_len parameter, and NULL passed for *avail_len parameter.
> 
> What about having the function split int two (eliminates occasional NULL arg)
> and avail_len renamed to accum_len in the special "accumulating" version?
> 
> int spc_memcpy(uint8_t *dst, uint8_t *src, uint32_t src_len, uint32_t
> *remain_len)
> {
>         int copy_len = min_t(uint32_t, *remain_len, src_len);
>         if (copy_len) {
>                 memcpy(dst, src, copy_len);
>                 *remain_len -= copy_len;
>         }
>         return copy_len;
> }
> 
> int spc_accum_memcpy(uint8_t *dst, uint8_t *src, uint32_t src_len,
>         uint32_t *remain_len, uinnt32_t *accum_len)
> {
>         spc_memcpy(dst, src, src_len, remain_len);
>         *accum_len += src_len;
> }
> 
> so for example in  report_opcodes_all() the call would be :
> actual_len = spc_memcpy(scsi_get_in_buffer(cmd), buf, avail_len, &alloc_len);
> 
> while in spc_pr_read_keys() it would be :
> actual_len += spc_accum_memcpy(&buf[actual_len], (uint8_t *)&reg_key, 8,
>                                            &remain_len,  /* update
> remaining len here */
>                                            &avail_len); /* accumulate
> available len here */
> 
>>> +void set_byte_n32(int val, uint8_t *dst, uint32_t index, uint32_t
>>> max_len)
>>> +{
>>> +     if (index < max_len)
>>> +             dst[index] = (uint8_t)val;
>>> +}
>>
>> This function could be used by other than mod_sense? If not, I prefer
>> to have more explicit function name in spc.c
> 
> what about safe_set_byte(...) ?
> i'd leave it in utils.c, but we can move to spc.c as well
> 
> What do you think?
> 
> Alexander
> --
> To unsubscribe from this list: send the line "unsubscribe stgt" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html