TS-2888: remove vararg and format parameters from build_error_resoponse #1299

persiaAziz-zz · 2017-01-03T22:31:13Z

This removes vararg and the formatting parameter from build_error_response. However the underlying fabricate_with_old_api still needs to be cleaned up. Not sure if someone is already working on the cleaning.

zwoop · 2017-01-04T00:28:43Z

[add to whitelist]

atsci · 2017-01-04T00:40:49Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1291/ for details.

atsci · 2017-01-04T00:43:46Z

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/1184/ for details.

SolidWallOfCode · 2017-02-07T23:37:18Z

proxy/http/HttpTransact.cc

    s->reverse_proxy = false;
    goto done;
  }
  /////////////////////////////////////////////////////////////////
  // Check if remap plugin set HTTP return code and return body  //
  /////////////////////////////////////////////////////////////////
  if (s->http_return_code != HTTP_STATUS_NONE) {
-    build_error_response(s, s->http_return_code, nullptr, nullptr, s->internal_msg_buffer_size ? s->internal_msg_buffer : nullptr);
+    build_error_response(s, s->http_return_code, nullptr, nullptr);


What happened to internal_msg_buffer? Can that no longer be set?

SolidWallOfCode · 2017-02-08T17:50:17Z

After talking to @persiaAziz about this I think the best approach is to remove the file char const* argument and the varargs but check for internal_msg_buffer in build_response so it doesn't have to be passed around.

atsci · 2017-02-09T05:08:34Z

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/freebsd-github/1492/ for details.

atsci · 2017-02-09T05:12:14Z

Linux build failed! See https://ci.trafficserver.apache.org/job/linux-github/1385/ for details.

atsci · 2017-02-09T05:28:03Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/56/ for details.

persiaAziz-zz · 2017-02-09T17:11:16Z

hmm, we need to remove the usage of varargs from all subsequent methods if we pass the internal message buffer. Looking at the code and the fail output of regression test, we can not pass a valid buffer with va_list pointing to nullptr. The regression test above is failing because of that. We need to fix ink_bvsprintf first @SolidWallOfCode

SolidWallOfCode · 2017-02-14T20:02:30Z

You shouldn't need to change further methods. At worse, you could pass "%s", internal_msg_buffer as the arguments.

persiaAziz-zz · 2017-02-28T22:13:36Z

@SolidWallOfCode please review

atsci · 2017-02-28T22:20:30Z

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/freebsd-github/1651/ for details.

atsci · 2017-02-28T22:26:08Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1547/ for details.

atsci · 2017-02-28T22:27:01Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/83/ for details.

atsci · 2017-02-28T22:31:54Z

Intel CC build failed! See https://ci.trafficserver.apache.org/job/icc-github/84/ for details.

atsci · 2017-02-28T22:45:23Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1652/ for details.

atsci · 2017-02-28T22:46:06Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1548/ for details.

atsci · 2017-02-28T23:00:08Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/215/ for details.

atsci · 2017-02-28T23:15:45Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/216/ for details.

atsci · 2017-03-01T16:21:23Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1554/ for details.

atsci · 2017-03-01T16:21:58Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/90/ for details.

atsci · 2017-03-01T16:36:52Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/222/ for details.

SolidWallOfCode · 2017-03-01T16:57:02Z

proxy/http/HttpTransact.cc

-      if (s->txn_conf->parent_failures_update_hostdb) {
-        s->state_machine->do_hostdb_update_if_necessary();
-      }
+      s->state_machine->do_hostdb_update_if_necessary();


This looks like a bad merge.

yeah it is, see #1464 merged about a week ago.

I will fix that. Thanks for noticing

SolidWallOfCode · 2017-03-01T16:58:21Z

proxy/http/HttpTransact.cc

-    "redirect#moved_temporarily", s, 8192, &s->internal_msg_buffer_size, body_language, sizeof(body_language), body_type,
-    sizeof(body_type), "%s <a href=\"%s\">%s</a>.  %s.", "The document you requested is now", new_url, new_url,
-    "Please update your documents and bookmarks accordingly", NULL);
+  s->internal_msg_buffer = body_factory->getFormat(8192, &s->internal_msg_buffer_size, "%s <a href=\"%s\">%s</a>.  %s.",


Add a comment here that this is necessary because the template format doesn't have access to this computed URL and there is no current way to provide that.

atsci · 2017-03-01T23:34:56Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1662/ for details.

atsci · 2017-03-01T23:36:18Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1558/ for details.

atsci · 2017-03-01T23:36:41Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/94/ for details.

atsci · 2017-03-01T23:51:56Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/226/ for details.

atsci · 2017-03-14T22:34:20Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/61/

atsci · 2017-03-14T22:34:22Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/74/

atsci · 2017-03-14T22:36:42Z

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/58/

atsci · 2017-03-14T22:38:40Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/187/

atsci · 2017-03-14T22:39:33Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1756/

atsci · 2017-03-14T22:43:00Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1650/

atsci · 2017-03-14T22:54:25Z

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/319/

This was introduced in apache#1299 But, the good news is that this has not been part of a release.

This was introduced in #1299 But, the good news is that this has not been part of a release.

This was introduced in #1299 But, the good news is that this has not been part of a release. (cherry picked from commit 7acfcfb)

zwoop assigned persiaAziz-zz Jan 4, 2017

zwoop requested a review from vmamidi January 4, 2017 00:23

zwoop added the HTTP label Jan 4, 2017

zwoop added this to the 7.1.0 milestone Jan 4, 2017

zwoop modified the milestones: 7.1.0, 7.2.0 Jan 8, 2017

SolidWallOfCode reviewed Feb 7, 2017

View reviewed changes

persiaAziz-zz force-pushed the TS-2888 branch from 94e4a7b to 6563ea8 Compare February 9, 2017 04:58

persiaAziz-zz force-pushed the TS-2888 branch from 6563ea8 to 0807f32 Compare February 28, 2017 22:11

persiaAziz-zz force-pushed the TS-2888 branch from afe25b6 to 4407e57 Compare March 1, 2017 16:08

SolidWallOfCode reviewed Mar 1, 2017

View reviewed changes

persiaAziz-zz force-pushed the TS-2888 branch from 4407e57 to 51a2bf6 Compare March 1, 2017 23:21

TS-2888: remove vararg and format parameters from build_error_resoponse

eaa6daa

persiaAziz-zz force-pushed the TS-2888 branch from 51a2bf6 to eaa6daa Compare March 14, 2017 22:25

SolidWallOfCode merged commit d6c2a82 into apache:master Mar 15, 2017

zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017

scw00 mentioned this pull request Jun 20, 2017

Missing Length if format was set in fabricate_with_old_api #2154

Merged

zwoop pushed a commit to zwoop/trafficserver that referenced this pull request Jul 17, 2018

Fixes some var-args missing va_start/end

c02da5e

This was introduced in apache#1299 But, the good news is that this has not been part of a release.

zwoop pushed a commit to zwoop/trafficserver that referenced this pull request Jul 18, 2018

Fixes some var-args missing va_start/end

6f55705

This was introduced in apache#1299 But, the good news is that this has not been part of a release.

zwoop pushed a commit that referenced this pull request Jul 18, 2018

Fixes some var-args missing va_start/end

7acfcfb

This was introduced in #1299 But, the good news is that this has not been part of a release.

zwoop pushed a commit that referenced this pull request Jul 31, 2018

Fixes some var-args missing va_start/end

0815a85

This was introduced in #1299 But, the good news is that this has not been part of a release. (cherry picked from commit 7acfcfb)

shinya1020 mentioned this pull request Jan 17, 2019

Fix ATS does not honor proxy.config.body_factory.response_max_size #4824

Merged

TS-2888: remove vararg and format parameters from build_error_resoponse #1299

TS-2888: remove vararg and format parameters from build_error_resoponse #1299

Conversation

persiaAziz-zz commented Jan 3, 2017

zwoop commented Jan 4, 2017

atsci commented Jan 4, 2017

atsci commented Jan 4, 2017

SolidWallOfCode Feb 7, 2017

Choose a reason for hiding this comment

SolidWallOfCode commented Feb 8, 2017

atsci commented Feb 9, 2017

atsci commented Feb 9, 2017

atsci commented Feb 9, 2017

persiaAziz-zz commented Feb 9, 2017 • edited Loading

SolidWallOfCode commented Feb 14, 2017

persiaAziz-zz commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Mar 1, 2017

atsci commented Mar 1, 2017

atsci commented Mar 1, 2017

SolidWallOfCode Mar 1, 2017

Choose a reason for hiding this comment

jrushford Mar 1, 2017

Choose a reason for hiding this comment

persiaAziz-zz Mar 1, 2017

Choose a reason for hiding this comment

SolidWallOfCode Mar 1, 2017

Choose a reason for hiding this comment

atsci commented Mar 1, 2017

atsci commented Mar 1, 2017

atsci commented Mar 1, 2017

atsci commented Mar 1, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

atsci commented Mar 14, 2017

persiaAziz-zz commented Feb 9, 2017 •

edited

Loading