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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list