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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101214/80394103/attachment-0001.sig>