Re: [libvirt] [PATCH] Fix virsh {net,pool}-edit

[Jim Meyering <jim@xxxxxxxxxxxx>]

Cole Robinson <crobinso@xxxxxxxxxx> 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@xxxxxxxxxx>
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

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list