GNOME Bugzilla – Bug 627116
Add API to allow drive locking
Last modified: 2018-05-24 12:35:37 UTC
Created attachment 168033 [details] [review] patch to add the above to glib/gio An API that would allow to set a lock on a GDrive object could be useful in cases where an application would like to tell all other applications that it is using the GDrive and that therefore they should avoid using/accessing it. Here is the proposal: typedef enum { G_DRIVE_LOCK_NONE = 0 } GDriveLockFlags; guint g_drive_obtain_lock_sync (GDrive *drive, GDriveLockFlags flags, const gchar *reason, GCancellable *cancellable, GError *error); /* Note: error returns the above reason argument set by the process holding the lock if the drive is already locked */ Note about the lock identifier returned by the above function, 0 means failure. gboolean g_drive_release_lock_sync (GDrive *drive, guint lock_id, /* a cookie like in Inhibit method */ GError **error); guint g_file_obtain_lock_finish (GDrive *drive, GAsyncResult *res, GError **error); gboolean g_file_release_lock_finish (GDrive *drive, GAsyncResult *res, GError **error); void g_drive_obtain_lock_async (GDrive *drive, GDriveLockFlags flags, const gchar *reason, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); void g_drive_release_lock_async (GDrive *drive, guint lock_id, GAsyncReadyCallback callback, gpointer user_data); gboolean g_drive_get_is_locked(GDrive); A new GIOErrorNum: G_IO_ERROR_LOCKED
Looks good in principle; some quick comments - we generally only have async methods in the GVolumeMonitor APIs; so should just have obtain_lock{,finish} and release_lock{,finish} - docs for g_drive_get_is_locked() should state that the user can connect to the GDrive::changed signal to check for changes - we general don't add API in gio without having an implementation; are you planning to work on one for the gdu monitor in GVfs? Thanks, David
(In reply to comment #1) > - we generally only have async methods in the GVolumeMonitor APIs; so > should just have obtain_lock{,finish} and release_lock{,finish} Ok, will do. > - docs for g_drive_get_is_locked() should state that the user can connect > to the GDrive::changed signal to check for changes I'll update the comment. > - we general don't add API in gio without having an implementation; are > you planning to work on one for the gdu monitor in GVfs? I already have an implementation for GVfs which is not gdu specific. Instead of that it uses g_object_set ()/g_object_get () on the GDriveProxy objects in /monitor/proxy/gvfsproxyvolumemonitordaemon.c to set the locks. One of the advantages is that it works all the time for any implementations. I chose this path as this was only meant to be a lock per session. I could add the patch to this bug if you want to take a look at it?
Created attachment 168726 [details] [review] Updated version of the patch
I updated the patch according to your suggestions I took the opportunity to shorten some function names removing the "_async" suffixes as there is no synchronous counterparts anymore. I also added some comments for the new methods added to the GDrive interface (that I had forgotten in the first patch).
(In reply to comment #2) > > - we general don't add API in gio without having an implementation; are > > you planning to work on one for the gdu monitor in GVfs? > > I already have an implementation for GVfs which is not gdu specific. Instead of > that it uses g_object_set ()/g_object_get () on the GDriveProxy objects in > /monitor/proxy/gvfsproxyvolumemonitordaemon.c to set the locks. One of the > advantages is that it works all the time for any implementations. I chose this > path as this was only meant to be a lock per session. > > I could add the patch to this bug if you want to take a look at it? Sure. Please do. Thanks.
Created attachment 169157 [details] [review] glib updated patch
Created attachment 169158 [details] [review] Gvfs patch This is the patch against gvfs. It is meant to implement locking generically so every backend does not need to implement its own locking facilities. It is unfinished (see below) but I am willing to improve it or even drop it entirely. It is mostly a way to show how I think locking could be implemented. Now if you prefer to add a locking API to Gdu and think it is worthier then I can do it as well. I said it is unfinished because I do not know how to properly implement cancellation. One of the main problem in this area is if the process which called g_drive_obtain_lock () triggers the GCancellable object just while the Gvfs daemon is returning its answer and cookie over DBus. In this case nobody will be able to get the lock until the calling process dies as the cookie will be lost. The solution would be to call ReleaseLock DBus method when we receive a positive answer and a cookie from the Gvfs daemon in GProxyDrive if the GCancellable has been triggered.
Hmm, I don't really know what a GDrive is and I am too lazy to look it up right now, but does this locking stuff have anything to do with the functionality wanted in bug #573293 (and with a quick first draft patch, which might be totally stupid and uninsightful of course)?
As far as I can tell, I would say the two should be seen as complementary. Your proposal is about files while this one is about drives (hard drives, optical disks, ...). I meant to facilitate the cooperation between applications when they want to access/mount/umount/... a drive. An example: when a cd burning application finishes to burn an optical disc, it may have other operations to perform which could be disrupted by another application that tries to mount or access the drive in any way. This is even truer while it is burning. Now of course your proposal is complementary since if we lock a drive, we should set a lock on all files held by its volumes as well; that would mean that when opening a file (if required), checking for the locking status of the containing drive status would be necessary. What do you think?
Hmm, based on that description I think the functionalities are really unrelated. If a disk drive is "locked", one should surely not even open a file on it read-only (with no per-file locking or share modes involved), because that would adjust the access timestamp, no? Is this bug only about locking optical drives and similar devices, and not drives like hard disks with one or several normal file systems on them? (Anyway, does the term "drive" as in GDrive really refer to an actual physical disk drive, or should it more be interpreted as a "volume" that can span several physical devices, or be just a part of a physical device? Oh well, whatever, if GDrive is stable API it's too late to reconsider terminology now...)
Created attachment 172698 [details] [review] Updated version
Created attachment 172699 [details] [review] Updated version of gvfs patch
David, please, now that the new glib was released could you review these patches? We really need these for brasero. I have updated both of them and add a confirmlock DBus method so that when the client requests a lock it goes like this: DBus "ObtainLock" > client receives a positive answer (hopefully) with a cookie > DBus "ConfirmLock" with cookie to avoid a timeout of the lock (2 seconds) Without this any cancellation that would happen on the client side right after the server sent his answer would lock the drive without letting the client aware of it. So now everything should work. If you prefer having the lock code specifically written in gdu let me know and I'll do it.
bump
Philippe has the gvfs patch been attached to a gvfs bug or only here? If only here you probably need to open a gvfs bug and attach it there.
Is there something new here? I would like brasero to work as it should, I don't like using k3b to burn my DVD's
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/329.