Return-Path: X-Original-To: apmail-subversion-commits-archive@minotaur.apache.org Delivered-To: apmail-subversion-commits-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id AF4D999EE for ; Thu, 23 Feb 2012 13:44:37 +0000 (UTC) Received: (qmail 48997 invoked by uid 500); 23 Feb 2012 13:44:37 -0000 Delivered-To: apmail-subversion-commits-archive@subversion.apache.org Received: (qmail 48972 invoked by uid 500); 23 Feb 2012 13:44:37 -0000 Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@subversion.apache.org Delivered-To: mailing list commits@subversion.apache.org Received: (qmail 48964 invoked by uid 99); 23 Feb 2012 13:44:37 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 23 Feb 2012 13:44:37 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=5.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 23 Feb 2012 13:44:34 +0000 Received: from eris.apache.org (localhost [127.0.0.1]) by eris.apache.org (Postfix) with ESMTP id 2912A23889B8 for ; Thu, 23 Feb 2012 13:44:13 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r1292801 - /subversion/trunk/subversion/libsvn_ra_serf/locks.c Date: Thu, 23 Feb 2012 13:44:13 -0000 To: commits@subversion.apache.org From: rhuijben@apache.org X-Mailer: svnmailer-1.0.8-patched Message-Id: <20120223134413.2912A23889B8@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: rhuijben Date: Thu Feb 23 13:44:12 2012 New Revision: 1292801 URL: http://svn.apache.org/viewvc?rev=1292801&view=rev Log: Remove a few memory and error leaks from the ra_serf lock/unlock code by moving to the common iterpool pattern. * subversion/libsvn_ra_serf/locks.c (handle_lock): Return more interesting error code. (svn_ra_serf__lock): Rename pool variables and destroy iterpool when we are finished. Use hash helper functions. (svn_ra_serf__unlock): Rename pool variables, destroy iterpool and properly clear errors when lock_func is NULL. Modified: subversion/trunk/subversion/libsvn_ra_serf/locks.c Modified: subversion/trunk/subversion/libsvn_ra_serf/locks.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/locks.c?rev=1292801&r1=1292800&r2=1292801&view=diff ============================================================================== --- subversion/trunk/subversion/libsvn_ra_serf/locks.c (original) +++ subversion/trunk/subversion/libsvn_ra_serf/locks.c Thu Feb 23 13:44:12 2012 @@ -412,7 +412,7 @@ handle_lock(serf_request_t *request, if (err && APR_STATUS_IS_EOF(err->apr_err)) { ctx->done = TRUE; - err = svn_error_createf(SVN_ERR_RA_DAV_REQUEST_FAILED, + err = svn_error_createf(SVN_ERR_RA_DAV_FORBIDDEN, err, _("Lock request failed: %d %s"), ctx->status_code, ctx->reason); @@ -572,44 +572,43 @@ svn_ra_serf__lock(svn_ra_session_t *ra_s svn_boolean_t force, svn_ra_lock_callback_t lock_func, void *lock_baton, - apr_pool_t *pool) + apr_pool_t *scratch_pool) { svn_ra_serf__session_t *session = ra_session->priv; apr_hash_index_t *hi; - apr_pool_t *subpool; + apr_pool_t *iterpool; - subpool = svn_pool_create(pool); + iterpool = svn_pool_create(scratch_pool); /* ### TODO for issue 2263: Send all the locks over the wire at once. This loop is just a temporary shim. */ - for (hi = apr_hash_first(pool, path_revs); hi; hi = apr_hash_next(hi)) + for (hi = apr_hash_first(scratch_pool, path_revs); + hi; + hi = apr_hash_next(hi)) { svn_ra_serf__handler_t *handler; svn_ra_serf__xml_parser_t *parser_ctx; const char *req_url; lock_info_t *lock_ctx; - const void *key; - void *val; svn_error_t *err; svn_error_t *new_err = NULL; - svn_pool_clear(subpool); + svn_pool_clear(iterpool); - lock_ctx = apr_pcalloc(subpool, sizeof(*lock_ctx)); + lock_ctx = apr_pcalloc(iterpool, sizeof(*lock_ctx)); - apr_hash_this(hi, &key, NULL, &val); - lock_ctx->pool = subpool; - lock_ctx->path = key; - lock_ctx->revision = *((svn_revnum_t*)val); - lock_ctx->lock = svn_lock_create(subpool); - lock_ctx->lock->path = key; + lock_ctx->pool = iterpool; + lock_ctx->path = svn__apr_hash_index_key(hi); + lock_ctx->revision = *((svn_revnum_t*)svn__apr_hash_index_val(hi)); + lock_ctx->lock = svn_lock_create(iterpool); + lock_ctx->lock->path = lock_ctx->path; lock_ctx->lock->comment = comment; lock_ctx->force = force; req_url = svn_path_url_add_component2(session->session_url.path, - lock_ctx->path, subpool); + lock_ctx->path, iterpool); - handler = apr_pcalloc(subpool, sizeof(*handler)); + handler = apr_pcalloc(iterpool, sizeof(*handler)); handler->method = "LOCK"; handler->path = req_url; @@ -617,9 +616,9 @@ svn_ra_serf__lock(svn_ra_session_t *ra_s handler->conn = session->conns[0]; handler->session = session; - parser_ctx = apr_pcalloc(subpool, sizeof(*parser_ctx)); + parser_ctx = apr_pcalloc(iterpool, sizeof(*parser_ctx)); - parser_ctx->pool = subpool; + parser_ctx->pool = iterpool; parser_ctx->user_data = lock_ctx; parser_ctx->start = start_lock; parser_ctx->end = end_lock; @@ -636,16 +635,18 @@ svn_ra_serf__lock(svn_ra_session_t *ra_s handler->response_baton = parser_ctx; svn_ra_serf__request_create(handler); - err = svn_ra_serf__context_run_wait(&lock_ctx->done, session, subpool); + err = svn_ra_serf__context_run_wait(&lock_ctx->done, session, iterpool); if (lock_func) new_err = lock_func(lock_baton, lock_ctx->path, TRUE, lock_ctx->lock, - err, subpool); + err, iterpool); svn_error_clear(err); SVN_ERR(new_err); } + svn_pool_destroy(iterpool); + return SVN_NO_ERROR; } @@ -677,40 +678,41 @@ svn_ra_serf__unlock(svn_ra_session_t *ra svn_boolean_t force, svn_ra_lock_callback_t lock_func, void *lock_baton, - apr_pool_t *pool) + apr_pool_t *scratch_pool) { svn_ra_serf__session_t *session = ra_session->priv; apr_hash_index_t *hi; - apr_pool_t *subpool; + apr_pool_t *iterpool; - subpool = svn_pool_create(pool); + iterpool = svn_pool_create(scratch_pool); /* ### TODO for issue 2263: Send all the locks over the wire at once. This loop is just a temporary shim. */ - for (hi = apr_hash_first(pool, path_tokens); hi; hi = apr_hash_next(hi)) + for (hi = apr_hash_first(scratch_pool, path_tokens); + hi; + hi = apr_hash_next(hi)) { svn_ra_serf__handler_t *handler; svn_ra_serf__simple_request_context_t *ctx; const char *req_url, *path, *token; - const void *key; - void *val; svn_lock_t *existing_lock = NULL; struct unlock_context_t unlock_ctx; - svn_error_t *lock_err = NULL; + svn_error_t *err = NULL; + svn_error_t *new_err = NULL; - svn_pool_clear(subpool); - ctx = apr_pcalloc(subpool, sizeof(*ctx)); - ctx->pool = subpool; + svn_pool_clear(iterpool); - apr_hash_this(hi, &key, NULL, &val); - path = key; - token = val; + ctx = apr_pcalloc(iterpool, sizeof(*ctx)); + ctx->pool = iterpool; + + path = svn__apr_hash_index_key(hi); + token = svn__apr_hash_index_val(hi); if (force && (!token || token[0] == '\0')) { SVN_ERR(svn_ra_serf__get_lock(ra_session, &existing_lock, path, - subpool)); + iterpool)); token = existing_lock->token; if (!token) { @@ -723,22 +725,25 @@ svn_ra_serf__unlock(svn_ra_session_t *ra if (lock_func) { svn_error_t *err2; - err2 = lock_func(lock_baton, path, FALSE, NULL, err, subpool); + err2 = lock_func(lock_baton, path, FALSE, NULL, err, + iterpool); svn_error_clear(err); if (err2) - return err2; + return svn_error_trace(err2); } + else + svn_error_clear(err); continue; } } unlock_ctx.force = force; - unlock_ctx.token = apr_pstrcat(subpool, "<", token, ">", (char *)NULL); + unlock_ctx.token = apr_pstrcat(iterpool, "<", token, ">", (char *)NULL); req_url = svn_path_url_add_component2(session->session_url.path, path, - subpool); + iterpool); - handler = apr_pcalloc(subpool, sizeof(*handler)); + handler = apr_pcalloc(iterpool, sizeof(*handler)); handler->method = "UNLOCK"; handler->path = req_url; @@ -752,7 +757,7 @@ svn_ra_serf__unlock(svn_ra_session_t *ra handler->response_baton = ctx; svn_ra_serf__request_create(handler); - SVN_ERR(svn_ra_serf__context_run_wait(&ctx->done, session, subpool)); + SVN_ERR(svn_ra_serf__context_run_wait(&ctx->done, session, iterpool)); switch (ctx->status) { @@ -760,23 +765,25 @@ svn_ra_serf__unlock(svn_ra_session_t *ra break; /* OK */ case 403: /* Api users expect this specific error code to detect failures */ - lock_err = svn_error_createf(SVN_ERR_FS_LOCK_OWNER_MISMATCH, NULL, - _("Unlock request failed: %d %s"), - ctx->status, ctx->reason); + err = svn_error_createf(SVN_ERR_FS_LOCK_OWNER_MISMATCH, NULL, + _("Unlock request failed: %d %s"), + ctx->status, ctx->reason); break; default: - lock_err = svn_error_createf(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL, - _("Unlock request failed: %d %s"), - ctx->status, ctx->reason); + err = svn_error_createf(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL, + _("Unlock request failed: %d %s"), + ctx->status, ctx->reason); } if (lock_func) - { - SVN_ERR(lock_func(lock_baton, path, FALSE, existing_lock, - lock_err, subpool)); - svn_error_clear(lock_err); - } + new_err = lock_func(lock_baton, path, FALSE, existing_lock, err, + iterpool); + + svn_error_clear(err); + SVN_ERR(new_err); } + svn_pool_destroy(iterpool); + return SVN_NO_ERROR; }