Re: [FFmpeg-devel] [PATCH 1/2] avutil/wchar_filename, file_open: Support long file names on Windows

[Soft Works <softworkz@hotmail.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of nil-
> admirari@mailo.com
> Sent: Sunday, May 15, 2022 9:03 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avutil/wchar_filename,
> file_open: Support long file names on Windows
> 
> > diff --git a/libavutil/wchar_filename.h b/libavutil/wchar_filename.h
> > ...
> > +static inline int path_is_extended(const wchar_t *path)
> > ...
> 
> Why path handling functions ended up in wchar_filename.h?
> Isn't it better to move them to file_open or os_support?

The functions are needed in both. file_open.c cannot be included
in libavformat/os_support.h and neither the other way round, 
so they need to be in a 3rd place. How about renaming
wchar_filename.h to windows_filename.h ?


> > +    num_chars = GetFullPathNameW(*ppath_w, num_chars, temp_w,
> NULL);
> 
> Turns out that GetFullPathNameW handles long path names without the
> manifest
> or a prefix \\?\. Other WinAPI functions, require either a prefix or a
> manifest,
> which is why I thought that path normalisation must be done by hand.

Yea, that's where we are lucky.


> > +static inline int path_normalize(wchar_t **ppath_w)
> > +{
> > +    int ret;
> > +
> > +    // see .NET6: PathHelper.Normalize()
> > +    if ((ret = get_full_path_name(ppath_w)) < 0)
> > +        return ret;
> > +
> > +    /* What .NET does at this point is to call
> PathHelper.TryExpandShortFileName()
> > +       in case the path contains a '~' character.
> > +       We don't need to do this as we don't need to normalize the
> file name
> > +       for presentation, and the extended path prefix works with
> 8.3 path
> > +       components as well */
> > +    return 0;
> > +}
> 
> This function simply forwards the return code of get_full_path_name().
> The only non-trivial part of it is a comment.

I wanted those functions to resemble the handling path from
.NET. 

I had already started the implementation of TryExpandShortFilePath()
as well, but then I figured that this isn't necessary. For this,
I had tested a long path in the form with 8.3 path components (where
even the 8.3 form is longer than 260) and even those paths work 
with the extended prefix. That's why I skipped this part as in our
case it will only be used internally while in .NET it is done
because the GetFullPathMethod() is also used for other purposes
where expansion is desirable.

Of course we could merge some of those functions together, but the
compiler will do the inlining anyway, that's why I chose to keep
the functions separate for better clarity.

 
> > +static inline int path_is_extended(const wchar_t *path)
> > +{
> > +    // see .NET6: PathInternal.IsExtended()
> > +    size_t len = wcslen(path);
> > +    if (len >= 4  && path[0] == L'\\' && (path[1] == L'\\' ||
> path[1] == L'?') && path[2] == L'?' && path[3] == L'\\')
> > +        return 1;
> > +
> > +    return 0;
> > +}
> >
> > +static inline int add_extended_prefix(wchar_t **ppath_w)
> > +{
> > +    const wchar_t *unc_prefix           = L"\\\\?\\UNC\\";
> > ...
> > +    // see .NET6: PathInternal.EnsureExtendedPrefix()
> > +    if (path_w[0] == L'\\' && path_w[1] == L'\\') {
> > ...
> > +        wcscpy(temp_w, unc_prefix);
> > +        wcscat(temp_w, path_w + 2);
> >
> > +static inline int get_extended_win32_path(const char *path, wchar_t
> **ppath_w)
> > +{
> > ...
> > +    if (path_is_extended(*ppath_w)) {
> > +        ...
> > +        return 0;
> > +    }
> > ...
> > +        if ((ret = add_extended_prefix(ppath_w)) < 0)
> 
> Actual PathInternal.EnsureExtendedPrefix
> (https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/
> System/IO/PathInternal.Windows.cs)
> checks for
> 
>             if (IsPartiallyQualified(path.AsSpan()) ||
> IsDevice(path.AsSpan()))
>                 return path;


> where IsDevice handles \\.\, which you do not handle. If I'm not
> mistaken,
> the code paths presented above will turn such paths into \\?\UNC\\.\,
> which is an error.

I have skipped those checks because we won't have partially qualified
paths at this point (due to having called GetFullPathNameW) and
device paths are not allowed to be longer than 260, so this it might
happen that the UNC prefix gets added, but only when it's a long
path which doesn't work anyway (I've tested those cases).

> > +static inline int add_extended_prefix(wchar_t **pp
ath_w)
> > +{
> > +    const wchar_t *unc_prefix           = L"\\\\?\\UNC\\";
> > ...
> > +        temp_w = (wchar_t *)av_calloc(len + 6 + 1,
> sizeof(wchar_t));
> 
> Wouldn't it be better to use sizeof unc_prefix instead of magic
> numbers?

Then we would need to subtract the terminating zeros and divide
by two => ugly.
Or do another wcslen() call => unnecessary. 

I have added comments right above now, explaining those numbers.

Thanks for your review,
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".