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

["Martin Storsjö" <martin@martin.st>]

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

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".