Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

Hi,

Stefan Agner  writes:
> Hi Felipe,
>
> On 2017-04-19 01:53, Krzysztof Opasiak wrote:
>> On 04/15/2017 03:35 AM, Stefan Agner wrote:
>>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> when read. This isn't obvious when simply using cat since the null
>>> characters are not visible, but hexdump unveils the true string:
>>>
>>>   # echo MSFT100 > os_desc/qw_sign
>>>   # hexdump -C os_desc/qw_sign
>>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>>> |M.S.F.T.1.0.0.|
>>>
>>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> common convention and add a new line at the end.
>>>
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> Resend as discussed here:
>>> https://patchwork.kernel.org/patch/9548869/
>>>
>>> Sorry, a bit later than we discussed... Hope still not too late?
>>>
>>> --
>>> Stefan
>>>
>>>  drivers/usb/gadget/configfs.c | 8 ++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>>> index cbff3b02840d..863ca4ded1be 100644
>>> --- a/drivers/usb/gadget/configfs.c
>>> +++ b/drivers/usb/gadget/configfs.c
>>> @@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
>>> config_item *item,
>>>  static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
>>>  {
>>> struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>> +   int res;
>>>
>>> -   memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
>>> -   return OS_STRING_QW_SIGN_LEN;
>>> +   res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
>>> + UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
>>> +   page[res++] = '\n';
>>> +
>>> +   return res;
>>>  }
>>>
>>>  static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
>>> *page,
>>>
>> 
>> Code itself looks good to me and from libusbgx perspective it's also
>> fine to add this new line as we can just drop it like we do with other
>> newlines in case of gadget/config strings.
>> 
>> Reviewed-by: Krzysztof Opasiak 
>> 
>
> Any chance we get this in this merge window?
>
> This still applies fine on v4.12-rc2.

I wouldn't consider this a fix. I'll get this into v4.13 merge window.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

Hi,

Stefan Agner  writes:
> Hi Felipe,
>
> On 2017-04-19 01:53, Krzysztof Opasiak wrote:
>> On 04/15/2017 03:35 AM, Stefan Agner wrote:
>>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> when read. This isn't obvious when simply using cat since the null
>>> characters are not visible, but hexdump unveils the true string:
>>>
>>>   # echo MSFT100 > os_desc/qw_sign
>>>   # hexdump -C os_desc/qw_sign
>>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>>> |M.S.F.T.1.0.0.|
>>>
>>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> common convention and add a new line at the end.
>>>
>>> Signed-off-by: Stefan Agner 
>>> ---
>>> Resend as discussed here:
>>> https://patchwork.kernel.org/patch/9548869/
>>>
>>> Sorry, a bit later than we discussed... Hope still not too late?
>>>
>>> --
>>> Stefan
>>>
>>>  drivers/usb/gadget/configfs.c | 8 ++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>>> index cbff3b02840d..863ca4ded1be 100644
>>> --- a/drivers/usb/gadget/configfs.c
>>> +++ b/drivers/usb/gadget/configfs.c
>>> @@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
>>> config_item *item,
>>>  static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
>>>  {
>>> struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>> +   int res;
>>>
>>> -   memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
>>> -   return OS_STRING_QW_SIGN_LEN;
>>> +   res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
>>> + UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
>>> +   page[res++] = '\n';
>>> +
>>> +   return res;
>>>  }
>>>
>>>  static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
>>> *page,
>>>
>> 
>> Code itself looks good to me and from libusbgx perspective it's also
>> fine to add this new line as we can just drop it like we do with other
>> newlines in case of gadget/config strings.
>> 
>> Reviewed-by: Krzysztof Opasiak 
>> 
>
> Any chance we get this in this merge window?
>
> This still applies fine on v4.12-rc2.

I wouldn't consider this a fix. I'll get this into v4.13 merge window.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

Hi Felipe,

