[PATCH] Add global mutex around each C call
From: Maros Zatko <mzatko@redhat.com> Patch adds recursive mutex around C calls, which tries to fix potential race condition in guestfs_umount_local (see [1]). [1] - https://bugzilla.redhat.com/show_bug.cgi?id=917706 Maros Zatko (1): generator: c.ml - wrap non deamon function with recursive mutex generator/c.ml | 12 ++++++++++++ 1 file changed, 12 insertions(+) -- 1.8.5.3
From: Maros Zatko <mzatko@redhat.com> --- generator/c.ml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/generator/c.ml b/generator/c.ml index ee276dc..4333bb8 100644 --- a/generator/c.ml +++ b/generator/c.ml @@ -788,6 +788,9 @@ and generate_internal_actions_h () = pr "#define GUESTFS_INTERNAL_ACTIONS_H_\n"; pr "\n"; + pr "#include \"glthread/lock.h\"\n"; + pr "\n"; + List.iter ( fun { c_name = c_name; style = style } -> generate_prototype ~single_line:true ~newline:true ~handle:"g" @@ -796,6 +799,10 @@ and generate_internal_actions_h () = ) non_daemon_functions; pr "\n"; + + pr "gl_recursive_lock_define_initialized(static, global_lock)\n"; + pr "\n"; + pr "#endif /* GUESTFS_INTERNAL_ACTIONS_H_ */\n" (* Generate guestfs-internal-frontend-cleanups.h file. *) @@ -1567,6 +1574,8 @@ and generate_client_actions hash () = c_name style; pr "{\n"; + pr " gl_recursive_lock_lock (global_lock);\n"; + handle_null_optargs optargs c_name; pr " int trace_flag = g->trace;\n"; @@ -1617,6 +1626,9 @@ and generate_client_actions hash () = trace_return name style "r"; ); pr "\n"; + + pr " gl_recursive_lock_unlock (global_lock);\n"; + pr " return r;\n"; pr "}\n"; pr "\n" -- 1.8.5.3
On Thu, May 15, 2014 at 05:39:08PM +0200, mzatko@redhat.com wrote:
+ + pr "gl_recursive_lock_define_initialized(static, global_lock)\n";
static? I suspect this only allows one libguestfs handle per process into the critical section at once. I think the lock needs to be per-handle unless I'm misunderstanding what this is for. Rich.
+ pr "\n"; + pr "#endif /* GUESTFS_INTERNAL_ACTIONS_H_ */\n"
(* Generate guestfs-internal-frontend-cleanups.h file. *) @@ -1567,6 +1574,8 @@ and generate_client_actions hash () = c_name style; pr "{\n";
+ pr " gl_recursive_lock_lock (global_lock);\n"; + handle_null_optargs optargs c_name;
pr " int trace_flag = g->trace;\n"; @@ -1617,6 +1626,9 @@ and generate_client_actions hash () = trace_return name style "r"; ); pr "\n"; + + pr " gl_recursive_lock_unlock (global_lock);\n"; + pr " return r;\n"; pr "}\n"; pr "\n" -- 1.8.5.3
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Fri, May 16, 2014 at 04:44:41PM +0100, Richard W.M. Jones wrote:
On Thu, May 15, 2014 at 05:39:08PM +0200, mzatko@redhat.com wrote:
+ + pr "gl_recursive_lock_define_initialized(static, global_lock)\n";
static?
I suspect this only allows one libguestfs handle per process into the critical section at once. I think the lock needs to be per-handle unless I'm misunderstanding what this is for.
Agreed, any locking should be per-handle, and I wouldn't expect to need to use recursive mutexes either. Internal libguestfs code shouldn't be calling back out to the public libguestfs API surely, so shouldn't need to have re-entrancy Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Fri, May 16, 2014 at 11:47:17AM -0400, Daniel P. Berrange wrote:
On Fri, May 16, 2014 at 04:44:41PM +0100, Richard W.M. Jones wrote:
On Thu, May 15, 2014 at 05:39:08PM +0200, mzatko@redhat.com wrote:
+ + pr "gl_recursive_lock_define_initialized(static, global_lock)\n";
static?
I suspect this only allows one libguestfs handle per process into the critical section at once. I think the lock needs to be per-handle unless I'm misunderstanding what this is for.
Agreed, any locking should be per-handle, and I wouldn't expect to need to use recursive mutexes either. Internal libguestfs code shouldn't be calling back out to the public libguestfs API surely, so shouldn't need to have re-entrancy
Actually libguestfs does call itself quite a lot. Whether it should call itself through the public API or not is another matter -- we could change that fairly easily. Is there a problem with recursive mutexes? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Fri, May 16, 2014 at 04:59:54PM +0100, Richard W.M. Jones wrote:
On Fri, May 16, 2014 at 11:47:17AM -0400, Daniel P. Berrange wrote:
On Fri, May 16, 2014 at 04:44:41PM +0100, Richard W.M. Jones wrote:
On Thu, May 15, 2014 at 05:39:08PM +0200, mzatko@redhat.com wrote:
+ + pr "gl_recursive_lock_define_initialized(static, global_lock)\n";
static?
I suspect this only allows one libguestfs handle per process into the critical section at once. I think the lock needs to be per-handle unless I'm misunderstanding what this is for.
Agreed, any locking should be per-handle, and I wouldn't expect to need to use recursive mutexes either. Internal libguestfs code shouldn't be calling back out to the public libguestfs API surely, so shouldn't need to have re-entrancy
Actually libguestfs does call itself quite a lot. Whether it should call itself through the public API or not is another matter -- we could change that fairly easily. Is there a problem with recursive mutexes?
Not a technical issue, rather its more of a philosophical issue. IME, if you permit recursive mutexes, it allows people to create a tangled mess of interdepedant/recursive API calls which can become quite hard to understand over time. Non-recursive mutexes help to force cleaner code separation. Of course if libguestfs already calls itself reentrantly via its public API entry points, the issue is moot, unless you decide you want to spend the time to refactor it. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/16/2014 05:59 PM, Richard W.M. Jones wrote:
On Fri, May 16, 2014 at 11:47:17AM -0400, Daniel P. Berrange wrote:
On Fri, May 16, 2014 at 04:44:41PM +0100, Richard W.M. Jones wrote:
On Thu, May 15, 2014 at 05:39:08PM +0200, mzatko@redhat.com wrote:
+ + pr "gl_recursive_lock_define_initialized(static, global_lock)\n"; static? eh, a mistake
I suspect this only allows one libguestfs handle per process into the critical section at once. I think the lock needs to be per-handle unless I'm misunderstanding what this is for. Agreed, any locking should be per-handle, and I wouldn't expect to need to use recursive mutexes either. Internal libguestfs code shouldn't be calling back out to the public libguestfs API surely, so shouldn't need to have re-entrancy I'll rework it as per-handle, will try non-recursive variant as well Actually libguestfs does call itself quite a lot. Whether it should call itself through the public API or not is another matter -- we could change that fairly easily. Is there a problem with recursive mutexes?
Rich.
participants (4)
-
Daniel P. Berrange -
Maros Zatko -
mzatko＠redhat.com
-
Richard W.M. Jones