Hi Andrzej,
thank you for the comment.
2015-02-02 11:33 GMT+01:00 Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxx>:
> Hi Mario,
>
> W dniu 01.02.2015 o 21:07, Mario Schuknecht pisze:
>>
>> A popular proprietary operating system expects that USB devices provide
>> extra
>> information via "OS descriptors". An introduction to the subject can be
>> found
>> here:
>>
>
> <snip>
>
>>
>> This patch adds support for property data type 0x7 multiple NUL-terminated
>> unicode strings.
>>
> This suggests that you mean
>
> #define USB_EXT_PROP_UNICODE_MULTI 7
>
>> Add case USB_EXT_PROP_UNICODE_MULTI in function fill_ext_prop()
>
>
> And you do.
>
>>
>> Signed-off-by: Mario Schuknecht <mario.schuknecht@xxxxxxxxxxxxxxx>
>> ---
>>
>> Changes in v2:
>> - improve commit log
>>
>> drivers/usb/gadget/composite.c | 5 +++++
>> drivers/usb/gadget/u_os_desc.h | 28 ++++++++++++++++++++++++++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c
>> b/drivers/usb/gadget/composite.c
>> index 6178353..bf30b73 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -1422,6 +1422,11 @@ static int fill_ext_prop(struct usb_configuration
>> *c, int interface, u8 *buf)
>> ext_prop->data,
>>
>> ext_prop->data_len);
>> break;
>> + case USB_EXT_PROP_UNICODE_LINK_MULTI:
>
> Does it compile? I mean the _LINK_ infix - didn't you mean
>
> + case USB_EXT_PROP_UNICODE_MULTI:
>
> instead?
You are right. Sorry.
>
>> +
>> usb_ext_prop_put_unicode_multi(buf, ret,
>> + ext_prop->data,
>> +
>> ext_prop->data_len);
>> + break;
>> case USB_EXT_PROP_BINARY:
>> usb_ext_prop_put_binary(buf, ret,
>> ext_prop->data,
>> diff --git a/drivers/usb/gadget/u_os_desc.h
>> b/drivers/usb/gadget/u_os_desc.h
>> index 947b7dd..bee4274 100644
>> --- a/drivers/usb/gadget/u_os_desc.h
>> +++ b/drivers/usb/gadget/u_os_desc.h
>> @@ -120,4 +120,32 @@ static inline int usb_ext_prop_put_unicode(u8 *buf,
>> int pnl, const char *string,
>> return data_len;
>> }
>>
>
> I'm not sure I understand the logic of the below function well.
>
>> +static inline int usb_ext_prop_put_unicode_multi(u8 *buf, int pnl,
>> + const char *string, int data_len)
>> +{
>> + int outlen = 0;
>> +
>> + put_unaligned_le32(data_len, usb_ext_prop_data_len_ptr(buf, pnl));
>> +
>> + while (*string && outlen < data_len - 2) {
>
>
> You keep looping as long as the source *string is not '\0'
> and advance the string in each loop by adding (strlen() + 1) of
> what is currently available starting at *string.
> For example:
>
> string: "a\0b\0and this is past the end of your source buffer"
>
> first loop iteration:
> len = strlen(string); /* len == 1 */
> string += len + 1; /* string: "b\0and this is past the end of your source
> buffer" */
>
> second loop iteration:
> len = strlen(string); /* len == 1 */
> string += len + 1; /* string: "and this is past the end of your source
> buffer" */
>
> so effectively the first part of the while condition rarely ever becomes
> "false".
> In other words when you process all the source strings from "string" you, by
> design,
> end up one byte past the terminating '\0' of the source buffer. The contents
> of this memory can be anything, there is just 1/256 chance it is zero,
> so the "while (*string" part does not make sense to me.
>
The assumtion is that the input string is also double Nul-terminated.
E.g. "one\0two\0three\0\0"
Should I add a parameter "inlen" which contains the input buffer length?
Or can I trust that the input string is double Nul-terminated?
Or is there a better way?
>
>> + int len = strlen(string);
>> + int result = utf8s_to_utf16s(string, len,
>> UTF16_LITTLE_ENDIAN,
>> + (wchar_t *) usb_ext_prop_data_ptr(buf, pnl +
>> outlen),
>> + data_len - outlen - 2);
>> + if (result < 0)
>> + return result;
>> + outlen += result << 1;
>> + string += len + 1;
>> + put_unaligned_le16(0,
>> + &buf[USB_EXT_PROP_B_PROPERTY_DATA + pnl +
>> outlen]);
>
> You put a terminating NUL "two-byte" after each destination, "wide" string.
>
>> + outlen += 2;
>
>
> Then you advance the "outlen" to reflect the above.
>
>
>> + }
>
> Suppose there is just one string in the source "string", so the loop
> terminates now.
>
> Am I correct that at this point "outlen" is supposed to be equal "data_len"?
> If it is, than please see below (*).
>
>
>> +
>> + put_unaligned_le16(0,
>> + &buf[USB_EXT_PROP_B_PROPERTY_DATA + pnl +
>> outlen]);
>
>
> Why again terminate the destination string? And, since outlen value was
> incremented by 2 in the meantime there will be two terminators in a row?
>
>> + outlen += 2;
>
>
It is supposed that outlen is less than (data_len - 2) inside the while-loop.
If I suppose the following input string
"one\0\0"
then data_len has to be >= 10.
After utf8s_to_utf16s(..) the output buffer contains:
"o\0n\0e\0"
outlen = 6
Add terminating Nul
"o\0n\0e\0\0\0"
outlen = 8
Leave the loop and add the double terminating Nul
"o\0n\0e\0\0\0\0\0"
outlen = 10
Could I answer your question?
> (*)
> If "outlen" was supposed to be equal "data_len" above then the
> return value of the whole function becomes data_len + 2.
>
>
>> +
>> + return outlen;
>> +}
>> +
>> #endif /* __U_OS_DESC_H__ */
>>
>
Regards,
Mario
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
