[libvirt] [PATCH] Fix virsh {net,pool}-edit
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
Cole Robinson <crobinso@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@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
On Fri, Feb 13, 2009 at 04:58:17PM +0100, Jim Meyering wrote:
Cole Robinson <crobinso@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 :|
"Daniel P. Berrange" <berrange@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@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
On Fri, Feb 13, 2009 at 05:56:14PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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@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 :|
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 :|
"Daniel P. Berrange" <berrange@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. ;-)
Jim Meyering wrote:
"Daniel P. Berrange" <berrange@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. Urgh, I rather dislike this code generation of the other edit calls. It'd be much clearer if we justincluded the source for
On Thu, Feb 12, 2009 at 04:58:50PM -0500, Cole Robinson wrote: 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
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Jim Meyering