11:25 a.m.
Without the following patch, this command would hang
printf 'domuuid fc4\ndomstate fc4\n' \
| ./virsh --connect test://$PWD/../docs/testnode.xml
with this stack trace:
__lll_lock_wait ...
_L_lock_105 ...
__pthread_mutex_lock ...
virUnrefDomain (domain=0x6a8b30) at hash.c:884
virDomainFree (domain=0x6a8b30) at libvirt.c:1211
cmdDomstate (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:677
vshCommandRun (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:4033
main (argc=3, argv=0x7fffea7234e8) at virsh.c:5015
The problem is that cmdDomuuid didn't call virDomainFree(dom), so
cmdDomstate hangs trying to do the Unref.
I then did a mind-numbing audit of all of the other cmd* functions in
virsh.c and found two other cases in which a "dom" may not be released.
Here's the patch:
Avoid virsh hang due to missing virDomainFree(dom) calls
* src/virsh.c (cmdDomuuid): Add missing virDomainFree call.
(cmdAttachDevice): Likewise.
(cmdDetachDevice): Likewise.
---
src/virsh.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/virsh.c b/src/virsh.c
index 0d93fb6..487f256 100644
--- a/src/virsh.c
+++ b/src/virsh.c
@@ -2161,6 +2161,7 @@ cmdDomuuid(vshControl * ctl, vshCmd * cmd)
else
vshError(ctl, FALSE, "%s", _("failed to get domain UUID"));
+ virDomainFree(dom);
return TRUE;
}
@@ -3038,8 +3039,10 @@ cmdAttachDevice(vshControl * ctl, vshCmd * cmd)
return FALSE;
}
- if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0)
+ if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
+ virDomainFree(dom);
return FALSE;
+ }
ret = virDomainAttachDevice(dom, buffer);
free (buffer);
@@ -3092,8 +3095,10 @@ cmdDetachDevice(vshControl * ctl, vshCmd * cmd)
return FALSE;
}
- if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0)
+ if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
+ virDomainFree(dom);
return FALSE;
+ }
ret = virDomainDetachDevice(dom, buffer);
free (buffer);
--
1.5.4.rc5.1.g0fa73
11:30 a.m.
On Wed, Jan 30, 2008 at 06:25:19PM +0100, Jim Meyering wrote:
Without the following patch, this command would hang
printf 'domuuid fc4\ndomstate fc4\n' \
| ./virsh --connect test://$PWD/../docs/testnode.xml
with this stack trace:
__lll_lock_wait ...
_L_lock_105 ...
__pthread_mutex_lock ...
virUnrefDomain (domain=0x6a8b30) at hash.c:884
virDomainFree (domain=0x6a8b30) at libvirt.c:1211
cmdDomstate (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:677
vshCommandRun (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:4033
main (argc=3, argv=0x7fffea7234e8) at virsh.c:501
The problem is that cmdDomuuid didn't call virDomainFree(dom), so
cmdDomstate hangs trying to do the Unref.
I don't understand why it would hang simply because it forgot to free
an object. Surely it ought to just be a memory leak not hang ? Each
individual libvirt API must always have a matched lock & unlock pair
in all codepaths, so a hang should only occur if an unlock is missing
in some codepath.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
11:36 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Wed, Jan 30, 2008 at 06:25:19PM +0100, Jim Meyering wrote:
> Without the following patch, this command would hang
>
> printf 'domuuid fc4\ndomstate fc4\n' \
> | ./virsh --connect test://$PWD/../docs/testnode.xml
>
> with this stack trace:
>
> __lll_lock_wait ...
> _L_lock_105 ...
> __pthread_mutex_lock ...
> virUnrefDomain (domain=0x6a8b30) at hash.c:884
> virDomainFree (domain=0x6a8b30) at libvirt.c:1211
> cmdDomstate (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:677
> vshCommandRun (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:4033
> main (argc=3, argv=0x7fffea7234e8) at virsh.c:501
>
> The problem is that cmdDomuuid didn't call virDomainFree(dom), so
> cmdDomstate hangs trying to do the Unref.
I don't understand why it would hang simply because it forgot to free
an object. Surely it ought to just be a memory leak not hang ? Each
individual libvirt API must always have a matched lock & unlock pair
in all codepaths, so a hang should only occur if an unlock is missing
in some codepath.
Well maybe virDomainFree is misnamed then.
It does more:
The missing virDomainFree calls cause that because
they are what is supposed to do the unlock.
virDomainFree calls virUnrefDomain, which calls virReleaseDomain
(when refs hits 0), which calls pthread_mutex_unlock.
11:44 a.m.
On Wed, Jan 30, 2008 at 06:36:37PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Wed, Jan 30, 2008 at 06:25:19PM +0100, Jim Meyering wrote:
>> Without the following patch, this command would hang
>>
>> printf 'domuuid fc4\ndomstate fc4\n' \
>> | ./virsh --connect test://$PWD/../docs/testnode.xml
>>
>> with this stack trace:
>>
>> __lll_lock_wait ...
>> _L_lock_105 ...
>> __pthread_mutex_lock ...
>> virUnrefDomain (domain=0x6a8b30) at hash.c:884
>> virDomainFree (domain=0x6a8b30) at libvirt.c:1211
>> cmdDomstate (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:677
>> vshCommandRun (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:4033
>> main (argc=3, argv=0x7fffea7234e8) at virsh.c:501
>>
>> The problem is that cmdDomuuid didn't call virDomainFree(dom), so
>> cmdDomstate hangs trying to do the Unref.
>
> I don't understand why it would hang simply because it forgot to free
> an object. Surely it ought to just be a memory leak not hang ? Each
> individual libvirt API must always have a matched lock & unlock pair
> in all codepaths, so a hang should only occur if an unlock is missing
> in some codepath.
Well maybe virDomainFree is misnamed then.
It does more:
The missing virDomainFree calls cause that because
they are what is supposed to do the unlock.
Not, its a bug in virUnrefDomain/Network - it calls mutex_lock() twice
in one codepath, instead of calling unlock().
Of course your patch to avoid the memory leak is still needed - so ACK
to that, but the locking flaw needs this patch:
Index: src/hash.c
===================================================================
RCS file: /data/cvs/libvirt/src/hash.c,v
retrieving revision 1.29
diff -u -p -r1.29 hash.c
--- src/hash.c 29 Jan 2008 18:15:54 -0000 1.29
+++ src/hash.c 30 Jan 2008 17:42:32 -0000
@@ -881,7 +881,7 @@ virUnrefDomain(virDomainPtr domain) {
return (0);
}
- pthread_mutex_lock(&domain->conn->lock);
+ pthread_mutex_unlock(&domain->conn->lock);
return (refs);
}
@@ -1013,7 +1013,7 @@ virUnrefNetwork(virNetworkPtr network) {
return (0);
}
- pthread_mutex_lock(&network->conn->lock);
+ pthread_mutex_unlock(&network->conn->lock);
return (refs);
}
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
noon
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Wed, Jan 30, 2008 at 06:36:37PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>
> > On Wed, Jan 30, 2008 at 06:25:19PM +0100, Jim Meyering wrote:
> >> Without the following patch, this command would hang
> >>
> >> printf 'domuuid fc4\ndomstate fc4\n' \
> >> | ./virsh --connect test://$PWD/../docs/testnode.xml
> >>
> >> with this stack trace:
> >>
> >> __lll_lock_wait ...
> >> _L_lock_105 ...
> >> __pthread_mutex_lock ...
> >> virUnrefDomain (domain=0x6a8b30) at hash.c:884
> >> virDomainFree (domain=0x6a8b30) at libvirt.c:1211
> >> cmdDomstate (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:677
> >> vshCommandRun (ctl=0x7fffea723390, cmd=0x6a8b10) at virsh.c:4033
> >> main (argc=3, argv=0x7fffea7234e8) at virsh.c:501
> >>
> >> The problem is that cmdDomuuid didn't call virDomainFree(dom), so
> >> cmdDomstate hangs trying to do the Unref.
> >
> > I don't understand why it would hang simply because it forgot to free
> > an object. Surely it ought to just be a memory leak not hang ? Each
> > individual libvirt API must always have a matched lock & unlock pair
> > in all codepaths, so a hang should only occur if an unlock is missing
> > in some codepath.
>
> Well maybe virDomainFree is misnamed then.
> It does more:
>
> The missing virDomainFree calls cause that because
> they are what is supposed to do the unlock.
Not, its a bug in virUnrefDomain/Network - it calls mutex_lock() twice
in one codepath, instead of calling unlock().
Of course your patch to avoid the memory leak is still needed - so ACK
to that, but the locking flaw needs this patch:
Index: src/hash.c
===================================================================
RCS file: /data/cvs/libvirt/src/hash.c,v
retrieving revision 1.29
diff -u -p -r1.29 hash.c
--- src/hash.c 29 Jan 2008 18:15:54 -0000 1.29
+++ src/hash.c 30 Jan 2008 17:42:32 -0000
@@ -881,7 +881,7 @@ virUnrefDomain(virDomainPtr domain) {
return (0);
}
- pthread_mutex_lock(&domain->conn->lock);
+ pthread_mutex_unlock(&domain->conn->lock);
return (refs);
}
@@ -1013,7 +1013,7 @@ virUnrefNetwork(virNetworkPtr network) {
return (0);
}
- pthread_mutex_lock(&network->conn->lock);
+ pthread_mutex_unlock(&network->conn->lock);
return (refs);
}
Oh! Two locks in a row.
Amazing that such a problem wasn't exposed sooner.
Ack.
12:21 p.m.
On Wed, Jan 30, 2008 at 07:00:30PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
>
> Not, its a bug in virUnrefDomain/Network - it calls mutex_lock() twice
> in one codepath, instead of calling unlock().
>
> Of course your patch to avoid the memory leak is still needed - so ACK
> to that, but the locking flaw needs this patch:
>
> Index: src/hash.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/hash.c,v
> retrieving revision 1.29
> diff -u -p -r1.29 hash.c
> --- src/hash.c 29 Jan 2008 18:15:54 -0000 1.29
> +++ src/hash.c 30 Jan 2008 17:42:32 -0000
> @@ -881,7 +881,7 @@ virUnrefDomain(virDomainPtr domain) {
> return (0);
> }
>
> - pthread_mutex_lock(&domain->conn->lock);
> + pthread_mutex_unlock(&domain->conn->lock);
> return (refs);
> }
>
> @@ -1013,7 +1013,7 @@ virUnrefNetwork(virNetworkPtr network) {
> return (0);
> }
>
> - pthread_mutex_lock(&network->conn->lock);
> + pthread_mutex_unlock(&network->conn->lock);
> return (refs);
> }
Oh! Two locks in a row.
Amazing that such a problem wasn't exposed sooner.
Well I only committed this flaw last week, so not much time to expose it.
Fixed in CVS now.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6142
days inactive
6142
days old
5 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Jim Meyering