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

Re: [libvirt] [PATCHv6] PHYP: Adding network interface



On 03/28/2011 02:07 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:

> +++ b/src/phyp/phyp_driver.c

>  static int
> +phypInterfaceDestroy(virInterfacePtr iface,
> +                     unsigned int flags)

> +    /* excluding interface */
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +
> +    virBufferAddLit(&buf, "chhwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype eth"
> +                      " --id %d -o r -s %d", lpar_id, slot_num);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        return -1;
> +    }
> +    cmd = virBufferContentAndReset(&buf);
> +
> +    VIR_FREE(ret);

Redundant VIR_FREE.

> +static virInterfacePtr
> +phypInterfaceDefineXML(virConnectPtr conn, const char *xml,
> +                       unsigned int flags)
> +{
> +    virCheckFlags(0, NULL);
> +
> +    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;
> +
> +    if (!(def = virInterfaceDefParseString(xml)))
> +        goto err;

Memory leak.  This allocates def...

> +
> +    /* Now need to get the next free slot number */
> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype slot --level slot"
> +                      " -Fslot_num --filter lpar_names=%s"
> +                      " |sort|tail -n 1", def->name);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        goto err;

but err: does not clean it up on at least this error path.  Also, the
regular path doesn't clean it up.  You need virInterfaceDefFree(def) to
avoid the leak.

> +    }
> +    cmd = virBufferContentAndReset(&buf);

Memory leak.  This allocates cmd...

> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> +        goto err;
> +
> +    /* The next free slot itself: */
> +    slot++;
> +
> +    /* 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);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        goto err;
> +    }
> +    cmd = virBufferContentAndReset(&buf);

but this silently overwrites it with a new allocation.

> +
> +    VIR_FREE(ret);
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret != NULL)
> +        goto err;
> +
> +    /* 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);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        goto err;
> +    }
> +    cmd = virBufferContentAndReset(&buf);

as does this.

> +
> +    VIR_FREE(ret);
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL) {
> +        /* roll back and excluding interface if error*/
> +        VIR_FREE(cmd);
> +        VIR_FREE(ret);
> +
> +        virBufferAddLit(&buf, "chhwres ");
> +        if (system_type == HMC)
> +            virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +        virBufferVSprintf(&buf,
> +                " -r virtualio --rsubtype eth"
> +                " -p %s -o r -s %d", def->name, slot);
> +
> +        if (virBufferError(&buf)) {
> +            virBufferFreeAndReset(&buf);
> +            virReportOOMError();
> +            goto err;
> +        }
> +        goto err;

Why are you building up a rollback command but never executing it?

> +    }
> +
> +    memcpy(name, ret, PHYP_IFACENAME_SIZE-1);
> +
> +    /* Getting the new interface mac addr */
> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      "-r virtualio --rsubtype eth --level lpar "
> +                      " |sed '/lpar_name=%s/!d; /slot_num=%d/!d; "
> +                      "s/^.*mac_addr=//'", def->name, slot);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        goto err;
> +    }
> +    cmd = virBufferContentAndReset(&buf);

And yet another clobber of cmd.

> +
> +    VIR_FREE(ret);
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    memcpy(mac, ret, PHYP_MAC_SIZE-1);
> +
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return virGetInterface(conn, name, mac);
> +
> +  err:
> +    VIR_FREE(cmd);
> +    VIR_FREE(ret);
> +    return NULL;
> +}
> +
> +static virInterfacePtr
> +phypInterfaceLookupByName(virConnectPtr conn, const char *name)
> +{

> +    cmd = virBufferContentAndReset(&buf);
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
> +        goto err;
> +
> +    /*Getting the lpar_id for the interface */
> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype slot --level slot "
> +                      " -F drc_name,lpar_id |"
> +                      " sed -n '/%s/ s/^.*,//p'", name);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        return NULL;
> +    }
> +    cmd = virBufferContentAndReset(&buf);

Yep, another leak.  I'll quit pointing them out.

