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

[Eduardo Otubo]

On 12/09/2010 07:16 PM, Eric Blake wrote:
> On 11/22/2010 06:03 PM, Eduardo Otubo wrote:
>
> Apologies for my review backlog (if you haven't guessed, I sometimes
> table big patches for later, then forget to come back rapidly).

No problem, I'll try to post earlier next time. Thanks anyway.

>
>> 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:
>>
>>   * MAC size and interface name are fixed due to specifications on HMC,
>>     both are created automatically and CAN'T be specified from user. They
>>     have the following format:
>>
>>      * MAC: 122980003002
>
> HMC represents MAC as a decimal number?  How hideous, but not your
> fault; and this comment in the commit message will be useful for anyone
> auditing the code in the future.

Yeah, no way I understand this notation either. Keeping all the commit 
comments on the PATCHv3 I'll send right away.

>
>> +    /* Getting the remote slot number */
>> +
>> +    char_ptr = NULL;
>> +    char_ptr = strchr(iface->mac, '\n');
>
> Redundant assignments (delete the first line).

ACK, fixed.

>
>> +    if (VIR_ALLOC_N(name, PHYP_IFACENAME_SIZE)<  0) {
>> +        virReportOOMError();
>> +        goto err;
>> +    }
>> +
>> +    if (VIR_ALLOC_N(mac, PHYP_MAC_SIZE)<  0) {
>> +        virReportOOMError();
>> +        goto err;
>> +    }
>
> Since these arrays are so small, it's faster to just stack-allocate them:
>
> char name[PHYP_IFACENAME_SIZE];

ACK, fixed.

>
>> +    /* The next free slot itself: */
>> +    slot++;
>> +
>> +    /* Now addig the new network interface */
>
> s/addig/adding/

ACK, fixed.

>
>> +    if (exit_status<  0 || ret == NULL)
>> +        goto err;
>> +
>> +    char_ptr = NULL;
>> +    char_ptr = strchr(ret, '\n');
>
> Another redundant assignment.

ACK, fixed.

>
>> +    if (memcpy(mac, ret, PHYP_IFACENAME_SIZE) == NULL)
>> +        goto err;
>> +
>> +    VIR_FREE(cmd);
>> +    VIR_FREE(ret);
>> +    return virGetInterface(conn, name, mac);
>> +
>> +  err:
>> +    VIR_FREE(name);
>> +    VIR_FREE(cmd);
>> +    VIR_FREE(ret);
>> +    return NULL;
>
> This leaks name if you end up calling virGetInterface.  And both paths
> leak mac.  But if you change name and mac to be stack-allocated, then
> you don't have to worry about freeing them.

ACK, fixed.

>
>> +    ret = phypExec(session, cmd,&exit_status, conn);
>> +
>> +    if (exit_status<  0 || ret == NULL)
>> +        goto err;
>> +
>> +    VIR_FREE(cmd);
>> +    return virGetInterface(conn, name, ret);
>> +
>> +  err:
>> +    VIR_FREE(cmd);
>> +    VIR_FREE(ret);
>> +    return NULL;
>
> This leaks ret if virGetInterface is called.

ACK, fixed.

>
>> +    ret = phypExec(session, cmd,&exit_status, iface->conn);
>> +
>> +    if (exit_status<  0 || ret == NULL)
>> +        goto err;
>> +
>> +    if (virStrToLong_i(ret,&char_ptr, 10,&state) == -1)
>> +        goto err;
>> +
>> +    if (char_ptr)
>> +        *char_ptr = '\0';
>> +
>> +    VIR_FREE(cmd);
>> +    return state;
>
> Another leak of ret.

ACK, fixed.

>
>> +static int
>> +phypListInterfaces(virConnectPtr conn, char **const names, int nnames)
>
> s/int/size_t/
>
>> +    ret = phypExec(session, cmd,&exit_status, conn);
>> +
>> +    /* I need to parse the textual return in order to get the network interfaces */
>> +    if (exit_status<  0 || ret == NULL)
>> +        goto err;
>> +    else {
>
> Rather than indenting the rest of the function inside an else{} block,
> you can leave the code at the top level indentation since the if() block
> was an unconditional goto.

ACK, fixed.

>
>> +    ret = phypExec(session, cmd,&exit_status, conn);
>> +
>> +    if (exit_status<  0 || ret == NULL)
>> +        goto err;
>> +
>> +    if (virStrToLong_i(ret,&char_ptr, 10,&nnets) == -1)
>> +        goto err;
>> +
>> +    if (char_ptr)
>> +        *char_ptr = '\0';
>> +
>> +    VIR_FREE(cmd);
>> +    return nnets;
>
> Another leak of ret.

ACK, fixed.

>
>>
>>   int
>> @@ -4117,7 +4651,7 @@ phypRegister(void)
>>           return -1;
>>       if (virRegisterStorageDriver(&phypStorageDriver)<  0)
>>           return -1;
>> -    if (virRegisterNetworkDriver(&phypNetworkDriver)<  0)
>> +    if (virRegisterInterfaceDriver(&phypInterfaceDriver)<  0)
>
> Are you intending to replace NetworkDriver with InterfaceDriver, or
> should you be supporting both drivers simultaneously (although this may
> be more an indication of how unfamiliar I am with the difference between
> what the two drivers are supposed to provide).

First option, I'll replace NetworkDriver with InterfaceDriver. I didn't 
know that there were an Interface API, my deep fault on this matter.

As I said, I will keep all the commit comments for further needs. 
Sending now the PATCHv3.

Thanks for the review.
Regards,

-- 
Eduardo Otubo
Software Engineer
Linux Technology Center
IBM Systems & Technology Group
Mobile: +55 19 8135 0885
eotubo at linux.vnet.ibm.com