On 2017-04-19 01:53, Krzysztof Opasiak wrote:
> On 04/15/2017 03:35 AM, Stefan Agner wrote:
>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> when read. This isn't obvious when simply using cat since the null
>> characters are not visible, but hexdump unveils the true string:
>>
>>   # echo MSFT100 > os_desc/qw_sign
>>   # hexdump -C os_desc/qw_sign
>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>> |M.S.F.T.1.0.0.|
>>
>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> common convention and add a new line at the end.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>> Resend as discussed here:
>> https://patchwork.kernel.org/patch/9548869/
>>
>> Sorry, a bit later than we discussed... Hope still not too late?
>>
>> --
>> Stefan
>>
>>  drivers/usb/gadget/configfs.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index cbff3b02840d..863ca4ded1be 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
>> config_item *item,
>>  static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
>>  {
>>  struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>> +int res;
>>
>> -memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
>> -return OS_STRING_QW_SIGN_LEN;
>> +res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
>> +  UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
>> +page[res++] = '\n';
>> +
>> +return res;
>>  }
>>
>>  static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
>> *page,
>>
> 
> Code itself looks good to me and from libusbgx perspective it's also
> fine to add this new line as we can just drop it like we do with other
> newlines in case of gadget/config strings.
> 
> Reviewed-by: Krzysztof Opasiak 
> 

Any chance we get this in this merge window?

This still applies fine on v4.12-rc2.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

Hi Felipe,

On 2017-04-19 01:53, Krzysztof Opasiak wrote:
> On 04/15/2017 03:35 AM, Stefan Agner wrote:
>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> when read. This isn't obvious when simply using cat since the null
>> characters are not visible, but hexdump unveils the true string:
>>
>>   # echo MSFT100 > os_desc/qw_sign
>>   # hexdump -C os_desc/qw_sign
>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>> |M.S.F.T.1.0.0.|
>>
>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> common convention and add a new line at the end.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>> Resend as discussed here:
>> https://patchwork.kernel.org/patch/9548869/
>>
>> Sorry, a bit later than we discussed... Hope still not too late?
>>
>> --
>> Stefan
>>
>>  drivers/usb/gadget/configfs.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index cbff3b02840d..863ca4ded1be 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
>> config_item *item,
>>  static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
>>  {
>>  struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>> +int res;
>>
>> -memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
>> -return OS_STRING_QW_SIGN_LEN;
>> +res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
>> +  UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
>> +page[res++] = '\n';
>> +
>> +return res;
>>  }
>>
>>  static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
>> *page,
>>
> 
> Code itself looks good to me and from libusbgx perspective it's also
> fine to add this new line as we can just drop it like we do with other
> newlines in case of gadget/config strings.
> 
> Reviewed-by: Krzysztof Opasiak 
> 

Any chance we get this in this merge window?

This still applies fine on v4.12-rc2.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Krzysztof Opasiak]

On 04/15/2017 03:35 AM, Stefan Agner wrote:

Currently qw_sign requires UTF-8 character to set, but returns UTF-16
when read. This isn't obvious when simply using cat since the null
characters are not visible, but hexdump unveils the true string:

  # echo MSFT100 > os_desc/qw_sign
  # hexdump -C os_desc/qw_sign
    4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|

Make qw_sign symmetric by returning an UTF-8 string too. Also follow
common convention and add a new line at the end.

Signed-off-by: Stefan Agner 
---
Resend as discussed here:
https://patchwork.kernel.org/patch/9548869/

Sorry, a bit later than we discussed... Hope still not too late?

--
Stefan

 drivers/usb/gadget/configfs.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index cbff3b02840d..863ca4ded1be 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
config_item *item,
 static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
 {
struct gadget_info *gi = os_desc_item_to_gadget_info(item);
+   int res;

-   memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
-   return OS_STRING_QW_SIGN_LEN;
+   res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
+ UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
+   page[res++] = '\n';
+
+   return res;
 }

 static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
*page,



Code itself looks good to me and from libusbgx perspective it's also 
fine to add this new line as we can just drop it like we do with other 
newlines in case of gadget/config strings.


Reviewed-by: Krzysztof Opasiak 

Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Krzysztof Opasiak]

On 04/15/2017 03:35 AM, Stefan Agner wrote:

Currently qw_sign requires UTF-8 character to set, but returns UTF-16
when read. This isn't obvious when simply using cat since the null
characters are not visible, but hexdump unveils the true string:

  # echo MSFT100 > os_desc/qw_sign
  # hexdump -C os_desc/qw_sign
    4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|

Make qw_sign symmetric by returning an UTF-8 string too. Also follow
common convention and add a new line at the end.

Signed-off-by: Stefan Agner 
---
Resend as discussed here:
https://patchwork.kernel.org/patch/9548869/

Sorry, a bit later than we discussed... Hope still not too late?

