3:58 p.m.
The dumpxml commands for networks and pools don't support any flag
arguments, and in fact explictly fail if flags != 0. This is not the
case for vm dumpxml though, and flags were added to the base 'edit'
implementation in virsh recently.
The net and pool derivatives were not addressing this difference, so any
net-edit or pool-edit attempt currently gives an error like:
Network error : invalid argument in virNetworkGetXMLDesc
The attached patch is one way to fix this. Thanks to Charles Duffy for
the report.
- Cole
9:58 a.m.
Cole Robinson <crobinso(a)redhat.com> wrote:
The dumpxml commands for networks and pools don't support any
flag
arguments, and in fact explictly fail if flags != 0. This is not the
case for vm dumpxml though, and flags were added to the base 'edit'
implementation in virsh recently.
The net and pool derivatives were not addressing this difference, so any
net-edit or pool-edit attempt currently gives an error like:
Network error : invalid argument in virNetworkGetXMLDesc
The attached patch is one way to fix this. Thanks to Charles Duffy for
the report.
Hi Cole,
That looks good to me.
I confirmed it fixes the bug when using the qemu-based driver:
cat <<EOF > net.xml
<network>
<name>N</name>
<ip address="192.168.199.1"
netmask="255.255.255.0"></ip>
</network>
EOF
printf '#!/bin/sh\nperl -pi -e "s/199/19/" "$@"\n' > ed;
chmod +x ed
qemud/libvirtd &
EDITOR=$PWD/ed src/virsh 'net-define net.xml; net-edit N'
However, using the test driver, my little test exposed a bug:
EDITOR=$PWD/ed src/virsh -c test:///default 'net-define net.xml; net-edit N'
zsh: segmentation fault
That was because the "def = NULL" assignment (to make it so
the upcoming "free" is a no-op) is too early, since there are
uses of def right after that.
And the same bug appears in 3 other nearby functions.
Here's the fix I'll commit soon, along with tests based on the above.
From 04bbdb86008744abd5e32cffb0573b873c9fc448 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Fri, 13 Feb 2009 16:49:50 +0100
Subject: [PATCH] test:///default driver: don't dereference NULL "def"
* src/test.c (testNetworkCreate, testNetworkDefine): Don't set
"def" to NULL until after the final use.
(testStoragePoolCreate, testStoragePoolDefine): Likewise.
---
src/test.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/test.c b/src/test.c
index 0e0ec7c..7e4e64d 100644
--- a/src/test.c
+++ b/src/test.c
@@ -2054,9 +2054,9 @@ static virNetworkPtr testNetworkCreate(virConnectPtr conn, const
char *xml) {
goto cleanup;
}
net->active = 1;
- def = NULL;
ret = virGetNetwork(conn, def->name, def->uuid);
+ def = NULL;
cleanup:
virNetworkDefFree(def);
@@ -2081,9 +2081,9 @@ static virNetworkPtr testNetworkDefine(virConnectPtr conn, const
char *xml) {
goto cleanup;
}
net->persistent = 1;
- def = NULL;
ret = virGetNetwork(conn, def->name, def->uuid);
+ def = NULL;
cleanup:
virNetworkDefFree(def);
@@ -2532,7 +2532,6 @@ testStoragePoolCreate(virConnectPtr conn,
if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) {
goto cleanup;
}
- def = NULL;
if (testStoragePoolObjSetDefaults(conn, pool) == -1) {
virStoragePoolObjRemove(&privconn->pools, pool);
@@ -2542,6 +2541,7 @@ testStoragePoolCreate(virConnectPtr conn,
pool->active = 1;
ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
+ def = NULL;
cleanup:
virStoragePoolDefFree(def);
@@ -2571,7 +2571,6 @@ testStoragePoolDefine(virConnectPtr conn,
if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) {
goto cleanup;
}
- def = NULL;
if (testStoragePoolObjSetDefaults(conn, pool) == -1) {
virStoragePoolObjRemove(&privconn->pools, pool);
@@ -2580,6 +2579,7 @@ testStoragePoolDefine(virConnectPtr conn,
}
ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
+ def = NULL;
cleanup:
virStoragePoolDefFree(def);
--
1.6.2.rc0.234.g2cc0b3
10:03 a.m.
On Fri, Feb 13, 2009 at 04:58:17PM +0100, Jim Meyering wrote:
Cole Robinson <crobinso(a)redhat.com> wrote:
> The dumpxml commands for networks and pools don't support any flag
> arguments, and in fact explictly fail if flags != 0. This is not the
> case for vm dumpxml though, and flags were added to the base 'edit'
> implementation in virsh recently.
>
> The net and pool derivatives were not addressing this difference, so any
> net-edit or pool-edit attempt currently gives an error like:
>
> Network error : invalid argument in virNetworkGetXMLDesc
>
> The attached patch is one way to fix this. Thanks to Charles Duffy for
> the report.
Hi Cole,
That looks good to me.
I confirmed it fixes the bug when using the qemu-based driver:
cat <<EOF > net.xml
<network>
<name>N</name>
<ip address="192.168.199.1"
netmask="255.255.255.0"></ip>
</network>
EOF
printf '#!/bin/sh\nperl -pi -e "s/199/19/" "$@"\n' > ed;
chmod +x ed
qemud/libvirtd &
EDITOR=$PWD/ed src/virsh 'net-define net.xml; net-edit N'
However, using the test driver, my little test exposed a bug:
EDITOR=$PWD/ed src/virsh -c test:///default 'net-define net.xml; net-edit N'
zsh: segmentation fault
That was because the "def = NULL" assignment (to make it so
the upcoming "free" is a no-op) is too early, since there are
uses of def right after that.
And the same bug appears in 3 other nearby functions.
Here's the fix I'll commit soon, along with tests based on the above.
This is not safe in general, because after the 'AssignDef' call, the
'def' is now owned by the 'obj'. If you don't set it to NULL
immediately
you have the risk of later error cleanup paths, seeing the non-NULL def
and free'ing it when they shouldn't.
The virGetNetwork() calls should be changed to call 'obj->def->name'
instead of just 'def->name'.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:56 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
This is not safe in general, because after the 'AssignDef'
call, the
'def' is now owned by the 'obj'. If you don't set it to NULL
immediately
you have the risk of later error cleanup paths, seeing the non-NULL def
and free'ing it when they shouldn't.
The virGetNetwork() calls should be changed to call 'obj->def->name'
instead of just 'def->name'.
Ok. that's safer, so I've adjusted the first two fixes.
The latter two functions weren't broken, but I've adjusted
the formatting to be consistent with single-stmt-no-curly-braces
convention. In these multiple-1000's-of-line files, every little
bit helps.
From baf2ec1ee6246f9e472b67d522676b8d41534525 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Fri, 13 Feb 2009 16:49:50 +0100
Subject: [PATCH] test:///default driver: don't dereference NULL "def"
* src/test.c (testNetworkCreate, testNetworkDefine): Since "def" is
set to NULL immediately after any vir*AssignDef call (to indicate we
no longer own it and to ensure no clean-up path mistakenly frees it),
dereference via net->def->, not def->.
---
src/test.c | 22 ++++++++--------------
1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/test.c b/src/test.c
index 0e0ec7c..226fe2e 100644
--- a/src/test.c
+++ b/src/test.c
@@ -2049,14 +2049,12 @@ static virNetworkPtr testNetworkCreate(virConnectPtr conn, const
char *xml) {
if ((def = virNetworkDefParseString(conn, xml)) == NULL)
goto cleanup;
- if ((net = virNetworkAssignDef(conn, &privconn->networks,
- def)) == NULL) {
+ if ((net = virNetworkAssignDef(conn, &privconn->networks, def)) == NULL)
goto cleanup;
- }
- net->active = 1;
def = NULL;
+ net->active = 1;
- ret = virGetNetwork(conn, def->name, def->uuid);
+ ret = virGetNetwork(conn, net->def->name, net->def->uuid);
cleanup:
virNetworkDefFree(def);
@@ -2076,14 +2074,12 @@ static virNetworkPtr testNetworkDefine(virConnectPtr conn, const
char *xml) {
if ((def = virNetworkDefParseString(conn, xml)) == NULL)
goto cleanup;
- if ((net = virNetworkAssignDef(conn, &privconn->networks,
- def)) == NULL) {
+ if ((net = virNetworkAssignDef(conn, &privconn->networks, def)) == NULL)
goto cleanup;
- }
- net->persistent = 1;
def = NULL;
+ net->persistent = 1;
- ret = virGetNetwork(conn, def->name, def->uuid);
+ ret = virGetNetwork(conn, net->def->name, net->def->uuid);
cleanup:
virNetworkDefFree(def);
@@ -2529,9 +2525,8 @@ testStoragePoolCreate(virConnectPtr conn,
goto cleanup;
}
- if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) {
+ if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def)))
goto cleanup;
- }
def = NULL;
if (testStoragePoolObjSetDefaults(conn, pool) == -1) {
@@ -2568,9 +2563,8 @@ testStoragePoolDefine(virConnectPtr conn,
def->allocation = defaultPoolAlloc;
def->available = defaultPoolCap - defaultPoolAlloc;
- if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) {
+ if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def)))
goto cleanup;
- }
def = NULL;
if (testStoragePoolObjSetDefaults(conn, pool) == -1) {
--
1.6.2.rc0.234.g2cc0b3
11:04 a.m.
On Fri, Feb 13, 2009 at 05:56:14PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
> This is not safe in general, because after the 'AssignDef' call, the
> 'def' is now owned by the 'obj'. If you don't set it to NULL
immediately
> you have the risk of later error cleanup paths, seeing the non-NULL def
> and free'ing it when they shouldn't.
>
> The virGetNetwork() calls should be changed to call 'obj->def->name'
> instead of just 'def->name'.
Ok. that's safer, so I've adjusted the first two fixes.
The latter two functions weren't broken, but I've adjusted
the formatting to be consistent with single-stmt-no-curly-braces
convention. In these multiple-1000's-of-line files, every little
bit helps.
From baf2ec1ee6246f9e472b67d522676b8d41534525 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Fri, 13 Feb 2009 16:49:50 +0100
Subject: [PATCH] test:///default driver: don't dereference NULL "def"
* src/test.c (testNetworkCreate, testNetworkDefine): Since "def" is
set to NULL immediately after any vir*AssignDef call (to indicate we
no longer own it and to ensure no clean-up path mistakenly frees it),
dereference via net->def->, not def->.
ACK, looks fine now.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
10:06 a.m.
On Thu, Feb 12, 2009 at 04:58:50PM -0500, Cole Robinson wrote:
The dumpxml commands for networks and pools don't support any
flag
arguments, and in fact explictly fail if flags != 0. This is not the
case for vm dumpxml though, and flags were added to the base 'edit'
implementation in virsh recently.
The net and pool derivatives were not addressing this difference, so any
net-edit or pool-edit attempt currently gives an error like:
Network error : invalid argument in virNetworkGetXMLDesc
The attached patch is one way to fix this. Thanks to Charles Duffy for
the report.
Urgh, I rather dislike this code generation of the other edit
calls. It'd be much clearer if we justincluded the source for
them directly and had shared helper methods, rather than munging
source with perl.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:13 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Thu, Feb 12, 2009 at 04:58:50PM -0500, Cole Robinson wrote:
> The dumpxml commands for networks and pools don't support any flag
> arguments, and in fact explictly fail if flags != 0. This is not the
> case for vm dumpxml though, and flags were added to the base 'edit'
> implementation in virsh recently.
>
> The net and pool derivatives were not addressing this difference, so any
> net-edit or pool-edit attempt currently gives an error like:
>
> Network error : invalid argument in virNetworkGetXMLDesc
>
> The attached patch is one way to fix this. Thanks to Charles Duffy for
> the report.
Urgh, I rather dislike this code generation of the other edit
calls. It'd be much clearer if we justincluded the source for
them directly and had shared helper methods, rather than munging
source with perl.
Hi Cole,
I just chatted with Daniel off-line and he actually did like that patch,
so you're welcome to commit it. ;-)
4:49 p.m.
Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Thu, Feb 12, 2009 at 04:58:50PM -0500, Cole Robinson wrote:
>> The dumpxml commands for networks and pools don't support any flag
>> arguments, and in fact explictly fail if flags != 0. This is not the
>> case for vm dumpxml though, and flags were added to the base 'edit'
>> implementation in virsh recently.
>>
>> The net and pool derivatives were not addressing this difference, so any
>> net-edit or pool-edit attempt currently gives an error like:
>>
>> Network error : invalid argument in virNetworkGetXMLDesc
>>
>> The attached patch is one way to fix this. Thanks to Charles Duffy for
>> the report.
> Urgh, I rather dislike this code generation of the other edit
> calls. It'd be much clearer if we justincluded the source for
> them directly and had shared helper methods, rather than munging
> source with perl.
Hi Cole,
I just chatted with Daniel off-line and he actually did like that patch,
so you're welcome to commit it. ;-)
Thanks, pushed now.
- Cole
5793
days inactive
5797
days old
7 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering