On 12/14/2010 02:27 PM, Eric Blake wrote:
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:
I actually didn't know about this option. Will use as default options from now on, thanks.
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.
Ack, fixed.
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.
Ack, fixed.
+ } + 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.
Ack, fixed.
+ if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return -1; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, iface->conn);
Two more leaks of ret.
Ack, fixed.
+ +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).
Ack, fixed.
+ + 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.
Yes, exit_status can assume values less than zero. Actually HMC has lots of error message numbers, not exactly in the standards, so unfortunately int would fit better for this variable.
+ + ret = phypExec(session, cmd,&exit_status, conn);
Another case of leaking the old value of ret.
Ack, fixed.
+ + ret = phypExec(session, cmd,&exit_status, conn);
and another.
Ack, fixed.
+ + ret = phypExec(session, cmd,&exit_status, conn);
and another.
Ack, fixed.
+static virInterfacePtr +phypInterfaceLookupByName(virConnectPtr conn, const char *name) +{ + + ret = phypExec(session, cmd,&exit_status, conn);
+ ret = phypExec(session, cmd,&exit_status, conn);
and another.
Ack, fixed.
+ if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + cmd = virBufferContentAndReset(&buf); + + ret = phypExec(session, cmd,&exit_status, conn);
again
Ack, fixed.
+ + 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.
Ack, fixed.
+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.
Yes, legacy code probably, sorry I didn't noticed this earlier. Sending the PATCHv4 right away, keeping all the same comments Thanks for the review. Regards and happy new year. -- Eduardo Otubo Software Engineer Linux Technology Center IBM Systems & Technology Group Mobile: +55 19 8135 0885 eotubo@linux.vnet.ibm.com