[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4] PHYP: Adding network interface management



On 12/29/2010 11:04 AM, 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:

Finally getting around to reviewing this again.  I'm not intentionally
putting you off, it's just that I'm less familiar with phyp and it takes
a good chunk of free time to review large patches in areas where I'm
less familiar.

> @@ -74,6 +75,11 @@
>  
>  static unsigned const int HMC = 0;
>  static unsigned const int IVM = 127;
> +static unsigned const int PHYP_IFACENAME_SIZE = 24;
> +static unsigned const int PHYP_MAC_SIZE= 12;
> +
> +
> +virCheckFlags(0,NULL);

This is misplaced, and causes a compile error.  It should be one of the
first statements (not declarations) in any function that takes a flags
argument where you do not otherwise use flags, and not something done at
the file scope.

>  
>  static int
>  waitsocket(int socket_fd, LIBSSH2_SESSION * session)
> @@ -1113,7 +1119,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
>  
>  static virDrvOpenStatus
>  phypOpen(virConnectPtr conn,
> -         virConnectAuthPtr auth, int flags ATTRIBUTE_UNUSED)
> +         virConnectAuthPtr auth, int flags)
>  {
>      LIBSSH2_SESSION *session = NULL;
>      ConnectionData *connection_data = NULL;

That is, somewhere in this function.

Seeing as how this causes a compile error:

  CC     libvirt_driver_phyp_la-phyp_driver.lo
phyp/phyp_driver.c:82:1: error: expected identifier or '(' before 'do'
phyp/phyp_driver.c:82:290: error: expected identifier or '(' before 'while'
cc1: warnings being treated as errors
phyp/phyp_driver.c: In function 'phypOpen':
phyp/phyp_driver.c:1122:38: error: unused parameter 'flags'
[-Wunused-parameter]
phyp/phyp_driver.c: In function 'phypInterfaceDestroy':

how can I even be sure that you've tested your patch, to spend my time
reviewing it?

> +static virInterfacePtr
> +phypInterfaceDefineXML(virConnectPtr conn, const char *xml,
> +                       unsigned int flags)
> +{

> +    /* Now adding the new network interface */
> +    virBufferAddLit(&buf, "chhwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype eth"
> +                      " -p %s -o a -s %d -a port_vlan_id=1,"
> +                      "ieee_virtual_eth=0", def->name, slot);

So if this succeeds,

> +    /* Need to sleep a little while to wait for the HMC to
> +     * complete the execution of the command.
> +     * */
> +    sleep(1);
> +
> +    /* Getting the new interface name */
> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype slot --level slot"
> +                      " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; "
> +                      "s/^.*drc_name=//'", def->name, slot);

but this fails, do you need to undo the reservation, or have you just
leaked a resource?

> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        goto err;
> +    }
> +    cmd = virBufferContentAndReset(&buf);
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    char_ptr = strchr(ret, '\n');
> +
> +    if (char_ptr)
> +        *char_ptr = '\0';
> +
> +    if (memcpy(name, ret, PHYP_IFACENAME_SIZE-1) == NULL)
> +        goto err;

Huh?  memcpy() can never fail on valid input.  No need for this if
statement; just do the memcpy() and ignore the result (which will be
name).  Multiple times in this patch.

> +
> +static virInterfacePtr
> +phypInterfaceLookupByName(virConnectPtr conn, const char *name)
> +{

> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (memcpy(mac, ret, PHYP_MAC_SIZE-1) == NULL)
> +        goto err;
> +
> +    VIR_FREE(cmd);
> +    return virGetInterface(conn, name, ret);

Memory leak; you never freed ret.  You need to do something like:

result = virGetInterface(conn, name, ret);
VIR_FREE(ret);
return result;

> +static int
> +phypInterfaceIsActive(virInterfacePtr iface)
> +{

> +
> +    if (virStrToLong_i(ret, &char_ptr, 10, &state) == -1)
> +        goto err;
> +
> +    if (char_ptr)
> +        *char_ptr = '\0';
> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return state;

The lines involving setting *char_ptr to '\0' are useless, and can
safely be deleted.  No need to NUL-terminate the integer portion of ret
if you are just about to free it.

Getting closer, but still not ready to merge in.  Hopefully v5 will
clinch it.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]