[libvirt] [PATCHv2 8/9] phyp: avoid memory leaks in command values

Matthias Bolte matthias.bolte at googlemail.com
Fri Apr 15 20:56:19 UTC 2011


2011/4/14 Eric Blake <eblake at redhat.com>:
> * src/phyp/phyp_driver.c (phypExecBuffer): New function. Use it
> throughout file for less code, and for plugging a few leaks.
> ---
>
> Amazing how much copy-and-paste code this consolidated.
>
>  src/phyp/phyp_driver.c |  803 ++++++------------------------------------------
>  1 files changed, 92 insertions(+), 711 deletions(-)
>

> @@ -1823,17 +1706,8 @@ phypCreateServerSCSIAdapter(virConnectPtr conn)
>     virBufferVSprintf(&buf, " -r prof -i 'name=%s,lpar_id=%d,"
>                       "\"virtual_scsi_adapters=%s,%d/server/any/any/1\"'",
>                       vios_name, vios_id, ret, slot);
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
>     VIR_FREE(ret);
> -
> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (exit_status < 0 || ret == NULL)
>         goto cleanup;
> @@ -1847,17 +1721,7 @@ phypCreateServerSCSIAdapter(virConnectPtr conn)
>     virBufferVSprintf(&buf,
>                       " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"",
>                       vios_name, slot);
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
> -    VIR_FREE(ret);

Why are you killing the VIR_FREE(ret) here, in the hunk before you
left it in. It's necessary here too.

> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (exit_status < 0 || ret == NULL)
>         goto cleanup;


> @@ -2000,18 +1841,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>
>     if (system_type == HMC)
>         virBufferAddChar(&buf, '\'');
> -
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
> -    VIR_FREE(ret);

Same for this VIR_FREE(ret) here, removing it produces a leak, doesn't it.

> -
> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (exit_status < 0 || ret == NULL)
>         goto cleanup;
> @@ -2029,17 +1859,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>     virBufferVSprintf(&buf,
>                       " slot_num,backing_device|grep %s|cut -d, -f1",
>                       dev->data.disk->src);
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
> -    VIR_FREE(ret);

And this VIR_FREE(ret) needs to stay too.

> -
> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (exit_status < 0 || ret == NULL)
>         goto cleanup;
> @@ -2057,17 +1877,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>                       " -r prof --filter lpar_ids=%d,profile_names=%s"
>                       " -F virtual_scsi_adapters|sed -e 's/\"//g'",
>                       vios_id, profile);
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
> -    VIR_FREE(ret);

And this VIR_FREE(ret) needs to stay too.

> -
> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (exit_status < 0 || ret == NULL)
>         goto cleanup;
> @@ -2083,17 +1893,8 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>                       "\"virtual_scsi_adapters=%s,%d/client/%d/%s/0\"'",
>                       domain_name, domain->id, ret, slot,
>                       vios_id, vios_name);
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
>     VIR_FREE(ret);

This one is left in correctly.

> -
> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (virStrToLong_i(ret, &char_ptr, 10, &slot) == -1)
>         goto cleanup;
> @@ -2107,17 +1908,7 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>     virBufferVSprintf(&buf,
>                       " -p %s -o a -s %d -d 0 -a \"adapter_type=server\"",
>                       domain_name, slot);
> -    if (virBufferError(&buf)) {
> -        virBufferFreeAndReset(&buf);
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    VIR_FREE(cmd);
> -    VIR_FREE(ret);

But this VIR_FREE(ret) needs to stay.

> -
> -    cmd = virBufferContentAndReset(&buf);
> -    ret = phypExec(session, cmd, &exit_status, conn);
> +    ret = phypExecBuffer(session, &buf, &exit_status, conn, false);
>
>     if (exit_status < 0 || ret == NULL) {
>         VIR_ERROR0(_

ACK, with that VIR_FREEs left in.

Matthias




More information about the libvir-list mailing list