diff mbox series

[FFmpeg-devel,v6,2/2] avformat/os_support: Support long file names on Windows

Message ID 6f8d400db74be1d704b4c98e2f1295803829ce75.1653400709.git.ffmpegagent@gmail.com
State New
Headers show
Series Support long file names on Windows | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

softworkz May 24, 2022, 1:58 p.m. UTC
From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavformat/os_support.h | 87 +++++++++++++++++++++++++++++-----------
 1 file changed, 63 insertions(+), 24 deletions(-)

Comments

Martin Storsjö May 24, 2022, 8:58 p.m. UTC | #1
On Tue, 24 May 2022, softworkz wrote:

> From: softworkz <softworkz@hotmail.com>
>
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
> libavformat/os_support.h | 87 +++++++++++++++++++++++++++++-----------
> 1 file changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/libavformat/os_support.h b/libavformat/os_support.h
> index 5e6b32d2dc..179b926293 100644
> --- a/libavformat/os_support.h
> +++ b/libavformat/os_support.h
> @@ -49,7 +49,24 @@
> #  ifdef stat
> #   undef stat
> #  endif
> -#  define stat _stati64
> +
> +#  define stat win32_stat
> +
> +    struct win32_stat
> +    {
> +        _dev_t         st_dev;     /* ID of device containing file */
> +        _ino_t         st_ino;     /* inode number */
> +        unsigned short st_mode;    /* protection */
> +        short          st_nlink;   /* number of hard links */
> +        short          st_uid;     /* user ID of owner */
> +        short          st_gid;     /* group ID of owner */
> +        _dev_t         st_rdev;    /* device ID (if special file) */
> +        long           st_size;    /* total size, in bytes */
> +        time_t         st_atime;   /* time of last access */
> +        time_t         st_mtime;   /* time of last modification */
> +        time_t         st_ctime;   /* time of last status change */
> +    };

Please use int64_t for both st_size and st_?time. We already use _stati64 
so far, so we get 64 bit sizes (and long definitely isn't a 64 bit type on 
Windows!), and with int64_t in the outer struct, we won't accidentally 
truncate any valid data that we got from the lower level stat function 
call.