--
Stefan

 drivers/usb/gadget/configfs.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index cbff3b02840d..863ca4ded1be 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -787,9 +787,13 @@ static ssize_t os_desc_b_vendor_code_store(struct 
config_item *item,
 static ssize_t os_desc_qw_sign_show(struct config_item *item, char *page)
 {
struct gadget_info *gi = os_desc_item_to_gadget_info(item);
+   int res;

-   memcpy(page, gi->qw_sign, OS_STRING_QW_SIGN_LEN);
-   return OS_STRING_QW_SIGN_LEN;
+   res = utf16s_to_utf8s((wchar_t *) gi->qw_sign, OS_STRING_QW_SIGN_LEN,
+ UTF16_LITTLE_ENDIAN, page, PAGE_SIZE - 1);
+   page[res++] = '\n';
+
+   return res;
 }

 static ssize_t os_desc_qw_sign_store(struct config_item *item, const char 
*page,



Code itself looks good to me and from libusbgx perspective it's also 
fine to add this new line as we can just drop it like we do with other 
newlines in case of gadget/config strings.


Reviewed-by: Krzysztof Opasiak 

Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

Hi,

Stefan Agner  writes:
>>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
 On 2017-02-01 08:59, Stefan Agner wrote:
 > On 2017-02-01 00:06, Greg KH wrote:
 >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
 >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
 >>> when read. This isn't obvious when simply using cat since the null
 >>> characters are not visible, but hexdump unveils the true string:
 >>>
 >>>   # echo MSFT100 > os_desc/qw_sign
 >>>   # hexdump -C os_desc/qw_sign
 >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
 >>> |M.S.F.T.1.0.0.|
 >>>
 >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
 >>> common convention and add a new line at the end.
 >>
 >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
 >> convert them?
 >
 > That is a discussion we should have had when the write side of this has
 > been added:
 >
 > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
 > char *page,
 >   size_t len)
 > {
 >  struct gadget_info *gi = os_desc_item_to_gadget_info(item);
 >  int res, l;
 >
 >  l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
 >  if (page[l - 1] == '\n')
 >  --l;
 >
 >  mutex_lock(>lock);
 >  res = utf8s_to_utf16s(page, l,
 >UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
 >OS_STRING_QW_SIGN_LEN);
 >  if (res > 0)
 >  res = len;
 >  mutex_unlock(>lock);
 >
 >  return res;
 > }
 >
 >
 > The store function is definitely already in use today, e.g. this script
 > used for ev3dev:
 > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
 >
 > Changing it to UTF-16 would break that script... So changing the store
 > part is the lesser of two evils.
 >
 > Regarding new line: Just following what other attributes are doing by
 > using the GS_STRINGS_R macro.

 Any comment on this? In my opinion especially this first patch really
 fixes a bug and should get applied... I can remove the newline if
 preferred.
>>>
>>> It's up to Felipe, give him a chance to catch up on patches...
>> 
>> I really don't know what to do here :-) Either way have the potential of
>> breaking userspace. Maybe returning UTF8 as in write is the lesser of
>> two evils.
>
> So far libusbg and libusbgx do not have support for OS Descriptors.
>
> I came across the issue while writing support for libusbgx (not upstream
> yet). And it is rather ugly to have to do it on the read side and not to
> do on the write side. It would also extend the dependencies of the
> library (I implemented a prototype using iconv).
>
> Also reading the old interface with UTF-8 does not crash the library, it
> just return only the first character (since the second character is
> \0...).

Okay, then let's go with your original patch. Can you resend once -rc1
is out?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

Hi,

Stefan Agner  writes:
>>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
 On 2017-02-01 08:59, Stefan Agner wrote:
 > On 2017-02-01 00:06, Greg KH wrote:
 >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
 >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
 >>> when read. This isn't obvious when simply using cat since the null
 >>> characters are not visible, but hexdump unveils the true string:
 >>>
 >>>   # echo MSFT100 > os_desc/qw_sign
 >>>   # hexdump -C os_desc/qw_sign
 >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
 >>> |M.S.F.T.1.0.0.|
 >>>
 >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
 >>> common convention and add a new line at the end.
 >>
 >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
 >> convert them?
 >
 > That is a discussion we should have had when the write side of this has
 > been added:
 >
 > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
 > char *page,
 >   size_t len)
 > {
 >  struct gadget_info *gi = os_desc_item_to_gadget_info(item);
 >  int res, l;
 >
 >  l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
 >  if (page[l - 1] == '\n')
 >  --l;
 >
 >  mutex_lock(>lock);
 >  res = utf8s_to_utf16s(page, l,
 >UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
 >OS_STRING_QW_SIGN_LEN);
 >  if (res > 0)
 >  res = len;
 >  mutex_unlock(>lock);
 >
 >  return res;
 > }
 >
 >
 > The store function is definitely already in use today, e.g. this script
 > used for ev3dev:
 > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
 >
 > Changing it to UTF-16 would break that script... So changing the store
 > part is the lesser of two evils.
 >
 > Regarding new line: Just following what other attributes are doing by
 > using the GS_STRINGS_R macro.

 Any comment on this? In my opinion especially this first patch really
 fixes a bug and should get applied... I can remove the newline if
 preferred.
