Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1335339 - in
 /subversion/trunk/subversion/libsvn_ra_serf:
 locks.c util.c
Date: Tue, 08 May 2012 05:22:59 -0000
To: commits@subversion.apache.org
From: gstein@apache.org
Message-Id: <20120508052259.E954023889B3@eris.apache.org>

Author: gstein
Date: Tue May  8 05:22:59 2012
New Revision: 1335339

URL: http://svn.apache.org/viewvc?rev=1335339&view=rev
Log:
Switch the locks processing over to proper error parsing/handling.

* subversion/libsvn_ra_serf/locks.c:
  (lock_info_t): remove DONE member.
  (determine_error): new helper for resolving the correct error to
    return from locking operations.
  (handle_lock): for errors 403 and 423, switch over to processing the
    body for a potential human-readable server error. drop the older
    error handling.
  (svn_ra_serf__get_lock, svn_ra_serf__lock): switch to HANDLER->DONE
    and run_one(), and use determine_error() to select the returned error.

* subversion/libsvn_ra_serf/util.c:
  (handle_response_cb): ensure that DONE is set if we ever see EOF

Modified:
    subversion/trunk/subversion/libsvn_ra_serf/locks.c
    subversion/trunk/subversion/libsvn_ra_serf/util.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=1335339&r1=1335338&r2=1335339&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/locks.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/locks.c Tue May  8 05:22:59 2012
@@ -72,8 +72,6 @@ typedef struct lock_info_t {
 
   svn_ra_serf__handler_t *handler;
 
-  /* are we done? */
-  svn_boolean_t done;
 } lock_info_t;
 
 

@@ -331,6 +329,42 @@ set_lock_headers(serf_bucket_t *headers,
   return APR_SUCCESS;
 }
 
