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

[Soft Works <softworkz@hotmail.com>]

> -----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
_______________________________________________
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".