> +
> +    VIR_FREE(ret);
> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret == NULL)
> +        goto err;
> +
> +    if (virStrToLong_i(ret, &char_ptr, 10, &lpar_id) == -1)
> +        goto err;
> +
> +    /*Getting the interface mac */
> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype eth --level lpar "
> +                      " -F lpar_id,slot_num,mac_addr|"
> +                      " sed -n '/%d,%d/ s/^.*,//p'", lpar_id, slot);
> +
> +    if (virBufferError(&buf)) {
> +        virBufferFreeAndReset(&buf);
> +        virReportOOMError();
> +        return NULL;

Oops; this should be goto err, or you leak.

> +static int
> +phypListInterfaces(virConnectPtr conn, char **const names, int nnames)
> +{

> +    while (got < nnames) {
> +        char_ptr2 = strchr(networks, '\n');
> +
> +        if (char_ptr2) {
> +            *char_ptr2 = '\0';
> +            if ((names[got++] = strdup(networks)) == NULL) {
> +                virReportOOMError();
> +                goto err;
> +            }
> +            char_ptr2++;
> +            networks = char_ptr2;
> +        } else
> +            break;

Style.  HACKING states that a multi-line if followed by one-line else
must have {} around the one-line side.

> @@ -3795,6 +4344,7 @@ static virDomainPtr
>  phypDomainCreateAndStart(virConnectPtr conn,
>                           const char *xml, unsigned int flags)
>  {
> +    virCheckFlags(0, NULL);

Unrelated to this patch, but I'll let it slide in.

This patch has dragged on for long enough, so I'm applying the following
fixes, and pushing (finally!):

diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c
index ad54693..b5145db 100644
--- i/src/phyp/phyp_driver.c
+++ w/src/phyp/phyp_driver.c
@@ -3382,12 +3382,10 @@ phypInterfaceDestroy(virInterfacePtr iface,
     if (virBufferError(&buf)) {
         virBufferFreeAndReset(&buf);
         virReportOOMError();
-        return -1;
+        goto err;
     }
     cmd = virBufferContentAndReset(&buf);

-    VIR_FREE(ret);
-
     ret = phypExec(session, cmd, &exit_status, iface->conn);

     if (exit_status < 0 || ret != NULL)
@@ -3456,6 +3454,9 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,
     slot++;

     /* Now adding the new network interface */
+    VIR_FREE(cmd);
+    VIR_FREE(ret);
+
     virBufferAddLit(&buf, "chhwres ");
     if (system_type == HMC)
         virBufferVSprintf(&buf, "-m %s ", managed_system);
@@ -3472,8 +3473,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,
     }
     cmd = virBufferContentAndReset(&buf);

-    VIR_FREE(ret);
-
     ret = phypExec(session, cmd, &exit_status, conn);

     if (exit_status < 0 || ret != NULL)
@@ -3485,6 +3484,9 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,
     sleep(1);

     /* Getting the new interface name */
+    VIR_FREE(cmd);
+    VIR_FREE(ret);
+
     virBufferAddLit(&buf, "lshwres ");
     if (system_type == HMC)
         virBufferVSprintf(&buf, "-m %s ", managed_system);
@@ -3501,8 +3503,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,
     }
     cmd = virBufferContentAndReset(&buf);

-    VIR_FREE(ret);
-
     ret = phypExec(session, cmd, &exit_status, conn);

     if (exit_status < 0 || ret == NULL) {
@@ -3523,12 +3523,19 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,
             virReportOOMError();
             goto err;
         }
+
+        cmd = virBufferContentAndReset(&buf);
+
+        ret = phypExec(session, cmd, &exit_status, conn);
         goto err;
     }

     memcpy(name, ret, PHYP_IFACENAME_SIZE-1);

     /* Getting the new interface mac addr */
+    VIR_FREE(cmd);
+    VIR_FREE(ret);
+
     virBufferAddLit(&buf, "lshwres ");
     if (system_type == HMC)
         virBufferVSprintf(&buf, "-m %s ", managed_system);
@@ -3545,8 +3552,6 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,
     }
     cmd = virBufferContentAndReset(&buf);

-    VIR_FREE(ret);
-
     ret = phypExec(session, cmd, &exit_status, conn);

     if (exit_status < 0 || ret == NULL)
@@ -3556,11 +3561,13 @@ phypInterfaceDefineXML(virConnectPtr conn, const
char *xml,

     VIR_FREE(cmd);
     VIR_FREE(ret);
+    virInterfaceDefFree(def);
     return virGetInterface(conn, name, mac);

   err:
     VIR_FREE(cmd);
     VIR_FREE(ret);
+    virInterfaceDefFree(def);
     return NULL;
 }

@@ -3594,7 +3601,7 @@ phypInterfaceLookupByName(virConnectPtr conn,
const char *name)
     if (virBufferError(&buf)) {
         virBufferFreeAndReset(&buf);
         virReportOOMError();
-        return NULL;
+        goto err;
     }
     cmd = virBufferContentAndReset(&buf);

@@ -3607,6 +3614,9 @@ phypInterfaceLookupByName(virConnectPtr conn,
const char *name)
         goto err;

     /*Getting the lpar_id for the interface */
+    VIR_FREE(cmd);
+    VIR_FREE(ret);
+
     virBufferAddLit(&buf, "lshwres ");
     if (system_type == HMC)
         virBufferVSprintf(&buf, "-m %s ", managed_system);
@@ -3623,8 +3633,6 @@ phypInterfaceLookupByName(virConnectPtr conn,
const char *name)
     }
     cmd = virBufferContentAndReset(&buf);

-    VIR_FREE(ret);
-
     ret = phypExec(session, cmd, &exit_status, conn);

     if (exit_status < 0 || ret == NULL)
@@ -3772,8 +3780,9 @@ phypListInterfaces(virConnectPtr conn, char
**const names, int nnames)
             }
             char_ptr2++;
             networks = char_ptr2;
-        } else
+        } else {
             break;
+        }
     }

     VIR_FREE(cmd);


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