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

[Eric Blake]

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).

> 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.

> +    /* Getting the remote slot number */
> +
> +    char_ptr = NULL;
> +    char_ptr = strchr(iface->mac, '\n');

Redundant assignments (delete the first line).

> +    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];

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

s/addig/adding/

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

Another redundant assignment.

> +    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.

> +    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.

> +    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.

> +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.

> +    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.

>  
>  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).

-- 
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/20101209/d56b7014/attachment-0001.sig>