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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]