>>>
>>> It's up to Felipe, give him a chance to catch up on patches...
>> 
>> I really don't know what to do here :-) Either way have the potential of
>> breaking userspace. Maybe returning UTF8 as in write is the lesser of
>> two evils.
>
> So far libusbg and libusbgx do not have support for OS Descriptors.
>
> I came across the issue while writing support for libusbgx (not upstream
> yet). And it is rather ugly to have to do it on the read side and not to
> do on the write side. It would also extend the dependencies of the
> library (I implemented a prototype using iconv).
>
> Also reading the old interface with UTF-8 does not crash the library, it
> just return only the first character (since the second character is
> \0...).

Okay, then let's go with your original patch. Can you resend once -rc1
is out?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-10 04:30, Felipe Balbi wrote:
> Hi,
> 
> Greg KH  writes:
>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>>> On 2017-02-01 08:59, Stefan Agner wrote:
>>> > On 2017-02-01 00:06, Greg KH wrote:
>>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> >>> when read. This isn't obvious when simply using cat since the null
>>> >>> characters are not visible, but hexdump unveils the true string:
>>> >>>
>>> >>>   # echo MSFT100 > os_desc/qw_sign
>>> >>>   # hexdump -C os_desc/qw_sign
>>> >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>>> >>> |M.S.F.T.1.0.0.|
>>> >>>
>>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> >>> common convention and add a new line at the end.
>>> >>
>>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>>> >> convert them?
>>> >
>>> > That is a discussion we should have had when the write side of this has
>>> > been added:
>>> >
>>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>>> > char *page,
>>> >size_t len)
>>> > {
>>> >   struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>> >   int res, l;
>>> >
>>> >   l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>>> >   if (page[l - 1] == '\n')
>>> >   --l;
>>> >
>>> >   mutex_lock(>lock);
>>> >   res = utf8s_to_utf16s(page, l,
>>> > UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>>> > OS_STRING_QW_SIGN_LEN);
>>> >   if (res > 0)
>>> >   res = len;
>>> >   mutex_unlock(>lock);
>>> >
>>> >   return res;
>>> > }
>>> >
>>> >
>>> > The store function is definitely already in use today, e.g. this script
>>> > used for ev3dev:
>>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>>> >
>>> > Changing it to UTF-16 would break that script... So changing the store
>>> > part is the lesser of two evils.
>>> >
>>> > Regarding new line: Just following what other attributes are doing by
>>> > using the GS_STRINGS_R macro.
>>>
>>> Any comment on this? In my opinion especially this first patch really
>>> fixes a bug and should get applied... I can remove the newline if
>>> preferred.
>>
>> It's up to Felipe, give him a chance to catch up on patches...
> 
> I really don't know what to do here :-) Either way have the potential of
> breaking userspace. Maybe returning UTF8 as in write is the lesser of
> two evils.

So far libusbg and libusbgx do not have support for OS Descriptors.

I came across the issue while writing support for libusbgx (not upstream
yet). And it is rather ugly to have to do it on the read side and not to
do on the write side. It would also extend the dependencies of the
library (I implemented a prototype using iconv).