+
+/* Register an error within the session. If something is already there,
+   then it will take precedence.  */
+static svn_error_t *
+determine_error(svn_ra_serf__handler_t *handler,
+                svn_error_t *err)
+{
+  /* If we found an error in the response, then blend it in.  */
+  if (handler->server_error)
+    {
+      /* Client-side error takes precedence.  */
+      err = svn_error_compose_create(err, handler->server_error->error);
+    }
+  else
+    {
+      apr_status_t errcode;
+
+      if (handler->sline.code == 423)
+        errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
+      else if (handler->sline.code == 403)
+        errcode = SVN_ERR_RA_DAV_FORBIDDEN;
+      else
+        return err;
+
+      /* The server did not send us a detailed human-readable error.
+         Provide a generic error.  */
+      err = svn_error_createf(errcode, err,
+                              _("Lock request failed: %d %s"),
+                              handler->sline.code,
+                              handler->sline.reason);
+    }
+
+  return err;
+}
+
+
 /* Implements svn_ra_serf__response_handler_t */
 static svn_error_t *
 handle_lock(serf_request_t *request,
@@ -340,30 +374,24 @@ handle_lock(serf_request_t *request,
 {
   svn_ra_serf__xml_parser_t *xml_ctx = handler_baton;
   lock_info_t *ctx = xml_ctx->user_data;
-  svn_error_t *err;
+
+  /* 403 (Forbidden) when a lock doesn't exist.
+     423 (Locked) when a lock already exists.  */
+  if (ctx->handler->sline.code == 403
+      || ctx->handler->sline.code == 423)
+    {
+      /* Go look in the body for a server-provided error. This will
+         reset flags for the core handler to Do The Right Thing. We
+         won't be back to this handler again.  */
+      return svn_error_trace(svn_ra_serf__expect_empty_body(
+                               request, response, ctx->handler, pool));
+    }
 
   if (ctx->read_headers == FALSE)
     {
       serf_bucket_t *headers;
       const char *val;
 
-      /* 423 == Locked */
-      if (ctx->handler->sline.code == 423)
-        {
-          /* Older servers may not give a descriptive error, so we'll
-             make one of our own if we can't find one in the response. */
-          err = svn_ra_serf__handle_server_error(request, response, pool);
-          if (!err)
-            {
-              err = svn_error_createf(SVN_ERR_FS_PATH_ALREADY_LOCKED,
-                                      NULL,
-                                      _("Lock request failed: %d %s"),
-                                      ctx->handler->sline.code,
-                                      ctx->handler->sline.reason);
-            }
-          return err;
-        }
-
       headers = serf_bucket_response_get_headers(response);
 
       val = serf_bucket_headers_get(headers, SVN_DAV_LOCK_OWNER_HEADER);
@@ -382,24 +410,6 @@ handle_lock(serf_request_t *request,
       ctx->read_headers = TRUE;
     }
 
-  /* Forbidden when a lock doesn't exist. */
-  if (ctx->handler->sline.code == 403)
-    {
-      /* If we get an "unexpected EOF" error, we'll wrap it with
-         generic request failure error. */
-      err = svn_ra_serf__handle_discard_body(request, response, NULL, pool);
-      if (err && APR_STATUS_IS_EOF(err->apr_err))
-        {
-          ctx->done = TRUE;
-          err = svn_error_createf(SVN_ERR_RA_DAV_FORBIDDEN,
-                                  err,
-                                  _("Lock request failed: %d %s"),
-                                  ctx->handler->sline.code,
-                                  ctx->handler->sline.reason);
-        }
-      return err;
-    }
-
   return svn_ra_serf__handle_xml_parser(request, response,
                                         handler_baton, pool);
 }
@@ -513,7 +523,7 @@ svn_ra_serf__get_lock(svn_ra_session_t *
   parser_ctx->start = start_lock;
   parser_ctx->end = end_lock;
   parser_ctx->cdata = cdata_lock;
-  parser_ctx->done = &lock_ctx->done;
+  parser_ctx->done = &handler->done;
 
   handler->body_delegate = create_getlock_body;
   handler->body_delegate_baton = lock_ctx;
@@ -526,8 +536,8 @@ svn_ra_serf__get_lock(svn_ra_session_t *
 
   lock_ctx->handler = handler;
 
-  svn_ra_serf__request_create(handler);
-  err = svn_ra_serf__context_run_wait(&lock_ctx->done, session, pool);
+  err = svn_ra_serf__context_run_one(handler, pool);
+  err = determine_error(handler, err);
 
   if (handler->sline.code == 404)
     {
@@ -608,7 +618,7 @@ svn_ra_serf__lock(svn_ra_session_t *ra_s
       parser_ctx->start = start_lock;
       parser_ctx->end = end_lock;
       parser_ctx->cdata = cdata_lock;
-      parser_ctx->done = &lock_ctx->done;
+      parser_ctx->done = &handler->done;
 
       handler->header_delegate = set_lock_headers;
       handler->header_delegate_baton = lock_ctx;
@@ -621,8 +631,8 @@ svn_ra_serf__lock(svn_ra_session_t *ra_s
 
       lock_ctx->handler = handler;
 
-      svn_ra_serf__request_create(handler);
-      err = svn_ra_serf__context_run_wait(&lock_ctx->done, session, iterpool);
+      err = svn_ra_serf__context_run_one(handler, iterpool);
+      err = determine_error(handler, err);
 
       if (lock_func)
         new_err = lock_func(lock_baton, lock_ctx->path, TRUE, lock_ctx->lock,

Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1335339&r1=1335338&r2=1335339&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Tue May  8 05:22:59 2012
@@ -1916,7 +1916,6 @@ handle_response_cb(serf_request_t *reque
                    apr_pool_t *scratch_pool)
 {
   svn_ra_serf__handler_t *handler = baton;
-  svn_ra_serf__session_t *session = handler->session;
   svn_error_t *err;
   apr_status_t inner_status;
   apr_status_t outer_status;
@@ -1925,8 +1924,16 @@ handle_response_cb(serf_request_t *reque
                                         handler, &inner_status,
                                         scratch_pool));
 
-  outer_status = save_error(session, err);
-  return outer_status ? outer_status : inner_status;
+  /* Select the right status value to return.  */
+  outer_status = save_error(handler->session, err);
+  if (!outer_status)
+    outer_status = inner_status;
+
+  /* Make sure the DONE flag is set properly.  */
+  if (APR_STATUS_IS_EOF(outer_status) || APR_STATUS_IS_EOF(inner_status))
+    handler->done = TRUE;
+
+  return outer_status;
 }
 
 /* Perform basic request setup, with special handling for HEAD requests,