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.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)