Also reading the old interface with UTF-8 does not crash the library, it
just return only the first character (since the second character is
\0...).

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-10 04:30, Felipe Balbi wrote:
> Hi,
> 
> Greg KH  writes:
>> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>>> On 2017-02-01 08:59, Stefan Agner wrote:
>>> > On 2017-02-01 00:06, Greg KH wrote:
>>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> >>> when read. This isn't obvious when simply using cat since the null
>>> >>> characters are not visible, but hexdump unveils the true string:
>>> >>>
>>> >>>   # echo MSFT100 > os_desc/qw_sign
>>> >>>   # hexdump -C os_desc/qw_sign
>>> >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>>> >>> |M.S.F.T.1.0.0.|
>>> >>>
>>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> >>> common convention and add a new line at the end.
>>> >>
>>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>>> >> convert them?
>>> >
>>> > That is a discussion we should have had when the write side of this has
>>> > been added:
>>> >
>>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>>> > char *page,
>>> >size_t len)
>>> > {
>>> >   struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>>> >   int res, l;
>>> >
>>> >   l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>>> >   if (page[l - 1] == '\n')
>>> >   --l;
>>> >
>>> >   mutex_lock(>lock);
>>> >   res = utf8s_to_utf16s(page, l,
>>> > UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>>> > OS_STRING_QW_SIGN_LEN);
>>> >   if (res > 0)
>>> >   res = len;
>>> >   mutex_unlock(>lock);
>>> >
>>> >   return res;
>>> > }
>>> >
>>> >
>>> > The store function is definitely already in use today, e.g. this script
>>> > used for ev3dev:
>>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>>> >
>>> > Changing it to UTF-16 would break that script... So changing the store
>>> > part is the lesser of two evils.
>>> >
>>> > Regarding new line: Just following what other attributes are doing by
>>> > using the GS_STRINGS_R macro.
>>>
>>> Any comment on this? In my opinion especially this first patch really
>>> fixes a bug and should get applied... I can remove the newline if
>>> preferred.
>>
>> It's up to Felipe, give him a chance to catch up on patches...
> 
> I really don't know what to do here :-) Either way have the potential of
> breaking userspace. Maybe returning UTF8 as in write is the lesser of
> two evils.

So far libusbg and libusbgx do not have support for OS Descriptors.

I came across the issue while writing support for libusbgx (not upstream
yet). And it is rather ugly to have to do it on the read side and not to
do on the write side. It would also extend the dependencies of the
library (I implemented a prototype using iconv).

Also reading the old interface with UTF-8 does not crash the library, it
just return only the first character (since the second character is
\0...).

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

Hi,

Greg KH  writes:
> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>> On 2017-02-01 08:59, Stefan Agner wrote:
>> > On 2017-02-01 00:06, Greg KH wrote:
>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> >>> when read. This isn't obvious when simply using cat since the null
>> >>> characters are not visible, but hexdump unveils the true string:
>> >>>
>> >>>   # echo MSFT100 > os_desc/qw_sign
>> >>>   # hexdump -C os_desc/qw_sign
>> >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>> >>> |M.S.F.T.1.0.0.|
>> >>>
>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> >>> common convention and add a new line at the end.
>> >>
>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>> >> convert them?
>> > 
>> > That is a discussion we should have had when the write side of this has
>> > been added:
>> > 
>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>> > char *page,
>> > size_t len)
>> > {
>> >struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>> >int res, l;
>> > 
>> >l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>> >if (page[l - 1] == '\n')
>> >--l;
>> > 
>> >mutex_lock(>lock);
>> >res = utf8s_to_utf16s(page, l,
>> >  UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>> >  OS_STRING_QW_SIGN_LEN);
>> >if (res > 0)
>> >res = len;
>> >mutex_unlock(>lock);
>> > 
>> >return res;
>> > }
>> > 
>> > 
>> > The store function is definitely already in use today, e.g. this script
>> > used for ev3dev:
>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>> > 
>> > Changing it to UTF-16 would break that script... So changing the store
>> > part is the lesser of two evils.
>> > 
>> > Regarding new line: Just following what other attributes are doing by
>> > using the GS_STRINGS_R macro.
>> 
>> Any comment on this? In my opinion especially this first patch really
>> fixes a bug and should get applied... I can remove the newline if
>> preferred.
>
> It's up to Felipe, give him a chance to catch up on patches...

I really don't know what to do here :-) Either way have the potential of
breaking userspace. Maybe returning UTF8 as in write is the lesser of
two evils.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Felipe Balbi]

Hi,