> +
> #  ifdef fstat
> #   undef fstat
> #  endif
> @@ -153,7 +170,7 @@ static inline int win32_##name(const char *filename_utf8) \
>     wchar_t *filename_w;                                  \
>     int ret;                                              \
>                                                           \
> -    if (utf8towchar(filename_utf8, &filename_w))          \
> +    if (get_extended_win32_path(filename_utf8, &filename_w)) \
>         return -1;                                        \
>     if (!filename_w)                                      \
>         goto fallback;                                    \
> @@ -171,37 +188,59 @@ DEF_FS_FUNCTION(unlink, _wunlink, _unlink)
> DEF_FS_FUNCTION(mkdir,  _wmkdir,  _mkdir)
> DEF_FS_FUNCTION(rmdir,  _wrmdir , _rmdir)
>
> -#define DEF_FS_FUNCTION2(name, wfunc, afunc, partype)     \
> -static inline int win32_##name(const char *filename_utf8, partype par) \
> -{                                                         \
> -    wchar_t *filename_w;                                  \
> -    int ret;                                              \
> -                                                          \
> -    if (utf8towchar(filename_utf8, &filename_w))          \
> -        return -1;                                        \
> -    if (!filename_w)                                      \
> -        goto fallback;                                    \
> -                                                          \
> -    ret = wfunc(filename_w, par);                         \
> -    av_free(filename_w);                                  \
> -    return ret;                                           \
> -                                                          \
> -fallback:                                                 \
> -    /* filename may be be in CP_ACP */                    \
> -    return afunc(filename_utf8, par);                     \
> +static inline int win32_access(const char *filename_utf8, int par)
> +{
> +    wchar_t *filename_w;
> +    int ret;
> +    if (get_extended_win32_path(filename_utf8, &filename_w))
> +        return -1;
> +    if (!filename_w)
> +        goto fallback;
> +    ret = _waccess(filename_w, par);
> +    av_free(filename_w);
> +    return ret;
> +fallback:
> +    return _access(filename_utf8, par);
> }
>
> -DEF_FS_FUNCTION2(access, _waccess, _access, int)
> -DEF_FS_FUNCTION2(stat, _wstati64, _stati64, struct stat*)
> +static inline int win32_stat(const char *filename_utf8, struct stat *par)
> +{

Maybe "struct win32_stat" in the parameter here too, for consistency?

> +    wchar_t *filename_w;
> +    int ret;
> +    struct _stati64 winstat = { 0 };
> +
> +    if (get_extended_win32_path(filename_utf8, &filename_w))
> +        return -1;
> +
> +    if (filename_w) {
> +        ret = _wstat64(filename_w, &winstat);
> +        av_free(filename_w);
> +    } else
> +        ret = _stat64(filename_utf8, &winstat);
> +
> +    par->st_dev   = winstat.st_dev;
> +    par->st_ino   = winstat.st_ino;
> +    par->st_mode  = winstat.st_mode;
> +    par->st_nlink = winstat.st_nlink;
> +    par->st_uid   = winstat.st_uid;
> +    par->st_gid   = winstat.st_gid;
> +    par->st_rdev  = winstat.st_rdev;
> +    par->st_size  = winstat.st_size;
> +    par->st_atime = winstat.st_atime;
> +    par->st_mtime = winstat.st_mtime;
> +    par->st_ctime = winstat.st_ctime;

Thanks, this approach seems robust and safe to me!

With this change in place, shouldn't we drop the #ifdef for 
stat/win32_stat in file.c at the same time?

Other than that, this starts to look ok now.

// Martin
Soft Works May 24, 2022, 10:12 p.m. UTC | #2
> -----Original Message-----
> From: Martin Storsjö <martin@martin.st>
> Sent: Tuesday, May 24, 2022 10:59 PM
> To: softworkz <ffmpegagent@gmail.com>
> Cc: ffmpeg-devel@ffmpeg.org; Soft Works <softworkz@hotmail.com>; Hendrik
> Leppkes <h.leppkes@gmail.com>
> Subject: Re: [PATCH v6 2/2] avformat/os_support: Support long file names
> on Windows
> 
> On Tue, 24 May 2022, softworkz wrote:
> 
> > From: softworkz <softworkz@hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> > libavformat/os_support.h | 87 +++++++++++++++++++++++++++++-----------
> > 1 file changed, 63 insertions(+), 24 deletions(-)
> >
> > diff --git a/libavformat/os_support.h b/libavformat/os_support.h
> > index 5e6b32d2dc..179b926293 100644
> > --- a/libavformat/os_support.h
> > +++ b/libavformat/os_support.h
> > @@ -49,7 +49,24 @@
> > #  ifdef stat
> > #   undef stat
> > #  endif
> > -#  define stat _stati64
> > +
> > +#  define stat win32_stat
> > +
> > +    struct win32_stat
> > +    {
> > +        _dev_t         st_dev;     /* ID of device containing file */
> > +        _ino_t         st_ino;     /* inode number */
> > +        unsigned short st_mode;    /* protection */
> > +        short          st_nlink;   /* number of hard links */
> > +        short          st_uid;     /* user ID of owner */
> > +        short          st_gid;     /* group ID of owner */
> > +        _dev_t         st_rdev;    /* device ID (if special file) */
> > +        long           st_size;    /* total size, in bytes */
> > +        time_t         st_atime;   /* time of last access */
> > +        time_t         st_mtime;   /* time of last modification */
> > +        time_t         st_ctime;   /* time of last status change */
> > +    };
> 
> Please use int64_t for both st_size and st_?time. We already use _stati64
> so far, so we get 64 bit sizes (and long definitely isn't a 64 bit type on
> Windows!), and with int64_t in the outer struct, we won't accidentally
> truncate any valid data that we got from the lower level stat function
> call.

I came to long by looking up _off_t in the Windows headers, but you are 
right: as we're explicitly using _stat64, we'll get int64 st_size values,
even on 32bit Windows.

Done.

> > +
> > #  ifdef fstat
> > #   undef fstat
> > #  endif
> > @@ -153,7 +170,7 @@ static inline int win32_##name(const char
> *filename_utf8) \
> >     wchar_t *filename_w;                                  \
> >     int ret;                                              \
> >                                                           \
> > -    if (utf8towchar(filename_utf8, &filename_w))          \
> > +    if (get_extended_win32_path(filename_utf8, &filename_w)) \
> >         return -1;                                        \
> >     if (!filename_w)                                      \
> >         goto fallback;                                    \
> > @@ -171,37 +188,59 @@ DEF_FS_FUNCTION(unlink, _wunlink, _unlink)
> > DEF_FS_FUNCTION(mkdir,  _wmkdir,  _mkdir)
> > DEF_FS_FUNCTION(rmdir,  _wrmdir , _rmdir)
> >
> > -#define DEF_FS_FUNCTION2(name, wfunc, afunc, partype)     \
> > -static inline int win32_##name(const char *filename_utf8, partype par)
> \
> > -{                                                         \
> > -    wchar_t *filename_w;                                  \
> > -    int ret;                                              \
> > -                                                          \
> > -    if (utf8towchar(filename_utf8, &filename_w))          \
> > -        return -1;                                        \
> > -    if (!filename_w)                                      \
> > -        goto fallback;                                    \
> > -                                                          \
> > -    ret = wfunc(filename_w, par);                         \
> > -    av_free(filename_w);                                  \
> > -    return ret;                                           \
> > -                                                          \
> > -fallback:                                                 \
> > -    /* filename may be be in CP_ACP */                    \
> > -    return afunc(filename_utf8, par);                     \
> > +static inline int win32_access(const char *filename_utf8, int par)
> > +{
> > +    wchar_t *filename_w;
> > +    int ret;
> > +    if (get_extended_win32_path(filename_utf8, &filename_w))
> > +        return -1;
> > +    if (!filename_w)
> > +        goto fallback;
> > +    ret = _waccess(filename_w, par);
> > +    av_free(filename_w);
> > +    return ret;
> > +fallback:
> > +    return _access(filename_utf8, par);
> > }
> >
> > -DEF_FS_FUNCTION2(access, _waccess, _access, int)
> > -DEF_FS_FUNCTION2(stat, _wstati64, _stati64, struct stat*)
> > +static inline int win32_stat(const char *filename_utf8, struct stat
> *par)
> > +{
> 
> Maybe "struct win32_stat" in the parameter here too, for consistency?

Yup. (it didn't work in an earlier iteration, but now it does)


> > +    wchar_t *filename_w;
> > +    int ret;
> > +    struct _stati64 winstat = { 0 };
> > +
> > +    if (get_extended_win32_path(filename_utf8, &filename_w))
> > +        return -1;
> > +
> > +    if (filename_w) {
> > +        ret = _wstat64(filename_w, &winstat);
> > +        av_free(filename_w);
> > +    } else
> > +        ret = _stat64(filename_utf8, &winstat);
> > +
> > +    par->st_dev   = winstat.st_dev;
> > +    par->st_ino   = winstat.st_ino;
> > +    par->st_mode  = winstat.st_mode;
> > +    par->st_nlink = winstat.st_nlink;
> > +    par->st_uid   = winstat.st_uid;
> > +    par->st_gid   = winstat.st_gid;
> > +    par->st_rdev  = winstat.st_rdev;
> > +    par->st_size  = winstat.st_size;
> > +    par->st_atime = winstat.st_atime;
> > +    par->st_mtime = winstat.st_mtime;
> > +    par->st_ctime = winstat.st_ctime;
> 
> Thanks, this approach seems robust and safe to me!
> 
> With this change in place, shouldn't we drop the #ifdef for
> stat/win32_stat in file.c at the same time?

Done. While doing that, I realized that fstat needs
to be remapped as well, otherwise _ftati64 would be
called with the win32_stat structure. Done that 
as well.

> Other than that, this starts to look ok now.

Thanks for your great help, much appreciated!

softworkz
Martin Storsjö May 25, 2022, 7:09 a.m. UTC | #3
On Tue, 24 May 2022, Soft Works wrote:

>> -----Original Message-----
>> From: Martin Storsjö <martin@martin.st>
>> Sent: Tuesday, May 24, 2022 10:59 PM
>> To: softworkz <ffmpegagent@gmail.com>
>> Cc: ffmpeg-devel@ffmpeg.org; Soft Works <softworkz@hotmail.com>; Hendrik
>> Leppkes <h.leppkes@gmail.com>
>> Subject: Re: [PATCH v6 2/2] avformat/os_support: Support long file names
>> on Windows
>>
>>> +    wchar_t *filename_w;
>>> +    int ret;
>>> +    struct _stati64 winstat = { 0 };
>>> +
>>> +    if (get_extended_win32_path(filename_utf8, &filename_w))
>>> +        return -1;
>>> +
>>> +    if (filename_w) {
>>> +        ret = _wstat64(filename_w, &winstat);
>>> +        av_free(filename_w);
>>> +    } else
>>> +        ret = _stat64(filename_utf8, &winstat);
>>> +
>>> +    par->st_dev   = winstat.st_dev;
>>> +    par->st_ino   = winstat.st_ino;
>>> +    par->st_mode  = winstat.st_mode;
>>> +    par->st_nlink = winstat.st_nlink;
>>> +    par->st_uid   = winstat.st_uid;
>>> +    par->st_gid   = winstat.st_gid;
>>> +    par->st_rdev  = winstat.st_rdev;
>>> +    par->st_size  = winstat.st_size;
>>> +    par->st_atime = winstat.st_atime;
>>> +    par->st_mtime = winstat.st_mtime;
>>> +    par->st_ctime = winstat.st_ctime;
>>
>> Thanks, this approach seems robust and safe to me!
>>
>> With this change in place, shouldn't we drop the #ifdef for
>> stat/win32_stat in file.c at the same time?
>
> Done. While doing that, I realized that fstat needs
> to be remapped as well, otherwise _ftati64 would be
> called with the win32_stat structure. Done that
> as well.

Good - I also just realized the same while grepping around for "struct 
stat".

// Martin
diff mbox series

Patch

diff --git a/libavformat/os_support.h b/libavformat/os_support.h
index 5e6b32d2dc..179b926293 100644
--- a/libavformat/os_support.h
+++ b/libavformat/os_support.h
@@ -49,7 +49,24 @@ 
 #  ifdef stat
 #   undef stat
 #  endif
-#  define stat _stati64
+
+#  define stat win32_stat
+
+    struct win32_stat
+    {
+        _dev_t         st_dev;     /* ID of device containing file */
+        _ino_t         st_ino;     /* inode number */
+        unsigned short st_mode;    /* protection */
+        short          st_nlink;   /* number of hard links */
+        short          st_uid;     /* user ID of owner */
+        short          st_gid;     /* group ID of owner */
+        _dev_t         st_rdev;    /* device ID (if special file) */
+        long           st_size;    /* total size, in bytes */
+        time_t         st_atime;   /* time of last access */
+        time_t         st_mtime;   /* time of last modification */
+        time_t         st_ctime;   /* time of last status change */
+    };
+
 #  ifdef fstat
 #   undef fstat
 #  endif
@@ -153,7 +170,7 @@  static inline int win32_##name(const char *filename_utf8) \
     wchar_t *filename_w;                                  \
     int ret;                                              \
                                                           \
-    if (utf8towchar(filename_utf8, &filename_w))          \
+    if (get_extended_win32_path(filename_utf8, &filename_w)) \
         return -1;                                        \
     if (!filename_w)                                      \
         goto fallback;                                    \
@@ -171,37 +188,59 @@  DEF_FS_FUNCTION(unlink, _wunlink, _unlink)
 DEF_FS_FUNCTION(mkdir,  _wmkdir,  _mkdir)
 DEF_FS_FUNCTION(rmdir,  _wrmdir , _rmdir)
 
-#define DEF_FS_FUNCTION2(name, wfunc, afunc, partype)     \
-static inline int win32_##name(const char *filename_utf8, partype par) \
-{                                                         \
-    wchar_t *filename_w;                                  \
-    int ret;                                              \
-                                                          \
-    if (utf8towchar(filename_utf8, &filename_w))          \
-        return -1;                                        \
-    if (!filename_w)                                      \
-        goto fallback;                                    \
-                                                          \
-    ret = wfunc(filename_w, par);                         \
-    av_free(filename_w);                                  \
-    return ret;                                           \
-                                                          \
-fallback:                                                 \
-    /* filename may be be in CP_ACP */                    \
-    return afunc(filename_utf8, par);                     \
+static inline int win32_access(const char *filename_utf8, int par)
+{
+    wchar_t *filename_w;
+    int ret;
+    if (get_extended_win32_path(filename_utf8, &filename_w))
+        return -1;
+    if (!filename_w)
+        goto fallback;
+    ret = _waccess(filename_w, par);
+    av_free(filename_w);
+    return ret;
+fallback:
+    return _access(filename_utf8, par);
 }
 
-DEF_FS_FUNCTION2(access, _waccess, _access, int)
-DEF_FS_FUNCTION2(stat, _wstati64, _stati64, struct stat*)
+static inline int win32_stat(const char *filename_utf8, struct stat *par)
+{
+    wchar_t *filename_w;
+    int ret;
+    struct _stati64 winstat = { 0 };
+
+    if (get_extended_win32_path(filename_utf8, &filename_w))
+        return -1;
+
+    if (filename_w) {
+        ret = _wstat64(filename_w, &winstat);
+        av_free(filename_w);
+    } else
+        ret = _stat64(filename_utf8, &winstat);
+
+    par->st_dev   = winstat.st_dev;
+    par->st_ino   = winstat.st_ino;
+    par->st_mode  = winstat.st_mode;
+    par->st_nlink = winstat.st_nlink;
+    par->st_uid   = winstat.st_uid;
+    par->st_gid   = winstat.st_gid;
+    par->st_rdev  = winstat.st_rdev;
+    par->st_size  = winstat.st_size;
+    par->st_atime = winstat.st_atime;
+    par->st_mtime = winstat.st_mtime;
+    par->st_ctime = winstat.st_ctime;
+
+    return ret;
+}
 
 static inline int win32_rename(const char *src_utf8, const char *dest_utf8)
 {
     wchar_t *src_w, *dest_w;
     int ret;
 
-    if (utf8towchar(src_utf8, &src_w))
+    if (get_extended_win32_path(src_utf8, &src_w))
         return -1;
-    if (utf8towchar(dest_utf8, &dest_w)) {
+    if (get_extended_win32_path(dest_utf8, &dest_w)) {
         av_free(src_w);
         return -1;
     }