Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571243 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner <stefan@agner.ch> - 2017-02-01 03:20 +0100
    [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner <stefan@agner.ch> - 2017-02-01 03:20 +0100
      Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line Greg KH <gregkh@linuxfoundation.org> - 2017-02-01 09:10 +0100
        Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line Felipe Balbi <balbi@kernel.org> - 2017-02-01 10:10 +0100
          Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line Stefan Agner <stefan@agner.ch> - 2017-02-01 18:00 +0100
    Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH <gregkh@linuxfoundation.org> - 2017-02-01 09:10 +0100
      Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner <stefan@agner.ch> - 2017-02-01 18:00 +0100
        Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner <stefan@agner.ch> - 2017-02-09 19:10 +0100
          Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH <gregkh@linuxfoundation.org> - 2017-02-10 12:40 +0100
            Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Felipe Balbi <balbi@kernel.org> - 2017-02-10 13:40 +0100
              Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Stefan Agner <stefan@agner.ch> - 2017-02-10 21:40 +0100
                Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Felipe Balbi <balbi@kernel.org> - 2017-02-13 11:50 +0100
    Re: [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric Greg KH <gregkh@linuxfoundation.org> - 2017-02-01 09:10 +0100

#1571243 — [PATCH 1/2] fs: configfs: make qw_sign attribute symmetric

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
  00000000  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 <stefan@agner.ch>
---
 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 78c44979dde3..ea1a40f5b23d 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -786,9 +786,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,
-- 
2.11.0

[toc] | [next] | [standalone]

#1571245 — [PATCH 2/2] fs: configfs: use hexadecimal values and new line

Other unsigned properties return hexadecimal values, follow this
convention when printing b_vendor_code too. Also add newlines to
the OS Descriptor support related properties, like other sysfs
files use.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/usb/gadget/configfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index ea1a40f5b23d..8e9adcfff748 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -737,7 +737,7 @@ static inline struct gadget_info *os_desc_item_to_gadget_info(
 
 static ssize_t os_desc_use_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d",
+	return sprintf(page, "%d\n",
 			os_desc_item_to_gadget_info(item)->use_os_desc);
 }
 
@@ -761,7 +761,7 @@ static ssize_t os_desc_use_store(struct config_item *item, const char *page,
 
 static ssize_t os_desc_b_vendor_code_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d",
+	return sprintf(page, "0x%02x\n",
 			os_desc_item_to_gadget_info(item)->b_vendor_code);
 }
 
@@ -903,7 +903,7 @@ static inline struct usb_os_desc_ext_prop
 
 static ssize_t ext_prop_type_show(struct config_item *item, char *page)
 {
-	return sprintf(page, "%d", to_usb_os_desc_ext_prop(item)->type);
+	return sprintf(page, "%d\n", to_usb_os_desc_ext_prop(item)->type);
 }
 
 static ssize_t ext_prop_type_store(struct config_item *item,
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1571310 — Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

On Tue, Jan 31, 2017 at 06:19:17PM -0800, Stefan Agner wrote:
> Other unsigned properties return hexadecimal values, follow this
> convention when printing b_vendor_code too. Also add newlines to
> the OS Descriptor support related properties, like other sysfs
> files use.

configfs is not sysfs, so watch out, you might not need/want those new
lines as tools read the values and are not expecting them.

Have you verified that nothing will break with this?  How have you
tested it?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1571340 — Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg KH <gregkh@linuxfoundation.org> writes:
> On Tue, Jan 31, 2017 at 06:19:17PM -0800, Stefan Agner wrote:
>> Other unsigned properties return hexadecimal values, follow this
>> convention when printing b_vendor_code too. Also add newlines to
>> the OS Descriptor support related properties, like other sysfs
>> files use.
>
> configfs is not sysfs, so watch out, you might not need/want those new
> lines as tools read the values and are not expecting them.
>
> Have you verified that nothing will break with this?  How have you
> tested it?

I would rather not change things like that, specially since Android guys
are alreadying using configfs.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1571755 — Re: [PATCH 2/2] fs: configfs: use hexadecimal values and new line

On 2017-02-01 01:02, Felipe Balbi wrote:
> Hi,
> 
> Greg KH <gregkh@linuxfoundation.org> writes:
>> On Tue, Jan 31, 2017 at 06:19:17PM -0800, Stefan Agner wrote:
>>> Other unsigned properties return hexadecimal values, follow this
>>> convention when printing b_vendor_code too. Also add newlines to
>>> the OS Descriptor support related properties, like other sysfs
>>> files use.
>>
>> configfs is not sysfs, so watch out, you might not need/want those new
>> lines as tools read the values and are not expecting them.
>>
>> Have you verified that nothing will break with this?  How have you
>> tested it?

I am actually writing OS Descriptor support for libusbgx (fork of
libusbg) when I stumbled upon this. The helper function there do not
care about the new line, but I just thought we should fix it for the
sake of coherency to the other properties. I am also fine dropping this
patch.

> 
> I would rather not change things like that, specially since Android guys
> are alreadying using configfs.

Do you happen to know where the source of that part is? So we could
check if they make use of any OS Descriptor support yet...

--
Stefan

[toc] | [prev] | [next] | [standalone]

#1571312

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
>   00000000  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

[toc] | [prev] | [next] | [standalone]

#1571753

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
>>   00000000  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(&gi->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(&gi->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

[toc] | [prev] | [next] | [standalone]

#1577830

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
>>>   00000000  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(&gi->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(&gi->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

[toc] | [prev] | [next] | [standalone]

#1578411

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
> >>>   00000000  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(&gi->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(&gi->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

[toc] | [prev] | [next] | [standalone]

#1578427

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg KH <gregkh@linuxfoundation.org> 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
>> >>>   00000000  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(&gi->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(&gi->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

[toc] | [prev] | [next] | [standalone]

#1578781

On 2017-02-10 04:30, Felipe Balbi wrote:
> Hi,
> 
> Greg KH <gregkh@linuxfoundation.org> 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
>>> >>>   00000000  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(&gi->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(&gi->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

[toc] | [prev] | [next] | [standalone]

#1579607

[Multipart message — attachments visible in raw view] — view raw

Hi,

Stefan Agner <stefan@agner.ch> 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
>>>> >>>   00000000  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(&gi->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(&gi->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

[toc] | [prev] | [next] | [standalone]

#1571314

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
>   00000000  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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web