Greg KH  writes:
> On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
>> On 2017-02-01 08:59, Stefan Agner wrote:
>> > On 2017-02-01 00:06, Greg KH wrote:
>> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> >>> when read. This isn't obvious when simply using cat since the null
>> >>> characters are not visible, but hexdump unveils the true string:
>> >>>
>> >>>   # echo MSFT100 > os_desc/qw_sign
>> >>>   # hexdump -C os_desc/qw_sign
>> >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>> >>> |M.S.F.T.1.0.0.|
>> >>>
>> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> >>> common convention and add a new line at the end.
>> >>
>> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>> >> convert them?
>> > 
>> > That is a discussion we should have had when the write side of this has
>> > been added:
>> > 
>> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
>> > char *page,
>> > size_t len)
>> > {
>> >struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>> >int res, l;
>> > 
>> >l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>> >if (page[l - 1] == '\n')
>> >--l;
>> > 
>> >mutex_lock(>lock);
>> >res = utf8s_to_utf16s(page, l,
>> >  UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
>> >  OS_STRING_QW_SIGN_LEN);
>> >if (res > 0)
>> >res = len;
>> >mutex_unlock(>lock);
>> > 
>> >return res;
>> > }
>> > 
>> > 
>> > The store function is definitely already in use today, e.g. this script
>> > used for ev3dev:
>> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
>> > 
>> > Changing it to UTF-16 would break that script... So changing the store
>> > part is the lesser of two evils.
>> > 
>> > Regarding new line: Just following what other attributes are doing by
>> > using the GS_STRINGS_R macro.
>> 
>> Any comment on this? In my opinion especially this first patch really
>> fixes a bug and should get applied... I can remove the newline if
>> preferred.
>
> It's up to Felipe, give him a chance to catch up on patches...

I really don't know what to do here :-) Either way have the potential of
breaking userspace. Maybe returning UTF8 as in write is the lesser of
two evils.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
> On 2017-02-01 08:59, Stefan Agner wrote:
> > On 2017-02-01 00:06, Greg KH wrote:
> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> >>> when read. This isn't obvious when simply using cat since the null
> >>> characters are not visible, but hexdump unveils the true string:
> >>>
> >>>   # echo MSFT100 > os_desc/qw_sign
> >>>   # hexdump -C os_desc/qw_sign
> >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
> >>> |M.S.F.T.1.0.0.|
> >>>
> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> >>> common convention and add a new line at the end.
> >>
> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
> >> convert them?
> > 
> > That is a discussion we should have had when the write side of this has
> > been added:
> > 
> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
> > char *page,
> >  size_t len)
> > {
> > struct gadget_info *gi = os_desc_item_to_gadget_info(item);
> > int res, l;
> > 
> > l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
> > if (page[l - 1] == '\n')
> > --l;
> > 
> > mutex_lock(>lock);
> > res = utf8s_to_utf16s(page, l,
> >   UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
> >   OS_STRING_QW_SIGN_LEN);
> > if (res > 0)
> > res = len;
> > mutex_unlock(>lock);
> > 
> > return res;
> > }
> > 
> > 
> > The store function is definitely already in use today, e.g. this script
> > used for ev3dev:
> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
> > 
> > Changing it to UTF-16 would break that script... So changing the store
> > part is the lesser of two evils.
> > 
> > Regarding new line: Just following what other attributes are doing by
> > using the GS_STRINGS_R macro.
> 
> Any comment on this? In my opinion especially this first patch really
> fixes a bug and should get applied... I can remove the newline if
> preferred.

It's up to Felipe, give him a chance to catch up on patches...

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Thu, Feb 09, 2017 at 10:04:43AM -0800, Stefan Agner wrote:
> On 2017-02-01 08:59, Stefan Agner wrote:
> > On 2017-02-01 00:06, Greg KH wrote:
> >> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> >>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> >>> when read. This isn't obvious when simply using cat since the null
> >>> characters are not visible, but hexdump unveils the true string:
> >>>
> >>>   # echo MSFT100 > os_desc/qw_sign
> >>>   # hexdump -C os_desc/qw_sign
> >>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
> >>> |M.S.F.T.1.0.0.|
> >>>
> >>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> >>> common convention and add a new line at the end.
> >>
> >> Doesn't USB require that strings be in UTF-16?  So why have the kernel
> >> convert them?
> > 
> > That is a discussion we should have had when the write side of this has
> > been added:
> > 
> > static ssize_t os_desc_qw_sign_store(struct config_item *item, const
> > char *page,
> >  size_t len)
> > {
> > struct gadget_info *gi = os_desc_item_to_gadget_info(item);
> > int res, l;
> > 
> > l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
> > if (page[l - 1] == '\n')
> > --l;
> > 
> > mutex_lock(>lock);
> > res = utf8s_to_utf16s(page, l,
> >   UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
> >   OS_STRING_QW_SIGN_LEN);
> > if (res > 0)
> > res = len;
> > mutex_unlock(>lock);
> > 
> > return res;
> > }
> > 
> > 
> > The store function is definitely already in use today, e.g. this script
> > used for ev3dev:
> > https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
> > 
> > Changing it to UTF-16 would break that script... So changing the store
> > part is the lesser of two evils.
> > 
> > Regarding new line: Just following what other attributes are doing by
> > using the GS_STRINGS_R macro.
> 
> Any comment on this? In my opinion especially this first patch really
> fixes a bug and should get applied... I can remove the newline if
> preferred.

