12:21 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org