Skip to content

/ curl

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

file: Support unicode urls on windows #6501

Closed
wants to merge 1 commit into from
Closed

file: Support unicode urls on windows #6501

wants to merge 1 commit into from

Conversation

@foopoiuyt
Copy link
Contributor

@foopoiuyt foopoiuyt commented Jan 21, 2021

Support file: urls with utf-8 filenames on Windows when _UNICODE is defined.
@jay jay added the Windows label Jan 21, 2021
@jay
Copy link
Member

@jay jay commented Jan 21, 2021

https://dev.azure.com/daniel0244/5571c33c-81e1-43d7-93ee-d3d4152f27b2/_apis/build/builds/4396/logs/99

2021-01-21T00:23:08.4699191Z file.c: In function 'file_upload':
2021-01-21T00:23:08.4700646Z file.c:280:55: error: macro "open" passed 3 arguments, but takes just 2
2021-01-21T00:23:08.4701344Z    fd = open(file->path, mode, data->set.new_file_perms);
2021-01-21T00:23:08.4701886Z                                                        ^
2021-01-21T00:23:08.4704068Z file.c:280:6: error: assignment to 'int' from 'int (__attribute__((cdecl)) *)(const char *, int,  ...)' {aka 'int (*)(const char *, int,  ...)'} makes integer from pointer without a cast [-Wint-conversion]
2021-01-21T00:23:08.4705100Z    fd = open(file->path, mode, data->set.new_file_perms);
2021-01-21T00:23:08.4705770Z       ^

That is because open takes 2 or 3 arguments. To fix it you could do # define open curlx_win32_open and then make curlx_win32_open variadic. The alternative is a variadic macro but I don't think that's supported with all compilers.
@emilengler
emilengler suggested changes Jan 21, 2021
View changes
lib/curl_setup.h Outdated
@@ -356,9 +358,11 @@
# define fstat(fdes,stp) _fstat(fdes, stp)
# define stat(fname,stp) curlx_win32_stat(fname, stp)
# define struct_stat struct _stat
# define open(fname,oflag) curlx_win32_open(fname, oflag)

This comment has been minimized.

Sign in to view
@emilengler

emilengler Jan 21, 2021
Contributor

Please don't redfine a POSIX syscall
lib/curl_multibyte.c Outdated
return result;
#endif

return (open)(filename, oflag);

This comment has been minimized.

Sign in to view
@emilengler

emilengler Jan 21, 2021
Contributor

Why even define the function on non Windows platforms?

This comment has been minimized.

Sign in to view
@MarcelRaad

MarcelRaad Jan 21, 2021
Member

It's not, this is in a #if defined(USE_WIN32_LARGE_FILES) || defined(USE_WIN32_SMALL_FILES) block.
@MarcelRaad
MarcelRaad reviewed Jan 21, 2021
View changes
lib/curl_multibyte.c Outdated
int curlx_win32_open(const char *filename, int oflag)
{
#ifdef _UNICODE
int result = 0;

This comment has been minimized.

Sign in to view
@MarcelRaad

MarcelRaad Jan 21, 2021
Member

The error value is defined as -1 and 0 might be a valid result: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen?view=msvc-160#return-value

This comment has been minimized.

Sign in to view
@foopoiuyt

foopoiuyt Jan 21, 2021
Author Contributor

Thanks, totally missed it (and the later check as mentioned below).
@foopoiuyt foopoiuyt force-pushed the foopoiuyt:windows_unicode_file branch 3 times, most recently from 63585f8 to 1106ff3 Jan 21, 2021
@MarcelRaad
MarcelRaad reviewed Jan 21, 2021
View changes
lib/curl_multibyte.c Outdated
@@ -82,6 +82,29 @@ char *curlx_convert_wchar_to_UTF8(const wchar_t *str_w)

#if defined(USE_WIN32_LARGE_FILES) || defined(USE_WIN32_SMALL_FILES)

int curlx_win32_open(const char *filename, int oflag, ...)
{
int pmode = 0;

This comment has been minimized.

Sign in to view
@MarcelRaad

MarcelRaad Jan 21, 2021
Member

Indentation is wrong here.
@MarcelRaad
MarcelRaad reviewed Jan 21, 2021
View changes
lib/curl_multibyte.c Outdated
if(filename_w)
result = _wopen(filename_w, oflag, pmode);
free(filename_w);
if(result)

This comment has been minimized.

Sign in to view
@MarcelRaad

MarcelRaad Jan 21, 2021
Member

This should be if(result != -1) now.
@foopoiuyt foopoiuyt force-pushed the foopoiuyt:windows_unicode_file branch from 1106ff3 to 6dc311a Jan 21, 2021
@jay
jay reviewed Jan 22, 2021
View changes
lib/curl_setup.h Outdated
@@ -356,9 +359,12 @@
# define fstat(fdes,stp) _fstat(fdes, stp)
# define stat(fname,stp) curlx_win32_stat(fname, stp)
# define struct_stat struct _stat
# define open(fname,oflag,...) curlx_win32_open(fname, oflag \
, ## __VA_ARGS__)

This comment has been minimized.

Sign in to view
@jay

jay Jan 22, 2021 •
edited
Member

I would do away with __VA_ARGS__ I don't think all compilers support it
@foopoiuyt foopoiuyt force-pushed the foopoiuyt:windows_unicode_file branch 5 times, most recently from 9a9a4fc to cf951b5 Jan 22, 2021
@jay jay added the next-feature-window label Jan 24, 2021
@jay
jay approved these changes Jan 24, 2021
View changes
@jay jay changed the title WIP: file: Support unicode urls on windows file: Support unicode urls on windows Jan 24, 2021
@nardl
nardl approved these changes Jan 24, 2021
View changes
@MarcelRaad
MarcelRaad approved these changes Jan 25, 2021
View changes
@foopoiuyt @jay
file: Support unicode urls on windows
Loading status checks…
2f71f51
@jay jay force-pushed the foopoiuyt:windows_unicode_file branch from cf951b5 to 2f71f51 Feb 3, 2021
@jay jay closed this in 1269c80 Feb 9, 2021
@jay jay removed the next-feature-window label Feb 9, 2021
@jay
Copy link
Member

@jay jay commented Feb 9, 2021

Thanks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilengler emilengler

@jay jay

@MarcelRaad MarcelRaad

@nardl nardl

Assignees
No one assigned
Labels
Windows
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@foopoiuyt @jay @MarcelRaad @emilengler @nardl