It's up to Felipe, give him a chance to catch up on patches...

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-01 08:59, Stefan Agner wrote:
> On 2017-02-01 00:06, Greg KH wrote:
>> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> when read. This isn't obvious when simply using cat since the null
>>> characters are not visible, but hexdump unveils the true string:
>>>
>>>   # echo MSFT100 > os_desc/qw_sign
>>>   # hexdump -C os_desc/qw_sign
>>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>>> |M.S.F.T.1.0.0.|
>>>
>>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> common convention and add a new line at the end.
>>
>> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>> convert them?
> 
> That is a discussion we should have had when the write side of this has
> been added:
> 
> static ssize_t os_desc_qw_sign_store(struct config_item *item, const
> char *page,
>size_t len)
> {
>   struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>   int res, l;
> 
>   l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>   if (page[l - 1] == '\n')
>   --l;
> 
>   mutex_lock(>lock);
>   res = utf8s_to_utf16s(page, l,
> UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
> OS_STRING_QW_SIGN_LEN);
>   if (res > 0)
>   res = len;
>   mutex_unlock(>lock);
> 
>   return res;
> }
> 
> 
> The store function is definitely already in use today, e.g. this script
> used for ev3dev:
> https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
> 
> Changing it to UTF-16 would break that script... So changing the store
> part is the lesser of two evils.
> 
> Regarding new line: Just following what other attributes are doing by
> using the GS_STRINGS_R macro.

Any comment on this? In my opinion especially this first patch really
fixes a bug and should get applied... I can remove the newline if
preferred.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-01 08:59, Stefan Agner wrote:
> On 2017-02-01 00:06, Greg KH wrote:
>> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>>> when read. This isn't obvious when simply using cat since the null
>>> characters are not visible, but hexdump unveils the true string:
>>>
>>>   # echo MSFT100 > os_desc/qw_sign
>>>   # hexdump -C os_desc/qw_sign
>>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>>> |M.S.F.T.1.0.0.|
>>>
>>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>>> common convention and add a new line at the end.
>>
>> Doesn't USB require that strings be in UTF-16?  So why have the kernel
>> convert them?
> 
> That is a discussion we should have had when the write side of this has
> been added:
> 
> static ssize_t os_desc_qw_sign_store(struct config_item *item, const
> char *page,
>size_t len)
> {
>   struct gadget_info *gi = os_desc_item_to_gadget_info(item);
>   int res, l;
> 
>   l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
>   if (page[l - 1] == '\n')
>   --l;
> 
>   mutex_lock(>lock);
>   res = utf8s_to_utf16s(page, l,
> UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
> OS_STRING_QW_SIGN_LEN);
>   if (res > 0)
>   res = len;
>   mutex_unlock(>lock);
> 
>   return res;
> }
> 
> 
> The store function is definitely already in use today, e.g. this script
> used for ev3dev:
> https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh
> 
> Changing it to UTF-16 would break that script... So changing the store
> part is the lesser of two evils.
> 
> Regarding new line: Just following what other attributes are doing by
> using the GS_STRINGS_R macro.

Any comment on this? In my opinion especially this first patch really
fixes a bug and should get applied... I can remove the newline if
preferred.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-01 00:06, Greg KH wrote:
> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> when read. This isn't obvious when simply using cat since the null
>> characters are not visible, but hexdump unveils the true string:
>>
>>   # echo MSFT100 > os_desc/qw_sign
>>   # hexdump -C os_desc/qw_sign
>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>> |M.S.F.T.1.0.0.|
>>
>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> common convention and add a new line at the end.
> 
> Doesn't USB require that strings be in UTF-16?  So why have the kernel
> convert them?

