file:// : Allow getting basic directory listings by colinleroy · Pull Request #13137 · curl/curl

colinleroy · 2024-03-16T11:42:24Z

Hi Daniel & curl team,

Here's a little PR that allows one to get basic directory listings over file://, like for example:

$ ./src/curl file:///home/colin/Documents/git-oss/curl/
winbuild
configure~
autom4te.cache
MacOSX-Framework
LICENSES
CMake
libtool
config.status
libcurl.pc.in
missing
...

I hope you folks will consider it useful. For me, it's a way to start diving into curl code to implement the same for smb:// (which, of course, seems to be much more complicated).

gvanem · 2024-03-16T12:05:53Z

Useful IMHO, but not all targets have readdir() etc.
So consider adding MSVC-support for this <dirent.h> stuff from e.g.
https://github.com/ThomasDickey/lynx-snapshots/blob/master/lib/dirent.h

into this PR first.

colinleroy · 2024-03-16T13:44:15Z

Useful IMHO, but not all targets have readdir() etc. So consider adding MSVC-support for this <dirent.h> stuff from e.g. https://github.com/ThomasDickey/lynx-snapshots/blob/master/lib/dirent.h

into this PR first.

Thank you for your comment, I'll do that. The linters are being mean to me, I'll handle that too.
Edit: Would you have some pointers about how to do that (like maybe another example of thing added for MSVC)?

I don't have a Windows development machine, VM or setup so I feel there may be a few issues I'll have to sort through.

dfandrich · 2024-03-16T16:08:58Z

Be sure to take a look at PR #9746 and its comments.

bagder · 2024-03-16T17:45:28Z

The linters are being mean to me

make checksrc locally is a good idea...

bagder · 2024-03-16T17:50:21Z

