Re: [patch] concurrency fixes for the fuse daemon

[David Zeuthen <david fubar dk>]

On Sun, 2008-04-20 at 19:16 -0500, Hans Petter Jansson wrote:
> On Sun, 2008-04-20 at 15:38 -0400, David Zeuthen wrote:
> 
> > After some more testing I realized we're leaking the reference. Which
> > means the same stream will get reused and this fails if the backend
> > doesn't support seeking (for example the archive backend; still worked
> > great with backends that support seeking e.g. cdda, sftp, gphoto2). With
> > the old patch this would happen on a fresh mount
> > 
> > $ file .gvfs/f9-20080319.iso/README 
> > .gvfs/f9-20080319.iso/README: UTF-8 Unicode English text
> > 
> > $ file .gvfs/f9-20080319.iso/README 
> > .gvfs/f9-20080319.iso/README: ERROR: cannot read `.gvfs/f9-20080319.iso/README' (Operation not supported)
> > 
> > The attached patch fixes that by unreffing the FileHandle in
> > vfs_release() before freeing it.
> 
> Ok, if you've tested this version of the patch and it works, I think it
> should be good to commit.

Yeah, I've tested this with a bunch of different backends and it seems
to work fine. I'll commit it now.

> > Hmm.. I wonder if the correct fix
> > involves not sharing FileHandle's at all? I mean, we only use it for
> > stream operations anyway; doesn't seem we gain a lot by sharing these.
> 
> That leaves the question of how we detect when an open file gets
> renamed. The shared table lets us redirect subsequent I/O on the open
> file descriptor to the renamed file, emulating Unix semantics. There are
> also special cases for unlink and ftruncate.
> 
> I'd rather get rid of that code if it's unnecessary, though. Does GVFS
> handle all that for us internally?

I think so. I mean, once you open a file with gio, you should only keep
a reference to the GInputStream or GOutputStream and that should stay
valid even if the underlying file changes (e.g. is renamed or unlinked).
Of course this might be backend specific but I think backends are
supposed to work that way too and if they don't work it's probably just
a bug with that backend. So if this is true, which I think it may be, we
should be able to get rid of that code. Thoughts?

    David