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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list