Some comments about GVFS

["Benjamin Otte" <otte gnome org>]

Hi,

I had an initial look at gvfs in particular the Inputstream and
Outputstream implementations, and some comments came to my mind, in
particular about the API. So I thought I'd post them as early as
possible.
These comments are about the code in
http://www.gnome.org/~alexl/git/gvfs.git from today - May 2nd.
I should also note that these ideas are probably heavily influenced by
where I'm coming from - namely GStreamer and Swfdec - which involve
handling low level byte streams possibly caring about low latency. In
this particular case I had been wondering about making the basic
input/output objects in Swfdec GInput/OutputStream.

1) I don't like GCancellable This is for several reasons:

1a) GCancellable is far too visible. It's present in every function
call, so it must be very important. Maybe exporting
g_push/pop_current_cancellable () would be a better API because it
decouples the cancelling from the actual operations?

1b) Cancelling operations from another thread doesn't look like good
design to me. I've learnt (both in theory and in practice) that
threads are supposed to be independant and not call into each other.

2) GVFS seems to avoid the glib main loop for asynchronous operations
in favour of relying on threads. I'm not sure this provides any
benefits besides making apps way harder to debug. Is there a reason
for that?

3) The whole API seems to be built around the model of synchronous
operation. (I think so because of this code comment: /* Async ops:
(optional in derived classes) */) I always thought everyone agrees
that synchronous operation should be a thing of the past. and only be
supported as an add-on for lazy coders or really simple apps.
Almost everyone implements synchronous operations something like this:
void foo () { while (errno == EAGAIN) foo_async (); };
The kernel sure does.

4) The asynchronous API seems to avoid the POSIX asynchronous model in
favour of callbacks. Is there a reason why? I'd have guesses an
asynchronous API looked like GIOChannel with some sugar on top.
Something like this:
if (!g_input_stream_read (stream, buf, size))
 g_input_stream_wakeup_when_available (stream, size, func, data);
And have that wake up in the glib main context when enough data is available.

5) You seem to use void * in as the data pointers. All applications I
know use guchar * (some use gchar *) to handle data. From my stream
handling experience this is to encourage people to think about what
they pass to such a function. This seems to encourage calling
functions like this: write (mywidget, sizeof (MyWidget)) - with is a
bad idea for multiple reasons, including but not limited to struct
padding. MS formats seem to have done that a lot.

6) Has there been thoughts about using a buffer-like struct? I seem to
remember having talked about that last Guadec, but don't remember. To
elaborate: A buffer structure is basically a refcounted memory region.
All the multimedia libs make use of a structure like it. Some examples
are at [1] and [2]. It allows some neat features like avoiding memcpys
to reference the data and subbuffers.

7) The error handling in gvfs seems to be oriented towards the "old"
and in glib still common error model of having people supply a GError
to every function. I much prefer the cairo model, where basically the
object keeps its own GError and has a function like cairo_status [3]
that gives you the last error that occured. This has multiple
advantages such as that I don't have to check for errors in every
function, I can get the error on request and don't have to keep it
around manually and function calls have one parameter less.

8) The API seems very forgiving to (IMO) obvious porogramming errors
that should g_return_if_fail. g_input_stream_read for example provides
a proper error message when there's already a pending operation or
when the size parameter is too large.

9) I got the feeling that the API is somewhat designed to make one
Stream object be usable from multiple threads at once. I can't
pinpoint that, it's just a feeling. That's not the case, is it?


Wow, that got more than I thought it would become. Please don't take
that as me slamming gvfs, I'm just curious as to why those decisions
were made.

Cheers,
Benjamin


[1] http://swfdec.freedesktop.org/documentation/swfdec/swfdec-SwfdecBuffer.html
[2] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/gstreamer-GstBuffer.html
[3] http://www.cairographics.org/manual/cairo-cairo-t.html#cairo-status