3:14 p.m.
Closing the handle in one thread while another thread is locked causes
undefined behavior (as closing does not obtain a lock, and can cause
use-after-free or other nasty problems to the thread that does own the
lock). However, it is not sensible to try and obtain a lock in
nbd_close, as POSIX says that it is also undefined for any other
thread to wait on a mutex that has already been destroyed. Therefore,
we don't need to change our code, but merely remind users that
nbd_close is not safe until all other uses of the handle have ceased.
---
generator/generator | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/generator/generator b/generator/generator
index 896ad2a..bdd8fd7 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3532,7 +3532,8 @@ for how to get further details of the error.
Closes the handle and frees any associated resources. The final
status of any command that has not been retired (whether by
C<nbd_aio_command_completed> or by a low-level completion callback
-returning C<1>) is lost.
+returning C<1>) is lost. This function is not safe to call while
+any other thread is still using any C<nbd_*> API on the same handle.
=head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD
--
2.20.1
3:41 a.m.
On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake wrote:
Closing the handle in one thread while another thread is locked
causes
undefined behavior (as closing does not obtain a lock, and can cause
use-after-free or other nasty problems to the thread that does own the
lock). However, it is not sensible to try and obtain a lock in
nbd_close, as POSIX says that it is also undefined for any other
thread to wait on a mutex that has already been destroyed. Therefore,
we don't need to change our code, but merely remind users that
nbd_close is not safe until all other uses of the handle have ceased.
---
generator/generator | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/generator/generator b/generator/generator
index 896ad2a..bdd8fd7 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3532,7 +3532,8 @@ for how to get further details of the error.
Closes the handle and frees any associated resources. The final
status of any command that has not been retired (whether by
C<nbd_aio_command_completed> or by a low-level completion callback
-returning C<1>) is lost.
+returning C<1>) is lost. This function is not safe to call while
+any other thread is still using any C<nbd_*> API on the same handle.
=head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD
ACK.
Yes it's not safe to call nbd_close until all other uses of
the same handle from any other thread are over.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
3:56 a.m.
On Thu, Jul 25, 2019 at 09:41:01AM +0100, Richard W.M. Jones wrote:
On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake wrote:
> Closing the handle in one thread while another thread is locked causes
> undefined behavior (as closing does not obtain a lock, and can cause
> use-after-free or other nasty problems to the thread that does own the
> lock). However, it is not sensible to try and obtain a lock in
> nbd_close, as POSIX says that it is also undefined for any other
> thread to wait on a mutex that has already been destroyed. Therefore,
> we don't need to change our code, but merely remind users that
> nbd_close is not safe until all other uses of the handle have ceased.
> ---
>
> generator/generator | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/generator/generator b/generator/generator
> index 896ad2a..bdd8fd7 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3532,7 +3532,8 @@ for how to get further details of the error.
> Closes the handle and frees any associated resources. The final
> status of any command that has not been retired (whether by
> C<nbd_aio_command_completed> or by a low-level completion callback
> -returning C<1>) is lost.
> +returning C<1>) is lost. This function is not safe to call while
> +any other thread is still using any C<nbd_*> API on the same handle.
>
> =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD
ACK.
Yes it's not safe to call nbd_close until all other uses of
the same handle from any other thread are over.
Would it be too much of a trouble to add reference counting and give users a way
to "copy" of the handle? It wouldn't be a copy, but merely an increment on
the
reference counter. Or is it not worth doing that?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
4:15 a.m.
On Thu, Jul 25, 2019 at 10:56:02AM +0200, Martin Kletzander wrote:
On Thu, Jul 25, 2019 at 09:41:01AM +0100, Richard W.M. Jones wrote:
>On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake wrote:
>>Closing the handle in one thread while another thread is locked causes
>>undefined behavior (as closing does not obtain a lock, and can cause
>>use-after-free or other nasty problems to the thread that does own the
>>lock). However, it is not sensible to try and obtain a lock in
>>nbd_close, as POSIX says that it is also undefined for any other
>>thread to wait on a mutex that has already been destroyed. Therefore,
>>we don't need to change our code, but merely remind users that
>>nbd_close is not safe until all other uses of the handle have ceased.
>>---
>>
>> generator/generator | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>>diff --git a/generator/generator b/generator/generator
>>index 896ad2a..bdd8fd7 100755
>>--- a/generator/generator
>>+++ b/generator/generator
>>@@ -3532,7 +3532,8 @@ for how to get further details of the error.
>> Closes the handle and frees any associated resources. The final
>> status of any command that has not been retired (whether by
>> C<nbd_aio_command_completed> or by a low-level completion callback
>>-returning C<1>) is lost.
>>+returning C<1>) is lost. This function is not safe to call while
>>+any other thread is still using any C<nbd_*> API on the same handle.
>>
>> =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD
>
>ACK.
>
>Yes it's not safe to call nbd_close until all other uses of
>the same handle from any other thread are over.
>
Would it be too much of a trouble to add reference counting and give users a way
to "copy" of the handle? It wouldn't be a copy, but merely an increment on
the
reference counter. Or is it not worth doing that?
Reference counting is a huge PITA as well as making it much harder to
correctly write bindings in other languages, so I'd greatly prefer not
to do this.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
8:31 a.m.
On 7/25/19 3:56 AM, Martin Kletzander wrote:
On Thu, Jul 25, 2019 at 09:41:01AM +0100, Richard W.M. Jones wrote:
> On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake wrote:
>> Closing the handle in one thread while another thread is locked causes
>> undefined behavior (as closing does not obtain a lock, and can cause
>> use-after-free or other nasty problems to the thread that does own the
>> lock). However, it is not sensible to try and obtain a lock in
>> nbd_close, as POSIX says that it is also undefined for any other
>> thread to wait on a mutex that has already been destroyed. Therefore,
>> we don't need to change our code, but merely remind users that
>> nbd_close is not safe until all other uses of the handle have ceased.
>> ---
> Yes it's not safe to call nbd_close until all other uses of
> the same handle from any other thread are over.
>
Would it be too much of a trouble to add reference counting and give
users a way
to "copy" of the handle? It wouldn't be a copy, but merely an increment
on the
reference counter.
So each time you hand an nbd handle to another thread, that new thread
has to increment the counter, and then every thread must call nbd_close
when done with their copy (but only the last thread to do so actually
frees resources). It might help, but only if we are willing to go that
route...
Or is it not worth doing that?
...and as Rich says, managing refcounting ourselves even with language
bindings is harder than it looks (how do you guarantee the refcount is
increased when another language has other ways to share the handle
between threads?). The user can just as easily pthread_join before
calling nbd_close, at which point the problem is equally solved, but
with less burden on the library. I don't think we've locked ourselves
in a corner - if there is a compelling reason to add thread-safe close
via ref-counting later on, we can do it, and update the docs at that
time, but for now I'm fine living with the doc patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:20 a.m.
On Thu, Jul 25, 2019 at 08:31:03AM -0500, Eric Blake wrote:
On 7/25/19 3:56 AM, Martin Kletzander wrote:
> On Thu, Jul 25, 2019 at 09:41:01AM +0100, Richard W.M. Jones wrote:
>> On Wed, Jul 24, 2019 at 03:14:28PM -0500, Eric Blake wrote:
>>> Closing the handle in one thread while another thread is locked causes
>>> undefined behavior (as closing does not obtain a lock, and can cause
>>> use-after-free or other nasty problems to the thread that does own the
>>> lock). However, it is not sensible to try and obtain a lock in
>>> nbd_close, as POSIX says that it is also undefined for any other
>>> thread to wait on a mutex that has already been destroyed. Therefore,
>>> we don't need to change our code, but merely remind users that
>>> nbd_close is not safe until all other uses of the handle have ceased.
>>> ---
>> Yes it's not safe to call nbd_close until all other uses of
>> the same handle from any other thread are over.
>>
>
> Would it be too much of a trouble to add reference counting and give
> users a way
> to "copy" of the handle? It wouldn't be a copy, but merely an
increment
> on the
> reference counter.
So each time you hand an nbd handle to another thread, that new thread
has to increment the counter, and then every thread must call nbd_close
when done with their copy (but only the last thread to do so actually
frees resources). It might help, but only if we are willing to go that
route...
> Or is it not worth doing that?
...and as Rich says, managing refcounting ourselves even with language
bindings is harder than it looks (how do you guarantee the refcount is
increased when another language has other ways to share the handle
between threads?). The user can just as easily pthread_join before
calling nbd_close, at which point the problem is equally solved, but
with less burden on the library. I don't think we've locked ourselves
in a corner - if there is a compelling reason to add thread-safe close
via ref-counting later on, we can do it, and update the docs at that
time, but for now I'm fine living with the doc patch.
Thanks for the explanation both of you, it makes perfect sense and I agree.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
1871
days inactive
1872
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Martin Kletzander -
Richard W.M. Jones