On Mon, 11 Feb 2013 10:33:40 +0200
Alexander Nezhinsky <nezhinsky@xxxxxxxxx> wrote:
> On Mon, Feb 11, 2013 at 2:07 AM, FUJITA Tomonori
> <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>
>>> From: Alexander Nezhinsky <nezhinsky@xxxxxxxxx>
>>>
>>> + * 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 *)®_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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