+  *data = NULL;
+  *size = 0;
+
+  if(!file) {


can file actually and legitimately be NULL here? Shouldn't this rather be an assert since it should never happen in real life?

bagder · 2024-03-16T17:53:41Z

+
+  dir = fdopendir(file->fd);
+  if(!dir) {
+    result = CURLE_BAD_FUNCTION_ARGUMENT;


This error is curious, since it already opened the file and it is said to be a dir, but this still fails. It seems to me that CURLE_BAD_FUNCTION_ARGUMENT is not the most accurate description of this error. Maybe just CURLE_READ_ERROR ?

bagder · 2024-03-16T17:56:22Z

+
+      *data = tmp;
+      snprintf(*data + *size, entry_len + 1, "%s\n", entry->d_name);
+      *size += entry_len;


Would it not make sense to just pass on the file entry to the write callback as soon as you have it, rather than to create a (potentially huge) allocated buffer and send them all later?

And if you do prefer the buffer approach, please use a dynbuf to avoid manual reallocs.

I went with a temporary buffer because it seemed much easier to calculate expected_size and do resuming. Do you think that's a good enough reason to go the buffer route?

It does provide an expected size, that is true. But resuming sounds super fragile and will break horribly if something in the directory changes in between...

Hmm this is true. Is it acceptable to reject resumes in case of a directory listing?

I think that's the only sensible thing to do

colinleroy · 2024-03-18T16:51:16Z

I repushed to address your comments. Is this something like that you had in mind?

bagder · 2024-03-19T07:07:41Z

+        goto out;
+    }
+    else {
+      DIR *dir = fdopendir(file->fd);


You need to add a check for fdopendir() to configure and CMake so that we know it exists for the platform you build this for. On Windows it won't exist. For now at least, I think we can presume the other POSIX dir funtions exist if this one does.

Then you can either opt to fail on other platforms, or continue to add support other dir-reading functions. Windows being a primary target I guess.

macOS also needs attention as the compiler there says:

'fdopendir' has been marked as being introduced in macOS 10.10 here, but the deployment target is macOS 10.9.0

Hi @bagder,
Thanks for your feedback. I will add the configure/CMake checks this evening, and probably switch to opendir(file->path) to address the Mac OS X case.
For Windows, I'll see if I can find someone with Windows experience to help me either add wrappers or #ifdefs.

bagder · 2024-03-19T08:59:22Z

I googled the Windows API for this and it seems fairly simple. Something in this style lists the contents of a given directory:

#include <windows.h>
#include <stdio.h>
#include <stdlib.h>

void listdir(const char *dir)
{
    WIN32_FIND_DATA entry;
    HANDLE h;
    char *wc = aprintf("%s\\*.*", dir);
    if(!wc)
      return; /* out of mem */

    h = FindFirstFile(wc, &entry);
    if(h == INVALID_HANDLE_VALUE)
      return;

    do {
      printf("%s\n", entry.cFileName);
    } while(FindNextFile(h, &entry));

    FindClose(h);
    free(wc);
}

Rough example, but the function calls should show.

colinleroy · 2024-03-19T19:53:08Z

I'm unsure the two failing pipelines are failed related to the PR?

colinleroy · 2024-03-19T19:53:40Z

Concerning Windows, is it OK if I do it in a different PR?

bagder · 2024-03-21T08:41:11Z

Concerning Windows, is it OK if I do it in a different PR?

Absolutely. As long as this PR does not break their builds or misbehaves for them in other ways.

bagder

FYI: I deem this to be a "feature/change", which means that we will not merge this PR until the feature window opens again. (Normally 10 days after a release.)

bagder · 2024-03-21T08:45:17Z

Please move the #ifdef (and #else, and #endif) to column zero.

colinleroy · 2024-03-21T22:40:21Z

FYI: I deem this to be a "feature/change", which means that we will not merge this PR until the feature window opens again. (Normally 10 days after a release.)

That seems logical :)
Your latest comments should be addressed in the last push.

colinleroy · 2024-04-08T16:36:15Z

Hi @bagder! I think the feature freeze is over?
do I have to do something about the failed Mac pipelines? It seems like they're unrelated to the PR?
Thanks 🙂

bagder · 2024-04-09T15:40:23Z

The freeze is over. Those CI fails should be fixed if you rebase and force-push.

I do miss a test case specifically for this feature.

colinleroy · 2024-04-10T16:39:00Z

Hi Daniel,
PR rebased, test added (i hope it's what you expected?) and CI passed!

bagder

👍

bagder · 2024-04-11T10:40:33Z

Thanks!

- configure: disable pthreads by default on Windows. - configure: disable detecting `fseeko()` on Windows. (It exists in mingw-w64 2.0.0 and newer, but it's permanently ignored in CMake, as this function is never necessary on Windows.) - extend existing exceptions with their Windows variants. - `lib/formdata.c`: prioritize `_fseeki64()` over `fseeko()`. To reduce the difference between Windows builds, which now all use `_fseeki64()`. - cmake: perm-enable `HAVE_DIRENT_H` and `HAVE_OPENDIR` for mingw-w64, to match configure. Follow-up to bfe54b0 #13137 This in theory could make the dir listing feature work in mingw-w64 build, but in my tests (on WINE) it failed at the preceding `open()` call. - cmake: perm-enable `HAVE_STRINGS_H` and `HAVE_UTIME_H` for mingw-w64, to match configure. (They are wrappers and make no difference in the build.) Also: - configure: sync `USE_MANUAL` macro with cmake, by only setting it for `src`. Drop checker exception. - CI: use `--disable-dependency-tracking` in existing jobs. - CI: install packages before git checkout, in existing jobs. Closes #14678

bagder reviewed Mar 16, 2024

View reviewed changes

colinleroy force-pushed the add-file-protocol-list branch from 26ea08f to e457ebc Compare March 18, 2024 16:55

bagder reviewed Mar 19, 2024

View reviewed changes

bagder reviewed Mar 21, 2024

View reviewed changes

bagder added the feature-window A merge of this requires an open feature window label Mar 21, 2024

github-actions Bot added the cmdline tool label Mar 21, 2024

github-actions Bot added the tests label Mar 22, 2024

colinleroy force-pushed the add-file-protocol-list branch from 783a505 to 9ae94c6 Compare March 22, 2024 07:29

colinleroy force-pushed the add-file-protocol-list branch from 9ae94c6 to 89fdcd3 Compare April 4, 2024 08:33

file:// : Allow getting basic directory listings

dea3e21

colinleroy force-pushed the add-file-protocol-list branch from 89fdcd3 to dea3e21 Compare April 10, 2024 08:44

bagder approved these changes Apr 11, 2024

View reviewed changes

bagder closed this in bfe54b0 Apr 11, 2024

vszakats mentioned this pull request Aug 25, 2024

GHA/configure-vs-cmake: add Windows build, fix issues #14678

Closed

3 tasks

Uh oh!

Uh oh!

Conversation

colinleroy commented Mar 16, 2024

Uh oh!

gvanem commented Mar 16, 2024

Uh oh!

colinleroy commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfandrich commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bagder commented Mar 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bagder Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colinleroy commented Mar 18, 2024

Uh oh!

bagder Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bagder commented Mar 19, 2024

Uh oh!

colinleroy commented Mar 19, 2024

Uh oh!

colinleroy commented Mar 19, 2024

Uh oh!

bagder commented Mar 21, 2024

Uh oh!

bagder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

colinleroy commented Mar 21, 2024

Uh oh!

colinleroy commented Apr 8, 2024

Uh oh!

bagder commented Apr 9, 2024

Uh oh!

colinleroy commented Apr 10, 2024

Uh oh!

bagder left a comment

Choose a reason for hiding this comment

Uh oh!

bagder commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

colinleroy commented Mar 16, 2024 •

edited

Loading

dfandrich commented Mar 16, 2024 •

edited

Loading

bagder Mar 16, 2024 •

edited

Loading

bagder Mar 19, 2024 •

edited

Loading