On 03/28/2011 02:07 PM, Eduardo Otubo wrote:
This is the implementation of the previous patch now using virInterface* API. Ended up this patch got much more simpler, smaller and easier to review. Here is some details:
+++ b/src/phyp/phyp_driver.c
static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags)
+ /* excluding interface */ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " --id %d -o r -s %d", lpar_id, slot_num); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + VIR_FREE(ret);
Redundant VIR_FREE.
+static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags) +{ + virCheckFlags(0, NULL); + + ConnectionData *connection_data = conn->networkPrivateData; + phyp_driverPtr phyp_driver = conn->privateData; + LIBSSH2_SESSION *session = connection_data->session; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *managed_system = phyp_driver->managed_system; + int system_type = phyp_driver->system_type; + int exit_status = 0; + char *char_ptr; + char *cmd = NULL; + int slot = 0; + char *ret = NULL; + char name[PHYP_IFACENAME_SIZE]; + char mac[PHYP_MAC_SIZE]; + virInterfaceDefPtr def; + + if (!(def = virInterfaceDefParseString(xml))) + goto err;
Memory leak. This allocates def...
+ + /* Now need to get the next free slot number */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " -Fslot_num --filter lpar_names=%s" + " |sort|tail -n 1", def->name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err;
but err: does not clean it up on at least this error path. Also, the regular path doesn't clean it up. You need virInterfaceDefFree(def) to avoid the leak.
+ } + cmd = virBufferContentAndReset(&buf);
Memory leak. This allocates cmd...
+ + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /* The next free slot itself: */ + slot++; + + /* Now adding the new network interface */ + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o a -s %d -a port_vlan_id=1," + "ieee_virtual_eth=0", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf);
but this silently overwrites it with a new allocation.
+ + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret != NULL) + goto err; + + /* Need to sleep a little while to wait for the HMC to + * complete the execution of the command. + * */ + sleep(1); + + /* Getting the new interface name */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot" + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*drc_name=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf);
as does this.
+ + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) { + /* roll back and excluding interface if error*/ + VIR_FREE(cmd); + VIR_FREE(ret); + + virBufferAddLit(&buf, "chhwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth" + " -p %s -o r -s %d", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + goto err;
Why are you building up a rollback command but never executing it?
+ } + + memcpy(name, ret, PHYP_IFACENAME_SIZE-1); + + /* Getting the new interface mac addr */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + "-r virtualio --rsubtype eth --level lpar " + " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; " + "s/^.*mac_addr=//'", def->name, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto err; + } + cmd = virBufferContentAndReset(&buf);
And yet another clobber of cmd.
+ + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + memcpy(mac, ret, PHYP_MAC_SIZE-1); + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, mac); + + err: + VIR_FREE(cmd); + VIR_FREE(ret); + return NULL; +} + +static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{
+ cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1) + goto err; + + /*Getting the lpar_id for the interface */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype slot --level slot " + " -F drc_name,lpar_id |" + " sed -n '/%s/ s/^.*,//p'", name); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf);
Yep, another leak. I'll quit pointing them out.
+ + VIR_FREE(ret); + + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL) + goto err; + + if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1) + goto err; + + /*Getting the interface mac */ + virBufferAddLit(&buf, "lshwres "); + if (system_type == HMC) + virBufferVSprintf(&buf, "-m %s ", managed_system); + + virBufferVSprintf(&buf, + " -r virtualio --rsubtype eth --level lpar " + " -F lpar_id,slot_num,mac_addr|" + " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL;
Oops; this should be goto err, or you leak.
+static int +phypListInterfaces(virConnectPtr conn, char **const names, int nnames) +{
+ while (got < nnames) { + char_ptr2 = strchr(networks, '\n'); + + if (char_ptr2) { + *char_ptr2 = '\0'; + if ((names[got++] = strdup(networks)) == NULL) { + virReportOOMError(); + goto err; + } + char_ptr2++; + networks = char_ptr2; + } else + break;
Style. HACKING states that a multi-line if followed by one-line else must have {} around the one-line side.
@@ -3795,6 +4344,7 @@ static virDomainPtr phypDomainCreateAndStart(virConnectPtr conn, const char *xml, unsigned int flags) { + virCheckFlags(0, NULL);
Unrelated to this patch, but I'll let it slide in. This patch has dragged on for long enough, so I'm applying the following fixes, and pushing (finally!): diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c index ad54693..b5145db 100644 --- i/src/phyp/phyp_driver.c +++ w/src/phyp/phyp_driver.c @@ -3382,12 +3382,10 @@ phypInterfaceDestroy(virInterfacePtr iface, if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return -1; + goto err; } cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - ret = phypExec(session, cmd, &exit_status, iface->conn); if (exit_status < 0 || ret != NULL) @@ -3456,6 +3454,9 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, slot++; /* Now adding the new network interface */ + VIR_FREE(cmd); + VIR_FREE(ret); + virBufferAddLit(&buf, "chhwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3472,8 +3473,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, } cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret != NULL) @@ -3485,6 +3484,9 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, sleep(1); /* Getting the new interface name */ + VIR_FREE(cmd); + VIR_FREE(ret); + virBufferAddLit(&buf, "lshwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3501,8 +3503,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, } cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) { @@ -3523,12 +3523,19 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, virReportOOMError(); goto err; } + + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn); goto err; } memcpy(name, ret, PHYP_IFACENAME_SIZE-1); /* Getting the new interface mac addr */ + VIR_FREE(cmd); + VIR_FREE(ret); + virBufferAddLit(&buf, "lshwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3545,8 +3552,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, } cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) @@ -3556,11 +3561,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const char *xml, VIR_FREE(cmd); VIR_FREE(ret); + virInterfaceDefFree(def); return virGetInterface(conn, name, mac); err: VIR_FREE(cmd); VIR_FREE(ret); + virInterfaceDefFree(def); return NULL; } @@ -3594,7 +3601,7 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); virReportOOMError(); - return NULL; + goto err; } cmd = virBufferContentAndReset(&buf); @@ -3607,6 +3614,9 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) goto err; /*Getting the lpar_id for the interface */ + VIR_FREE(cmd); + VIR_FREE(ret); + virBufferAddLit(&buf, "lshwres "); if (system_type == HMC) virBufferVSprintf(&buf, "-m %s ", managed_system); @@ -3623,8 +3633,6 @@ phypInterfaceLookupByName(virConnectPtr conn, const char *name) } cmd = virBufferContentAndReset(&buf); - VIR_FREE(ret); - ret = phypExec(session, cmd, &exit_status, conn); if (exit_status < 0 || ret == NULL) @@ -3772,8 +3780,9 @@ phypListInterfaces(virConnectPtr conn, char **const names, int nnames) } char_ptr2++; networks = char_ptr2; - } else + } else { break; + } } VIR_FREE(cmd); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org