Re: [libvirt] [PATCHv3] PHYP: Adding network interface management

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.