file:// : Allow getting basic directory listings #13137

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

lib/file.c

+  *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

lib/file.c

+
+  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

lib/file.c

+
+      *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

lib/file.c

+        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

lib/file.c

+    }
+  }
+  else {
+    #ifdef HAVE_OPENDIR


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

lib/file.c

lib/urldata.h

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!

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

file:// : Allow getting basic directory listings #13137

file:// : Allow getting basic directory listings #13137

colinleroy commented Mar 16, 2024

gvanem commented Mar 16, 2024

colinleroy commented Mar 16, 2024 •

edited

dfandrich commented Mar 16, 2024 •

edited

bagder commented Mar 16, 2024

bagder Mar 16, 2024

bagder Mar 16, 2024

bagder Mar 16, 2024 •

edited

colinleroy Mar 18, 2024

bagder Mar 18, 2024

colinleroy Mar 18, 2024

bagder Mar 18, 2024

colinleroy commented Mar 18, 2024

bagder Mar 19, 2024 •

edited

colinleroy Mar 19, 2024

bagder commented Mar 19, 2024

colinleroy commented Mar 19, 2024

colinleroy commented Mar 19, 2024

bagder commented Mar 21, 2024

bagder left a comment

bagder Mar 21, 2024

colinleroy commented Mar 21, 2024

colinleroy commented Apr 8, 2024

bagder commented Apr 9, 2024

colinleroy commented Apr 10, 2024

bagder left a comment

bagder commented Apr 11, 2024

file:// : Allow getting basic directory listings #13137

file:// : Allow getting basic directory listings #13137

Conversation

colinleroy commented Mar 16, 2024

gvanem commented Mar 16, 2024

colinleroy commented Mar 16, 2024 • edited

dfandrich commented Mar 16, 2024 • edited

bagder commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder Mar 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinleroy commented Mar 18, 2024

bagder Mar 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Mar 19, 2024

colinleroy commented Mar 19, 2024

colinleroy commented Mar 19, 2024

bagder commented Mar 21, 2024

bagder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinleroy commented Mar 21, 2024

colinleroy commented Apr 8, 2024

bagder commented Apr 9, 2024

colinleroy commented Apr 10, 2024

bagder left a comment

Choose a reason for hiding this comment

bagder commented Apr 11, 2024

colinleroy commented Mar 16, 2024 •

edited

dfandrich commented Mar 16, 2024 •

edited

bagder Mar 16, 2024 •

edited

bagder Mar 19, 2024 •

edited