Re: GDir as DIR replacement - patch

[Owen Taylor <otaylor redhat com>]

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.

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

   What makes this a special chunk of functionality?

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.

 - 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.)

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?

 - We are confident that we can get it right on the
   first try.

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. g_dir_len()
should be omitted because it invites race conditions
or just really ineffecient code. 

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

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

Regards,
                                        Owen