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

[Eduardo Otubo]

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 at linux.vnet.ibm.com