Re: [gfvs] cdda backend

[Alexander Larsson <alexl redhat com>]

On Sun, 2007-12-16 at 18:22 -0500, David Zeuthen wrote:

> So I wrote the attached cdda module because with gio/gvfs I think it
> actually makes sense have a cdda:// backend (also, and sorry for
> sounding like a cargo cult follower, to have feature parity with Mac OS
> X and Windows). And I also wanted to get a feel / help review the plugin
> API before we freeze. 

I'm not convinced this is what people want (over launching e.g.
sound-juicer or gnome-cd when clicking on the audio disc icon), but lets
add it, it doesn't hurt.

> First, a small feature list
> 
>  - Stateful mounts; yes, you actually "mount" the cdda gvfs filesystem
>    driver; there's one instance of gvfsd-cdda per mount. You also have
>    to mount the fs manually: gvfs-mount cdda://sr0

You don't actually have to make each cdda mount its own process. If you
want you can add a well-known dbus name to the cdda.mount.in file (which
is missing from your patch btw). gvfsd will hand off cdda mount
operations to that name if it exists, instead of spawning a new copy.

>  - Works with multiple clients at the same time as well as when
>    a iso9660 filesystem is mounted by a kernel driver. There's seeking
>    and it's a bit slow (can be alleviated once we introduce readahead
>    in the GVfsBackend base class, yes?) but basically works very well

Readahead is mainly gonna speed up the case where an app does
read_source()
write_destination()
read_source()
write_destingation()
...

I.e. while it blocks writing what it just read we can automatically
schedule a read from the source so that the next read will finish
faster. Its not really related to seeks.

>  1. The GVfs FUSE daemon never class close() on my fs backend when
>     it's done reading.. this leads to leaks. Have you seen this?

The fuse daemon is mainly the work of hans petter. I haven't really
looked into it or tried it that much.

>  2. I have to set GMountSpec in try_mount(); it's not enough just
>     setting it in do_mount(). Bug?

Yes, that shouldn't happen. Can you try to look into this?

>  3. Once mounted, there's no way for the backend to change it's display
>     name. I think I understand why you did this; it's because of the
>     FUSE daemon right? E.g. you want to ensure that there are no
>     name collisions to make it easier to figure out what directories
>     to return in ~/.gvfs ...
> 
>     Anyway, the reason we want this is that initially we probably want
>     the display name "Audio Disc" and if we (much later) acquire meta
>     data for the disc we want to change it to "Tori Amos - Silent
>     all these years" or whatever the title is.
> 
>     (same applies for the icon (think cover art); I haven't tested if
>      this doesn't work)

Hmmm. Tricky. Its kinda weird having the name of the actual mountpoint
change. Especially with it being the identifier for the mount in the
fuse case. I think the best solution is to have the volume be called
_("Audio Disc"), but then set the display name of the root of the disk
to the actual disk title. Thats what will be displayed in many places
like the window title for the toplevel directory.

>  4. There's very little in way of documentation so it was really
>     confusing to start writing the backend. Once I got the hang of
>     it, it was a smooth ride though.

Well, gvfs is still kinda raw and not at the same state of polish as
gio. On the other hand, its not all that complex once you get the hang
of it (except some internal details).

>  5. There needs to be a way to associate a GDaemonMount to an existing
>     GVolume object (stemming from the HAL volume monitor). This is
>     not a problem inherent to the CDDA backend; it applies to any
>     backend that provides a user space file system driver for a block
>     device (e.g. NTFS, secure paranoid vfat etc.).
> 
> The first problems are probably easily fixable; the last one is more
> problematic. Here's how I envision it will work
> 
> User inserts audio disc into drive. The HAL volume monitor notices this
> and creates a GVolume object representing the unmounted audio disc. The
> mount() method on the HAL volume monitor for said GVolume will simply do
> the equivalent of
> 
>      $ gvfs-mount cdda://sr0

(This is just g_file_mount_enclosing_volume)

> since it knows there a cdda:// backend already. So when this happens the
> gvfs-cdda backend is started for sr0 and as a result a GDaemonMount is
> created in the process space of all gvfs clients using volume monitoring
> (via GDaemonVolumeMonitor). Now, I think all we need to do is
> 
>  o  Make the GHalVolume live object for sr0 note that the
>     new GDaemonMount object was created and associate with it (e.g.
>     take a weak ref and emit "changed". Future calls into get_mount()
>     for the GHalVolume will return that ref).
> 
>  o  Have a way for the gvfsd-cdda backend to tell that the created
>     GDaemonMount objects representing it needs to make get_volume()
>     on them return a reference to the GHalVolume object.
> 
> This all sounds complicated but I'm not sure it really is. I think all
> we need to do is to have some kind of tags we can decorate the
> GHalVolume and GDaemonVolume objects so they can find each other.
> Anyway, you probably have some ideas I haven't thought about...

I don't see an obvious easy solution. It requires some work. The ideal
thing to match with is an GMountSpec (which is basically the set of
key-value pairs that define a gvfs mount). For a GDaemonMount this is
availible in the GMountInfo object.

Thats a gvfs-specific object though, so we can't make that availible in
the base GVolume apis. What if we add some new vtable calls in
GVolumeMonitor like find_volume_for_new_mount() and
find_mount_for_new_volume() which we have GUnionVolumeMonitor call on
the installed backends when new, unowned volume/mounts are added.

Some comments on the patch:

For the mount spec you use e.g. "host=sr". There is no strict need to
map mount specs to the uri fields. If you want to you can have a more
descriptive mount spec like device="sr0". However, then you need to add
a uri mapper for "cdda". See smburi.c for an example.

+  track_num = get_track_num_from_name (cdda_backend, job->filename);
+  if (track_num == -1)
+    {
+      error = g_error_new (G_IO_ERROR, G_IO_ERROR_FAILED, 
+                           _("No such file %s on drive %s"), job->filename, cdda_backend->device_path);

You should return G_IO_ERROR_NOT_FOUND in this case. Not FAILED.

+set_info_for_track (GVfsBackendCdda *cdda_backend, GFileInfo *info, int track_num)

Also set these attributes:
G_FILE_ATTRIBUTE_ACCESS_CAN_READ = TRUE
G_FILE_ATTRIBUTE_ACCESS_CAN_WRITE = FALSE
G_FILE_ATTRIBUTE_ACCESS_CAN_DELETE = FALSE
G_FILE_ATTRIBUTE_ACCESS_CAN_EXECUTE = FALSE
G_FILE_ATTRIBUTE_ACCESS_CAN_TRASH = FALSE
G_FILE_ATTRIBUTE_ACCESS_CAN_RENAME = FALSE

+do_query_info (GVfsBackend *backend,
+  if (strcmp (filename, "/") == 0)
+    {

Set display name to something nice. This is what e.g. the window title
will say when displaying the toplevel dir in nautilus.

+do_enumerate (GVfsBackend *backend,

This needs to verify that path is /, or return G_IO_ERROR_NOT_DIRECTORY