You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/07/31 19:23:53 UTC
[GitHub] [incubator-nuttx] johannes-nivus opened a new pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
johannes-nivus opened a new pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499
Had been PR #1478, can't reopen, perhaps because of force push.
## Summary
New CONFIG_FAT_LFN_UTF8: UTF8 strings are converted to UCS2-LFN
Bugfix in fat_createalias: space is now also converted to underbar.
Change (bugfix) in fat_getlfname: init characters (0xff) and '\0' are rewound as well.
## Impact
When activating CONFIG_FAT_LFN_UTF8 the full 3byte UTF8 space is converted to UCS2 for long filenames, which allows e.g. for chinese characters in folder and file names.
The bug in createalias caused problems with names including space characters.
## Testing
Tested with mkdir on nsh and with apps/testing/fatutf8 (seperate commit on nuttx-apps).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] johannes-nivus commented on pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499#issuecomment-667541189
> For sure I thought about your proposed changes earlier, but this would force activating CONFIG_LIBC_WCHAR for a non exposed interface.
I thought about it once again, and I could include only the two needed functions (wcslen and wcsrchr) in the build if it's ok to include libc functions by control of CONFIG_FAT_LFN_UTF8 (a little ugly, isn't it?). But if I'm correct, I'd have to write the wcsrchr anyway because it isn't yet implemented.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] johannes-nivus commented on pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499#issuecomment-667534104
@xiaoxiang781216
For sure I thought about your proposed changes earlier, but this would force activating CONFIG_LIBC_WCHAR for a non exposed interface.
The usage of wide characters, in the case of the PR, is limited to only one file (fs_fat32dirent.c) and is not visible to the user.
Also I didn't want to override the multibyte to wchar (and vice versa) functions because this affects more aspects than only FAT filesystem, I didn't want to force users to use UTF8 all over the OS.
A clean approach would be adding locales with codepages or UTF8, which then control the behaviour multibyte to wchar functions.
As soon as there is locale support (for UTF8 as well as single byte codepages) I would be happy to fix and clean my current approach, but if there's consensus to use wchar lib and rewrite/add the multibyte to wchar functions already now, I'll do it as well.
Regards, Johannes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] johannes-nivus edited a comment on pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499#issuecomment-667541189
> For sure I thought about your proposed changes earlier, but this would force activating CONFIG_LIBC_WCHAR for a non exposed interface.
I thought about it once again, and I could include only the two needed functions (wcslen and wcsrchr) in the build if it's ok to include libc functions by control of CONFIG_FAT_LFN_UTF8 (a little ugly, isn't it?). But if I'm correct, I'd have to write the wcsrchr anyway because it isn't yet implemented.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499#issuecomment-667546752
> @xiaoxiang781216
> For sure I thought about your proposed changes earlier, but this would force activating CONFIG_LIBC_WCHAR for a non exposed interface.
First, I think CONFIG_LIBC_WCHAR is redundant and can be removed because the linker is smart enough to bring in functions which are really called.
> The usage of wide characters, in the case of the PR, is limited to only one file (fs_fat32dirent.c) and is not visible to the user.
>
wide char is a very useful feature, many user can get benefit if the implement is moved into libc.
> Also I didn't want to override the multibyte to wchar (and vice versa) functions because this affects more aspects than only FAT filesystem, I didn't want to force users to use UTF8 all over the OS.
> A clean approach would be adding locales with codepages or UTF8, which then control the behaviour multibyte to wchar functions.
>
> As soon as there is locale support (for UTF8 as well as single byte codepages) I would be happy to fix and clean my current approach, but if there's consensus to use wchar lib and rewrite/add the multibyte to wchar functions already now, I'll do it as well.
>
The codepage is a legacy technology. It may enough to only support ANSI, UTF8, UTF16 and UTF32.
> > For sure I thought about your proposed changes earlier, but this would force activating CONFIG_LIBC_WCHAR for a non exposed interface.
>
> I thought about it once again, and I could include only the two needed functions (wcslen and wcsrchr) in the build if it's ok to include libc functions by control of CONFIG_FAT_LFN_UTF8 (a little ugly, isn't it?)
CONFIG_LIBC_WCHAR make the configuration complex but don't give us any benefit. we can remove it.
>. But if I'm correct, I'd have to write the wcsrchr anyway because it isn't yet implemented.
Yes, it's a big work to implement all wide char functions, but we can do it step by step.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499#discussion_r463945462
##########
File path: fs/fat/fs_fat32dirent.c
##########
@@ -2071,7 +2278,11 @@ static int fat_putlfname(FAR struct fat_mountpt_s *fs,
* LDIR_MAXLFNCHARS (13).
*/
- namelen = strlen((FAR char *)dirinfo->fd_lfname);
+ for (namelen = 0; dirinfo->fd_lfname[namelen] != '\0'; namelen++)
Review comment:
Why not process like lnfchar?
```
#ifdef CONFIG_FAT_LFN_UTF8
#define lnflen(s) wcslen(s)
#else
#define lnflen(s) strlen(s)
#endif
##########
File path: fs/fat/fs_fat32dirent.c
##########
@@ -564,19 +668,25 @@ static inline int fat_parselfname(FAR const char **path,
#ifdef CONFIG_FAT_LFN
static inline int fat_createalias(FAR struct fat_dirinfo_s *dirinfo)
{
- uint8_t ch; /* Current character being processed */
- char *ext; /* Pointer to the extension substring */
- char *src; /* Pointer to the long file name source */
- int len; /* Total length of the long file name */
- int namechars; /* Number of characters available in long name */
- int extchars; /* Number of characters available in long name extension */
- int endndx; /* Maximum index into the short name array */
- int ndx; /* Index to store next character */
+ uint8_t ch; /* Current character being processed */
+ lfnchar *ext; /* Pointer to the extension substring */
+ lfnchar *src; /* Pointer to the long file name source */
+ int len; /* Total length of the long file name */
+ int namechars; /* Number of characters available in long name */
+ int extchars; /* Number of characters available in long name extension */
+ int endndx; /* Maximum index into the short name array */
+ int ndx; /* Index to store next character */
/* First, let's decide what is name and what is extension */
- len = strlen((FAR char *)dirinfo->fd_lfname);
- ext = strrchr((FAR char *)dirinfo->fd_lfname, '.');
+ for (len = 0, ext = NULL; dirinfo->fd_lfname[len] != '\0'; len++)
Review comment:
#ifdef CONFIG_FAT_LFN_UTF8
#define lnfchr(s,c) wcschr(s,c)
#else
#define lnflen(s,c) strchr(s,c)
#endif
Instead the manual loop
##########
File path: fs/fat/fs_fat32dirent.c
##########
@@ -143,10 +143,102 @@ static int fat_putsfdirentry(FAR struct fat_mountpt_s *fs,
FAR struct fat_dirinfo_s *dirinfo,
uint8_t attributes, uint32_t fattime);
+#if defined(CONFIG_FAT_LFN) && defined(CONFIG_FAT_LFN_UTF8)
+static int fat_utf8toucs(FAR const char **str, lfnchar *ucs);
+static int fat_ucstoutf8(FAR uint8_t *dest, uint8_t offset, lfnchar ucs);
Review comment:
it's better to move the implementation to mbstowcs and wcsrtombs. and call mbstowcs/wcsrtombs instead. Then the conversion can be reused.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499#issuecomment-667551330
@johannes-nivus https://github.com/apache/incubator-nuttx/pull/1500 remove CONFIG_LIBC_WCHAR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #1499: FAT Filesystem: UTF8 support for long filenames, bugfixes.
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #1499:
URL: https://github.com/apache/incubator-nuttx/pull/1499
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org