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

[Alexander Nezhinsky]

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.

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