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

[Krzysztof Opasiak <k.opasiak@samsung.com>]

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
>   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>
> ---
> 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 <k.opasiak@samsung.com>

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