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

Re: [libvirt] [PATCH v2] Merge all returns paths from dispatcher into single path



On 04/14/2011 07:28 AM, Daniel P. Berrange wrote:
> The dispatcher functions have numerous places where they
> return to the caller. This leads to duplicated cleanup
> code, often resulting in memory leaks. It makes it harder
> to ensure that errors are dispatched before freeing objects,
> which may overwrite the original error.
> 

Rather than review your entire patch again, I just reviewed this interdiff.

You really did get rid of all remoteDispatchOOMError callers; I'd be
fine if you squashed in deleting the declaration from
daemon/dispatch.[ch] as part of this patch, or as a followup.

> diff --git c/daemon/remote.c w/daemon/remote.c
> index 8f4d6a6..a25c095 100644
> --- c/daemon/remote.c
> +++ w/daemon/remote.c
> @@ -912,7 +912,7 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
>                                             remote_domain_get_scheduler_parameters_ret *ret)
>  {
>      virDomainPtr dom = NULL;
> -    virSchedParameterPtr params;
> +    virSchedParameterPtr params = NULL;
>      int i, r, nparams;
>      int rv = -1;
> 
> @@ -927,10 +927,8 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
>          virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
>          goto cleanup;
>      }
> -    if (VIR_ALLOC_N(params, nparams) < 0) {
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> +    if (VIR_ALLOC_N(params, nparams) < 0)
> +        goto no_memory;
> 
>      dom = get_nonnull_domain(conn, args->dom);
>      if (dom == NULL) {
> @@ -1002,7 +1000,7 @@ remoteDispatchDomainSetSchedulerParameters(struct qemud_server *server ATTRIBUTE
>  {
>      virDomainPtr dom = NULL;
>      int i, r, nparams;
> -    virSchedParameterPtr params;
> +    virSchedParameterPtr params = NULL;
>      int rv = -1;
> 
>      if (!conn) {
> @@ -1960,7 +1958,7 @@ remoteDispatchDomainGetSecurityLabel(struct qemud_server *server ATTRIBUTE_UNUSE
>                                       remote_domain_get_security_label_ret *ret)
>  {
>      virDomainPtr dom = NULL;
> -    virSecurityLabelPtr seclabel;
> +    virSecurityLabelPtr seclabel = NULL;
>      int rv = -1;
> 
>      if (!conn) {
> @@ -3041,7 +3039,7 @@ remoteDispatchDomainSetMemoryParameters(struct qemud_server *server
>  {
>      virDomainPtr dom = NULL;
>      int i, r, nparams;
> -    virMemoryParameterPtr params;
> +    virMemoryParameterPtr params = NULL;
>      unsigned int flags;
>      int rv = -1;
> 
> @@ -3142,7 +3140,7 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
>                                          * ret)
>  {
>      virDomainPtr dom = NULL;
> -    virMemoryParameterPtr params;
> +    virMemoryParameterPtr params = NULL;
>      int i, r, nparams;
>      unsigned int flags;
>      int rv = -1;
> @@ -3233,8 +3231,6 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
>  success:
>      rv = 0;
> 
> -no_memory:
> -    virReportOOMError();
>  cleanup:
>      if (rv < 0) {
>          remoteDispatchError(rerr);
> @@ -3246,6 +3242,10 @@ cleanup:
>          virDomainFree(dom);
>      VIR_FREE(params);
>      return rv;
> +
> +no_memory:
> +    virReportOOMError();
> +    goto cleanup;
>  }

Good, this addresses my review comments.

> 
>  static int
> @@ -3262,7 +3262,7 @@ remoteDispatchDomainSetBlkioParameters(struct qemud_server *server
>  {
>      virDomainPtr dom = NULL;
>      int i, r, nparams;
> -    virBlkioParameterPtr params;
> +    virBlkioParameterPtr params = NULL;
>      unsigned int flags;
>      int rv = -1;
> 
> @@ -3363,7 +3363,7 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
>                                          * ret)
>  {
>      virDomainPtr dom = NULL;
> -    virBlkioParameterPtr params;
> +    virBlkioParameterPtr params = NULL;
>      int i, r, nparams;
>      unsigned int flags;
>      int rv = -1;
> @@ -4846,9 +4846,8 @@ remoteDispatchAuthSaslInit(struct qemud_server *server,
>                      virStrerror(errno, ebuf, sizeof ebuf));
>          goto error;
>      }
> -    if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) {
> +    if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL)
>          goto error;
> -    }

Looks like the cleanups from one of your followup patches leaked in here
when you reverted your sasl changes, but no real harm leaving this hunk in.

> 
>      /* Get remote address in form  IPADDR:PORT */
>      sa.len = sizeof(sa.data.stor);
> @@ -4964,7 +4963,7 @@ authfail:
>  error:
>      PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_SASL);
>      virMutexUnlock(&client->lock);
> -    goto error;
> +    return -1;
>  }
> 
> 
> @@ -5126,6 +5125,7 @@ remoteDispatchAuthSaslStart(struct qemud_server *server,
>      if (serverout) {
>          if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
>              virReportOOMError();
> +            remoteDispatchError(rerr);
>              goto error;
>          }
>          memcpy(ret->data.data_val, serverout, serveroutlen);
> @@ -5227,6 +5227,7 @@ remoteDispatchAuthSaslStep(struct qemud_server *server,
>      if (serverout) {
>          if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
>              virReportOOMError();
> +            remoteDispatchError(rerr);
>              goto error;
>          }
>          memcpy(ret->data.data_val, serverout, serveroutlen);
> @@ -5403,7 +5404,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server,
>      client->auth = REMOTE_AUTH_NONE;
> 
>      virMutexUnlock(&client->lock);
> -    rv = 0;
> +    return 0;
> 
>  authfail:
>      PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);
> @@ -5535,7 +5536,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server,
>      client->auth = REMOTE_AUTH_NONE;
> 
>      virMutexUnlock(&client->lock);
> -    rv = 0;
> +    return 0;

Looks like you correctly reverted any changes to sasl code, as planned.

> 
>  authfail:
>      PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);
> @@ -6964,7 +6965,7 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED,
>                                    remote_node_device_get_parent_ret *ret)
>  {
>      virNodeDevicePtr dev = NULL;
> -    const char *parent;
> +    const char *parent = NULL;
>      int rv = -1;
> 
>      if (!conn) {
> @@ -6988,8 +6989,8 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED,
>              virReportOOMError();
>              goto cleanup;
>          }
> -        *parent_p = strdup(parent);
> -        if (*parent_p == NULL) {
> +        if (!(*parent_p = strdup(parent))) {
> +            VIR_FREE(parent_p);
>              virReportOOMError();
>              goto cleanup;
>          }

For all my waffling reviews, I think you got this one right now.

> @@ -8413,7 +8414,7 @@ remoteDispatchDomainSnapshotCreateXml(struct qemud_server *server ATTRIBUTE_UNUS
>                                        remote_domain_snapshot_create_xml_args *args,
>                                        remote_domain_snapshot_create_xml_ret *ret)
>  {
> -    virDomainSnapshotPtr snapshot;
> +    virDomainSnapshotPtr snapshot = NULL;
>      virDomainPtr domain = NULL;
>      int rv = -1;
> 
> @@ -8589,7 +8590,7 @@ remoteDispatchDomainSnapshotLookupByName(struct qemud_server *server ATTRIBUTE_U
>                                           remote_domain_snapshot_lookup_by_name_args *args,
>                                           remote_domain_snapshot_lookup_by_name_ret *ret)
>  {
> -    virDomainSnapshotPtr snapshot;
> +    virDomainSnapshotPtr snapshot = NULL;
>      virDomainPtr domain = NULL;
>      int rv = -1;
> 
> @@ -8671,7 +8672,7 @@ remoteDispatchDomainSnapshotCurrent(struct qemud_server *server ATTRIBUTE_UNUSED
>                                      remote_domain_snapshot_current_args *args,
>                                      remote_domain_snapshot_current_ret *ret)
>  {
> -    virDomainSnapshotPtr snapshot;
> +    virDomainSnapshotPtr snapshot = NULL;
>      virDomainPtr domain = NULL;
>      int rv = -1;
> 

ACK - you addressed all my findings from v1.

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