> -----Original Message-----
> From: Martin Storsjö <mar...@martin.st>
> Sent: Tuesday, May 24, 2022 10:59 PM
> To: softworkz <ffmpegag...@gmail.com>
> Cc: ffmpeg-devel@ffmpeg.org; Soft Works <softwo...@hotmail.com>; Hendrik
> Leppkes <h.lepp...@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 <softwo...@hotmail.com>
> >
> > Signed-off-by: softworkz <softwo...@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-requ...@ffmpeg.org with subject "unsubscribe".
