| Message ID | 6f8d400db74be1d704b4c98e2f1295803829ce75.1653400709.git.ffmpegagent@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support long file names on Windows | expand |
| 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 |
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
> -----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
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 --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; }