GNOME Bugzilla – Bug 316403
Add API for editing GnomeVFSVolumes
Last modified: 2008-09-06 21:22:18 UTC
As suggested by [1], we should add editing caps for VFS volumes so that users are able to change the activation URI/display name for VFS volumes. [1] http://mail.gnome.org/archives/gnome-vfs-list/2005-September/msg00005.html
Milestoning to 2.14. The API doesn't look challengeing, I think I've just put sth. nice together in 10 minutes :).
Created attachment 52288 [details] [review] Proposed API
Created attachment 52289 [details] Testing program
You should use G_DIR_SEPARATOR_S instead of "/".
I think with gconf keys "/" is always used so we are safe to use it instead of G_DIR_SEPARATOR_S which would be "\" on windows
Its really a bit strange that when you change a volume it is destroyed and a new one with the changed detail appears. I don't think users of this API will expect that. In fact, its reasonable to assume that calling gnome_vfs_volume_get_activation_uri() on the volume after calling gnome_vfs_volume_connected_server_set_activation_uri() will return the newly set uri, which it won't with this patch. Of course, getting it actually working correctly is much harder since it involves redoing the locking scheme for the volume manager, but that might be worthwile anyway, since e.g. the hal backend could use that too.
Created attachment 62842 [details] [review] Proposed new proof-of-concept patch The attached patch implements the IPC part as well, adds a "volume-changed" signal to the volume monitor, and ensures that after setting the name, the icon or the uri, the volume monitor reprobes its volumes.
Created attachment 62843 [details] Testing program #2 This testing program is less buggy and shuts down (i.e. deregisters its GnomeVFSClient).
> Of course, getting it actually working correctly is much harder since it involves redoing the locking scheme for the volume manager. Why? Isn't it enough to use the monitor's mutex (cf. attachment 62842 [details] [review]).
*** Bug 321755 has been marked as a duplicate of this bug. ***
Alex: On the mailing list you demanded for an API that reflects that editing is limited to connected servers. Bug 321755 requests a similar feature for CD-ROM volumes, though. I've marked it as a duplicate, but I'm not sure whether the right solution to bug 321755 wouldn't rather be to directly provide a volume name source to HAL for audio CDs.
Inserting VolumeChanged in the middle of the corba interface totally breaks the ABI between the client and the daemon. Adding it at the end is at least marginally safe. (I.E. at most the client will crash, not run random code it didn't expect to run.) Also, you need to handle NULL strings better when calling this. Passing NULL for corba strings is bad. But back to the thread safeness issue. There is no way that volume_monitor->priv->mutex protects the volume. It only protects the lists in the volume monitor. For instance, at the same time as _gnome_vfs_volume_monitor_changed() frees the old display_name for a volume some other thread could be running inside gnome_vfs_volume_get_display_name(), referencing the new already freed name. The way threadsafety is handled for volumes atm is: /* Locking strategy: * * Its important that the public API is threadsafe, since it will be used * to implement gnome-vfs backends (which are threaded). * This is handled in various ways: * a) volumes/drives are mostly read-only, and for the few items that * can change (refcount, drive/volume pointer) we lock on getters * and setters * b) Changes to the volume monitor itself is only done by the main thread. * This means we don't have to protect main thread reads of the state, since * the data won't change unpredictably. * c) All writes to the volume manager thread are protected by the volume monitor * lock, and all public API that reads this state also uses this lock. * */ The refcount really doesn't need to be protected anymore since gobject ref/unref is now threadsafe. It might be possible to just extend the use of the current lock to the new fields that can change, but it requires serious consideration to all the possible codepaths.
(In reply to comment #12) > Inserting VolumeChanged in the middle of the corba interface totally breaks the > ABI between the client and the daemon. Adding it at the end is at least > marginally safe. (I.E. at most the client will crash, not run random code it > didn't expect to run.) Also, you need to handle NULL strings better when > calling this. Passing NULL for corba strings is bad. I guess this problem isn't relevant anymore as we're using D-Bus for the daemon, right?
Created attachment 79141 [details] [review] Proposed final patch OK, the attached patch is the first serious candidate for inclusion. * Client sets GConf keys * Daemon eats GConf keys * Daemon notes that a server was changed (i.e. ID is identical, but name/URI/icon changed) * Daemon updates its volume * Daemon emits "volume-changed" on its volume monitor * Daemon emits D-Bus signal "VolumeChanged" with new volume * Client handles D-Bus signal "VolumeChanged", extracts name/URI/icon from D-Bus struct, sets them on its own copy (locked!). * Client emits "volume-changed" on its volume monitor. * Application handles "volume-changed" Note that this patch also adds a neat GTK+-based volume editor that will be compiled in test/, and also adds some docs (yay!).
I still don't see any handling of the thread safety issue. For example, gnome_vfs_volume_update() modifies volume->priv->display_name with the volumes lock taken. However, the volumes lock doesn't protect display_name in e.g. gnome_vfs_volume_get_display_name() which might be running on another thread, as we free the current value.
Ah, I see. Thanks, that's where it turns out that I never did multithreaded programming due - mainloops are pretty. Should we use a per-volume GMutex for this or is the global volume lock more appropriate? I don't think we'll face any mass-renames, so a per-object GMutex is probably overhead - right?
The global lock is probably ok.
Created attachment 88840 [details] [review] Proposed final patch #2 OK, I finally had some time to change the patch according to your suggestions.
Christian, Alexander, is this patch going in?
gnome-vfs has been deprecated and superseded by gio/gvfs since GNOME 2.22, hence mass-closing many of the gnome-vfs requests/bug reports. This means that gnome-vfs is NOT actively maintained anymore, however patches are still welcome. If your reported issue is still valid for gio/gvfs, please feel free to file a bug report against glib/gio or gvfs. @Bugzilla mail recipients: query for gnome-vfs-mass-close to get rid of this notification noise all together. General further information: http://en.wikipedia.org/wiki/GVFS Reasons behind this decision are listed at http://www.mail-archive.com/gnome-vfs-list@gnome.org/msg00899.html
Opened followup bug 551158.