Re: GDir as DIR replacement - patch

[Hans Breuer <hans breuer org>]

At 10:33 03.11.01 -0500, Owen Taylor wrote:
>
>Hans Breuer <hans breuer org> writes:
>
>> As some of you may have noticed recently the plain 
>> DIR emulation was removed from glib (win32) to
>> avoid namespace pollution.
>> 
>> [see:
>>   http://mail.gnome.org/archives/gtk-devel-list/2001-October/msg00001.html
>>   http://mail.gnome.org/archives/gtk-devel-list/2001-October/msg00511.html
>> ]
>> 
>> To fill the gap (and to not require a small extra lib
>> within most of the GTK+ libs and apps) it would be
>> nice if some g_dir_* functions could be included in glib.
>> 
>> If the following patch is acceptable I would obviously 
>> volunteer to write inline docs, a test app and remove all 
>> the DIR occurences from 'downstream' libs and apps ported 
>> to win32.
>
>Thoughts here:
>
> * I don't like the idea of adding more API at this point at
>   all. This seems hard to get wrong, but still...
>   we need to cut off API additions and in a couple of
>   days we'll be saying no to even fixing obvious API
>   mistakes.
>
Sounds reasonable ...

> * Is this the only "POSIX but non ANSI" thing we'd end
>   up wrapping? It doesn't do much good to rush this into 
>   GLib-2.0 and then find that we need mkdir() or stat()
>   or whatever anyways.
>
No need for these yet. The DIR emulation has a special history
and at least _stat() and _mkdir() work just fine.

>   What makes this a special chunk of functionality?
>
It has long-time lived in glib/win32 under its original name.
Recently Tor decided to remove it to prevent namespace clashes.

That introduces the requirement to handle DIR emulation in about
10 projcets (88 occurences of opendir in my tree). It can be done 
(and partly already is) but IMHO it's a mess to include and link 
it the same way if there is one place where it belongs :-)

>I took a look at the NSPR and APR APIS for comparison, and
>they are pretty much just opendir()/readdir()/closedir()
>(APR has rewinddir(), NSPR doesn't). Additions are:
>
> - APR's readdir() equivalent allows getting stat information
>   at the same time. (You pass in a mask of what name/stat
>   information you want; though it isn't implemented now
>   in APR, the idea is that 'struct dirent' might contain
>   the information you need saving a separate stat.)
>
>   This could be added later if wanted; since we don't
>   have anything stat() style, it would be premature
>   for now.
>
That's what I thought looking at some use case in 'g'libs/apps

> - NSPR's readdir() has flags "SKIP_DOT, SKIP_DOT_DOT,
>   SKIP_BOTH, SKIP_HIDDEN". I think most uses of readdir
>   do want SKIP_BOTH, so it could be considered convenient
>   to not have to put the strcmp()'s in each usage.
>   OTOH, it could be considered bloat since you _can_
>   do the strcmp. (Actually, I'd almost say that 
>   you should just always skip . and .. since if you
>   want them, you know they are there, and otherwise
>   they are just an invitation to infinite loops.)
>
Even some combination with glob or fcntl may be nice and
returning stuff in glib data structures; which obviously
is all 2.2 stuff if desired.

>My general feeling is that this sounds very reasonable
>for 2.2, but we should only put it in for 2.0 if:
>
> - We are confident that it fills an important hole,
>   and is not just a small piece of what is needed.
>   What is special about the DIR functions?
>
See above. Additional one may count the removement after the
api-freeze as already breaking the api.

> - We are confident that we can get it right on the
>   first try.
>
I'll try my best.

>Detailed comments on the implementation:
>
>> #include <glib/gerror.h>
>> 
>> G_BEGIN_DECLS
>> typedef struct _GDir GDir;
>> 
>> GDir    *g_dir_open (const gchar *path, GError **error);
>> int      g_dir_len (GDir *dir);
>> G_CONST_RETURN gchar *g_dir_read_name (GDir *dir);
>> void     g_dir_rewind (GDir *dir);
>> int      g_dir_tell (GDir *dir);
>> void     g_dir_seek (GDir *dir, int len);
>> gboolean g_dir_close (GDir *dir, GError **error);
> 
>I would omit g_dir_tell(), g_dir_seek(), since they aren't
>in POSIX, and I've never seen a use for them. 
fine with me (no occurence of telldir in my tree beside in glib :).  

>g_dir_len()
>should be omitted because it invites race conditions
>or just really ineffecient code. 
>
Also fine with me. A side note: I 'designed' it after looking
in gtk+/gtk/gtkfilesel.c ...
 
>> G_CONST_RETURN gchar*
>> g_dir_read_name (GDir *dir)
>> {
>>   struct dirent *entry;
>> 
>>   g_return_val_if_fail (dir != NULL, NULL);
>>   g_return_val_if_fail (dir->dir != NULL, NULL);
>
>How could dir->dir ever be NULL? g_return_val_if_fail()
>statements are generally checking for programmer messups
>not "the world blew up"
>
Will remove them.

>>   entry = readdir (dir->dir);
>> 
>>   if (entry != NULL)
>>     return entry->d_name;
>> 
>>   return NULL;
>> }
>
>I seem to remember on some systems, if you open a non-directory,
>opendir() suceeds and readdir() fails with ENOTDIR. (I'm
>sure that the contrary works this way - if you open() a
>directory, than read() fails with EISDIR()). So perhaps
>we need a GError argument here?
> 
It would additional give symmetrie on the remaining three
g_dir functions.

>This would be 
>
>> gboolean
>> g_dir_close (GDir *dir, GError **error)
>> {
>>   int ret = 0;
>> 
>>   g_return_val_if_fail (dir != NULL, FALSE);
>>   g_return_val_if_fail (dir->dir != NULL, FALSE);
>>   ret = closedir (dir->dir);
>> 
>>   if (ret)
>>     {
>>       /* error case */
>>       g_set_error (error,
>>                    G_FILE_ERROR,
>>                    g_file_error_from_errno (errno),
>>                    _("Error closeing dir : %s"),
>                                  ^
>>                    strerror (errno));
>>     }
>> 
>>   return !ret; 
>> }
>
>How do we _free_ a GDir structure?
>
Just in g_dir_close () ? I already had the wrong g_free (dir->dir)
in my local copy. Just fixed to g_free (dir).

Thanks,
	Hans

-------- Hans "at" Breuer "dot" Org -----------
Tell me what you need, and I'll tell you how to 
get along without it.                -- Dilbert