That is a discussion we should have had when the write side of this has
been added:

static ssize_t os_desc_qw_sign_store(struct config_item *item, const
char *page,
 size_t len)
{
struct gadget_info *gi = os_desc_item_to_gadget_info(item);
int res, l;

l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
if (page[l - 1] == '\n')
--l;

mutex_lock(>lock);
res = utf8s_to_utf16s(page, l,
  UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
  OS_STRING_QW_SIGN_LEN);
if (res > 0)
res = len;
mutex_unlock(>lock);

return res;
}


The store function is definitely already in use today, e.g. this script
used for ev3dev:
https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh

Changing it to UTF-16 would break that script... So changing the store
part is the lesser of two evils.

Regarding new line: Just following what other attributes are doing by
using the GS_STRINGS_R macro.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Stefan Agner]

On 2017-02-01 00:06, Greg KH wrote:
> On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
>> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
>> when read. This isn't obvious when simply using cat since the null
>> characters are not visible, but hexdump unveils the true string:
>>
>>   # echo MSFT100 > os_desc/qw_sign
>>   # hexdump -C os_desc/qw_sign
>>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00
>> |M.S.F.T.1.0.0.|
>>
>> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
>> common convention and add a new line at the end.
> 
> Doesn't USB require that strings be in UTF-16?  So why have the kernel
> convert them?

That is a discussion we should have had when the write side of this has
been added:

static ssize_t os_desc_qw_sign_store(struct config_item *item, const
char *page,
 size_t len)
{
struct gadget_info *gi = os_desc_item_to_gadget_info(item);
int res, l;

l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
if (page[l - 1] == '\n')
--l;

mutex_lock(>lock);
res = utf8s_to_utf16s(page, l,
  UTF16_LITTLE_ENDIAN, (wchar_t *) gi->qw_sign,
  OS_STRING_QW_SIGN_LEN);
if (res > 0)
res = len;
mutex_unlock(>lock);

return res;
}


The store function is definitely already in use today, e.g. this script
used for ev3dev:
https://github.com/ev3dev/ev3-systemd/blob/ev3dev-jessie/scripts/ev3-usb.sh

Changing it to UTF-16 would break that script... So changing the store
part is the lesser of two evils.

Regarding new line: Just following what other attributes are doing by
using the GS_STRINGS_R macro.

--
Stefan

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> when read. This isn't obvious when simply using cat since the null
> characters are not visible, but hexdump unveils the true string:
> 
>   # echo MSFT100 > os_desc/qw_sign
>   # hexdump -C os_desc/qw_sign
>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|
> 
> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> common convention and add a new line at the end.

The newline is not part of the descriptor, so why add it?

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> when read. This isn't obvious when simply using cat since the null
> characters are not visible, but hexdump unveils the true string:
> 
>   # echo MSFT100 > os_desc/qw_sign
>   # hexdump -C os_desc/qw_sign
>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|
> 
> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> common convention and add a new line at the end.

The newline is not part of the descriptor, so why add it?

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> when read. This isn't obvious when simply using cat since the null
> characters are not visible, but hexdump unveils the true string:
> 
>   # echo MSFT100 > os_desc/qw_sign
>   # hexdump -C os_desc/qw_sign
>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|
> 
> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> common convention and add a new line at the end.

Doesn't USB require that strings be in UTF-16?  So why have the kernel
convert them?

thanks,

greg k-h

Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

[Greg KH]

On Tue, Jan 31, 2017 at 06:19:16PM -0800, Stefan Agner wrote:
> Currently qw_sign requires UTF-8 character to set, but returns UTF-16
> when read. This isn't obvious when simply using cat since the null
> characters are not visible, but hexdump unveils the true string:
> 
>   # echo MSFT100 > os_desc/qw_sign
>   # hexdump -C os_desc/qw_sign
>     4d 00 53 00 46 00 54 00  31 00 30 00 30 00|M.S.F.T.1.0.0.|
> 
> Make qw_sign symmetric by returning an UTF-8 string too. Also follow
> common convention and add a new line at the end.

Doesn't USB require that strings be in UTF-16?  So why have the kernel
convert them?

thanks,

greg k-h