On 12/13/2010 01:09 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:
You may want to use --enable-compile-warnings=error at autogen.sh time, since it would have caught this bug: CC libvirt_driver_phyp_la-phyp_driver.lo cc1: warnings being treated as errors phyp/phyp_driver.c:4633:5: error: initialization from incompatible pointer type make[2]: *** [libvirt_driver_phyp_la-phyp_driver.lo] Error 1 Fixed by making phypListInterfaces return int.
static int +phypInterfaceDestroy(virInterfacePtr iface, + unsigned int flags ATTRIBUTE_UNUSED) +{
+ ret = phypExec(session, cmd, &exit_status, iface->conn); +
+ + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1;
Memory leak for ret; this should goto err rather than return.
+ } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn);
Memory leak for the old value of ret; you need to VIR_FREE the old value before starting a new phypExec, or track the exec's in separate strings all of which get freed at the end.
+ if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, iface->conn);
Two more leaks of ret.
+ +static virInterfacePtr +phypInterfaceDefineXML(virConnectPtr conn, const char *xml, + unsigned int flags ATTRIBUTE_UNUSED) +{ + 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; +
Rather than marking flags as unused, it would be better to insert virCheckFlags(0,NULL).
+ + ret = phypExec(session, cmd, &exit_status, conn); + + if (exit_status < 0 || ret == NULL)
Can exit_status ever be less than 0? Or does it reflect the same values as a process exit status, where things like WIFEXITED apply, and where the result will always be in the range of uint16_t? But that's a question for the entire file, and not just this patch.
+ + ret = phypExec(session, cmd, &exit_status, conn);
Another case of leaking the old value of ret.
+ + ret = phypExec(session, cmd, &exit_status, conn);
and another.
+ + ret = phypExec(session, cmd, &exit_status, conn);
and another.
+static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{ + + ret = phypExec(session, cmd, &exit_status, conn);
+ ret = phypExec(session, cmd, &exit_status, conn);
and another.
+ if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd, &exit_status, conn);
again
+ + if (exit_status < 0 || ret == NULL) + goto err; + + if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL) + goto err; + + VIR_FREE(cmd); + VIR_FREE(ret); + return virGetInterface(conn, name, ret);
NULL pointer dereference, because you're trying to use ret after freeing it.
+static int +phypNumOfInterfaces(virConnectPtr conn) +{
+ + if (virStrToLong_i(ret, &char_ptr, 10, &nnets) == -1) + goto err; + + if (char_ptr) + *char_ptr = '\0';
What's this for? You don't use char_ptr in the rest of the function, and you're about to free ret, which is where char_ptr points. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org