Re: [squid-dev] [PATCH] Add reply_header_add

[Alex Rousskov]

On 03/15/2016 07:29 PM, Nathan Hoad wrote:
> On 15 March 2016 at 12:11, Alex Rousskov wrote:
>> On 03/14/2016 05:46 PM, Nathan Hoad wrote:

>> * You have not adjusted HTTP response headers produced by
>> Http::One::Server::writeControlMsgAndCall(). Please either apply the
>> same logic to those headers or explicitly document that control message
>> headers are out of this option reach.

> Done.


Thank you. After those improvements, you can probably see a new pattern
emerging:

> if (Config2.onoff.mangle_request_headers)
> httpHdrMangleList(hdr_out, request, ROR_REQUEST);
> 
> httpHdrAdd(hdr_out, request, al, Config.request_header_add);

and


> httpHdrMangleList(>header, http->request, ROR_REPLY);
> httpHdrAdd(>header, http->request, http->al, Config.reply_header_add);

and

> httpHdrMangleList(hdr, request, ROR_REPLY);
> 
> httpHdrAdd(hdr, request, http->al, Config.reply_header_add);

and

> $ bzr grep 'httpHdrMangleList' | wc -l
> 3

and, I would imagine,

> bzr grep 'httpHdrAdd' | wc -l
> 3


Yes, there are only three cases, and in all of them, we apply the same
set of two operations. Thus, you can now move the httpHdrAdd() call into
httpHdrMangleList()!

If you do that, move the code that gets the right Config.*_header_access
mangler from the beginning of httpHdrMangle() to httpHdrMangleList()
itself and adjust it to also check Config2.onoff.mangle_request_headers
during ROR_REQUEST.

Adjust as needed: httpHdrMangle() is a private/static function and you
will be working with all three httpHdrMangleList() calls in existence
(AFAICT).


I cannot require these additional changes from you, but if you need an
extra incentive/motivation, then just look at the loop in
httpHdrMangleList() and what happens at the start of httpHdrMangle()
that the loop calls on every iteration. Consider the common case of no
Config.*_header_access manglers configured. See how httpHdrMangleList()
always iterates through all header fields while repeating the same
checks and doing nothing useful at all?

With the above changes (that move those checks to be done once, in front
of the loop, and that will entirely eliminate looping in the common
case), you would be making Squid a tiny bit faster despite adding a new
feature!


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] Build failed in Jenkins: trunk-full-matrix » clang,d-debian-unstable #126

[noc]

See 


--
[...truncated 6843 lines...]
make[5]: Leaving directory 
'
Making dvi in RADIUS
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in SMB
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in SMB_LM
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in fake
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
Making dvi in getpwnam
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi-am'.
make[5]: Leaving directory 
'
make[4]: Leaving directory 
'
Making dvi in digest
make[4]: Entering directory 
'
Making dvi in file
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 
'
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi-am'.
make[5]: Leaving directory 
'
make[4]: Leaving directory 
'
Making dvi in negotiate
make[4]: Entering directory 
'
Making dvi in wrapper
make[5]: Entering directory 
'
make[5]: Nothing to be done for 'dvi'.
make[5]: Leaving directory 

[squid-dev] Jenkins build is back to normal : trunk-full-matrix » clang,d-debian-jessie #126

[noc]

See 


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Add reply_header_add

[Nathan Hoad]

On 15 March 2016 at 12:11, Alex Rousskov
 wrote:
> On 03/14/2016 05:46 PM, Nathan Hoad wrote:
>
>> The attached patch implements reply_header_add, for adding HTTP
>> headers to reply objects as they're sent to the client.
>
> Thank you for this useful addition. Unfortunately, it needs quite a bit
> of work.
>
>
> * Please _carefully_ review your src/cf.data.pre changes. There appear
> to be many copy-paste errors there, some of which seriously misleading
> (e.g., "incoming" vs. "outgoing" and "request" vs. "response").

My bad! Good catch.

>
> * You have not adjusted HTTP response headers produced by
> Http::One::Server::writeControlMsgAndCall(). Please either apply the
> same logic to those headers or explicitly document that control message
> headers are out of this option reach.

Done.

>
> * You have not adjusted HTTP response headers produced by
> tunnelConnected() and ClientHttpRequest::sslBumpStart(). Please either
> apply the same logic to those headers or explicitly document that
> successful CONNECT responses are out of this option reach.

I've opted to document that CONNECT responses aren't covered by this
option. Making it work in these places is outside my capabilities.

>
>
> I also recommend the following adjustments:
>
> * The scary "In theory, all of ..." paragraph that made a lot of sense
> for requests is barely applicable to post-cache responses, where most
> information is already available. Please consider making that text
> context-neutral and moving it to an option-independent location in the
> begging of squid.conf. That would be really useful IMO because we have
> many options using logformat codes now and that old text is applicable,
> to various degrees, to all of them.

Sure, I've added this at the top of the documentation, after SMP
Macros, and removed the paragraphs from both request_header_add and
reply_header_add. I'm not quite sure of the wording, feel free to make
suggestions for how it could be improved.

>
> * Adding a sentence or two explicitly stating that this directive does
> not affect cached responses. Your copied text says that it "has no
> affect on cache hit detection", and that is also true, if somewhat
> redundant for responses.

Done.

>
> * Adding a "See also: request_header_add" statement and the symmetrical
> statement for request_header_add.

Done.

>
>
>
>> +if (Config.reply_header_add && !Config.reply_header_add->empty())
>> +httpHdrAdd(hdr, request, http->al, *Config.reply_header_add);
>
>
> I know you are reusing an existing code template here, but this
> particular copy starts duplicating a lot of logic. I recommend moving
> both guards/conditions from the if-statement into httpHdrAdd() itself.
> If others object to moving both conditions on performance grounds,
> please move at least the second/empty() condition:
>
> That is, ideally:
>
>   httpHdrAdd(hdr, request, http->al, Config.reply_header_add);
>
> Or, if others object:
>
>   if (Config.reply_header_add)
>   httpHdrAdd(hdr, request, http->al, *Config.reply_header_add);

I've gone with your ideal version. If people object, I'm happy to move
to the second version.

>
>
>
> Thank you,
>
> Alex.
>


Thank you,

Nathan.
=== modified file 'src/HttpHeaderTools.cc'
--- src/HttpHeaderTools.cc	2016-01-24 17:41:43 +
+++ src/HttpHeaderTools.cc	2016-03-16 00:31:23 +
@@ -462,11 +462,14 @@
 }
 
 void
-httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer , HeaderWithAclList )
+httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPointer , HeaderWithAclList *headersAdd)
 {
+if (headersAdd == NULL || headersAdd->empty())
+return;
+
 ACLFilledChecklist checklist(NULL, request, NULL);
 
-for (HeaderWithAclList::const_iterator hwa = headersAdd.begin(); hwa != headersAdd.end(); ++hwa) {
+for (HeaderWithAclList::const_iterator hwa = headersAdd->begin(); hwa != headersAdd->end(); ++hwa) {
 if (!hwa->aclList || checklist.fastCheck(hwa->aclList) == ACCESS_ALLOWED) {
 const char *fieldValue = NULL;
 MemBuf mb;

=== modified file 'src/SquidConfig.h'
--- src/SquidConfig.h	2016-03-12 20:27:35 +
+++ src/SquidConfig.h	2016-03-14 23:38:41 +
@@ -463,6 +463,8 @@
 HeaderManglers *reply_header_access;
 ///request_header_add access list
 HeaderWithAclList *request_header_add;
+///reply_header_add access list
+HeaderWithAclList *reply_header_add;
 ///note
 Notes notes;
 char *coredump_dir;

=== modified file 'src/cf.data.pre'
--- src/cf.data.pre	2016-03-12 20:27:35 +
+++ src/cf.data.pre	2016-03-16 00:56:41 +
@@ -109,6 +109,21 @@
 	${service_name} expands into the current Squid service instance
 	name identifier which is provided by -n on the command line.
 
+  Logformat Macros
+
+	Logformat macros can be used in many places outside of the logformat
+	directive. In theory, all of the logformat codes can be used as %macros,
+	

Re: [squid-dev] [RFC][PATCH] Bug4438 second attempt: give MemBlob its own pools

[Alex Rousskov]

On 03/14/2016 02:11 PM, Kinkie wrote:

>   this second attempt at bug4438 tries a different approach: by giving
> MemBlob its own pools and having a hard initialization dependency in
> MemBlob's allocating function, instead of relying on memAllocString.

Forgive me if I missed how this decision was made earlier, but I do not
understand why we want to duplicate the existing memAllocString()-like
functionality and split a single set of raw memory pools shared by all
callers into two isolated sets?

In other words, how did we get from "SIGSEGV in memFreeString" (i.e.,
bug 4438) to this? If your working (or perhaps already proven!) theory
is that memAllocString() does something wrong, why not fix
memAllocString() instead of duplicating it? I must be missing something
obvious here...


> Known side-effects:
> - allows to tune memblob sizes according to actual use patterns, which
> can be collected via SBuf/MemBlob detailed stats. The currently-set
> sizes follow some light testing I've done.

Why is that a side effect? Could we not "tune memblob sizes according to
actual use patterns" before this change?

Or do you meant that we can tune two separate sets of pools now? If yes,
then you should also list "two separate sets of pools instead of one
shared by all callers" as a side effect -- "more isolated sets" is the
"cost" associated with this ability to tune more sets...


> - as no MemBlob can be initialized before MemPools, there is no need
> to baloon statically-initialized MemBlobs to
> SmallestStringBeforeMemIsInitialized.

If your change guarantees that no MemBlob can be initialized before
MemPools, why cannot a similar change be done to the original code so
that no memAllocString()/etc. caller can get its result before MemPools
are initialized? AFAICT, you have solved the problem that you were
trying to solve, but I do not understand why you had to solve it in a
new location (leaving the old problem unsolved?).


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"

[Christos Tsantilas]

On 03/15/2016 07:28 PM, Christos Tsantilas wrote:

I applied the t4 patch to trunk as r14592


The t4 patch applied to squid-3.5  as r14001




I am attaching the t4 patch for squid-3.5.
I believe that we should apply this patch for this major reasons:
   - The (statefull) FTP protocol requires good coordination between
client-side and server side. The pinned connections is one mechanism
which helps, but this is not enough. The stopWaiting/startWaiting adds
one more mechanism. Will help us to solve more problems in the future.

   - The t4 patch also solves one more bug: In the case the FTP server
respond with an error status after the download is finished, squid will
return wrong status code to the user (success). Fixing this bug is
possible because of the new mechanism.

Christos


On 03/15/2016 01:01 AM, Amos Jeffries wrote:

On 15/03/2016 10:41 a.m., Alex Rousskov wrote:

On 03/10/2016 02:35 PM, Alex Rousskov wrote:

Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is
enough to fix the known assertion. Take2 fixes that assertion as well,
but it is bigger because it also fixes design problems that may lead to
other bugs in v3.5. Which one do you want in v3.5?



Amos, that question still stands: What do you want to see in v3.5? The
simple take1 patch that only addresses a specific assertion failure OR
the ported take4+ patch that attempts to fix the general flaw via the
new startWaitingForOrigin()/stopWaitingForOrigin() mechanism?



I dont have a preference for either one.

Amos



___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] assertion failed: Write.cc:41: "!ccb->active()"

[Christos Tsantilas]

I applied the t4 patch to trunk as r14592

I am attaching the t4 patch for squid-3.5.
I believe that we should apply this patch for this major reasons:
  - The (statefull) FTP protocol requires good coordination between 
client-side and server side. The pinned connections is one mechanism 
which helps, but this is not enough. The stopWaiting/startWaiting adds 
one more mechanism. Will help us to solve more problems in the future.


  - The t4 patch also solves one more bug: In the case the FTP server 
respond with an error status after the download is finished, squid will 
return wrong status code to the user (success). Fixing this bug is 
possible because of the new mechanism.


Christos


On 03/15/2016 01:01 AM, Amos Jeffries wrote:

On 15/03/2016 10:41 a.m., Alex Rousskov wrote:

On 03/10/2016 02:35 PM, Alex Rousskov wrote:

Amos, do you want us to port take2 to v3.5? The take1 patch for v3.5 is
enough to fix the known assertion. Take2 fixes that assertion as well,
but it is bigger because it also fixes design problems that may lead to
other bugs in v3.5. Which one do you want in v3.5?



Amos, that question still stands: What do you want to see in v3.5? The
simple take1 patch that only addresses a specific assertion failure OR
the ported take4+ patch that attempts to fix the general flaw via the
new startWaitingForOrigin()/stopWaitingForOrigin() mechanism?



I dont have a preference for either one.

Amos
assertion failed: Write.cc:41: "!ccb->active()"

Bug description:
   - The client side and server side are finished
   - On server side the Ftp::Relay::finalizeDataDownload() is called and
 schedules the Ftp::Server::originDataCompletionCheckpoint
   - On client side the "Ftp::Server::userDataCompletionCheckpoint" is
 called. This is schedules a write to control connection and closes
 data connection.
   - The Ftp::Server::originDataCompletionCheckpoint is called which is
 trying to write to control connection and the assertion triggered.

This bug is an corner case, where the client-side  (FTP::Server) should
wait for the server side (Ftp::Client/Ftp::Relay) to finish its job before
respond to the FTP client. In this bug the existing mechanism, designed
to handle such problems, did not worked correctly and resulted to a double
write response to the client.

This patch try to fix the existing mechanism as follows:

- When Ftp::Server receives a "startWaitingForOrigin" callback, postpones
  writting possible responses to the client and keeps waiting for the
  stopWaitingForOrigin callback

- When the Ftp::Server receives a "stopWaitingForOrigin" callback,
  resumes any postponed response.

- When the Ftp::Client starts working on a DATA-related transaction, calls the
  Ftp::Server::startWaitingForOrigin callback

- When the Ftp::Client finishes its job or when its abort abnormaly, checks
  whether it needs to call Ftp::Server::stopWaitingForOrigin callback.

- Also this patch try to fix the status code returned to the FTP client
  taking in account the status code returned by FTP server. The
  "Ftp::Server::stopWaitingForOrigin" is used to pass the returned status code
  to the client side.

This is a Measurement Factory project

=== modified file 'src/clients/FtpRelay.cc'
--- src/clients/FtpRelay.cc	2016-01-31 06:14:05 +
+++ src/clients/FtpRelay.cc	2016-03-15 12:44:32 +
@@ -39,77 +39,84 @@
 const Ftp::MasterState () const;
 Ftp::MasterState ();
 Ftp::ServerState serverState() const { return master().serverState; }
 void serverState(const Ftp::ServerState newState);
 
 /* Ftp::Client API */
 virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
 virtual void dataChannelConnected(const CommConnectCbParams );
 
 /* Client API */
 virtual void serverComplete();
 virtual void handleControlReply();
 virtual void processReplyBody();
 virtual void handleRequestBodyProducerAborted();
 virtual bool mayReadVirginReplyBody() const;
 virtual void completeForwarding();
 virtual bool abortOnData(const char *reason);
 
 /* AsyncJob API */
 virtual void start();
+virtual void swanSong();
 
 void forwardReply();
 void forwardError(err_type error = ERR_NONE, int xerrno = 0);
 void failedErrorMessage(err_type error, int xerrno);
 HttpReply *createHttpReply(const Http::StatusCode httpStatus, const int64_t clen = 0);
 void handleDataRequest();
 void startDataDownload();
 void startDataUpload();
 bool startDirTracking();
 void stopDirTracking();
 bool weAreTrackingDir() const {return savedReply.message != NULL;}
 
 typedef void (Relay::*PreliminaryCb)();
 void forwardPreliminaryReply(const PreliminaryCb cb);
 void proceedAfterPreliminaryReply();
 PreliminaryCb thePreliminaryCb;
 
 typedef void (Relay::*SM_FUNC)();
 static const SM_FUNC SM_FUNCS[];
 void readGreeting();
 void sendCommand();
 void readReply();
 void readFeatReply();
 

Re: [squid-dev] [PATCH] implement RFC3986

[Alex Rousskov]

On 03/15/2016 07:05 AM, Kinkie wrote:
>> I do not think so. The crux of the issue is that you are defending a
>> medium-size template function as a necessary and small evil, while I am
>> attacking the principle or direction of accommodating helper needs by
>> making Squid code worse.
> 
> Hi,
>   the attached patch is a modification of what was currently shared,
> de-templatized.
> It relies on SBuf and provides a convenient std::string-based wrapper,
> like you suggest.

Thank you.


> +extern const int16_t fromHexTable[256];
> +
> +/// \return the numeric representation of the HEXDIG argument ch, or -1 if 
> invalid.
> +inline const int16_t
> +FromHex(unsigned char ch)
> +{
> +// no need to check bounds, the lookup table has 256 entries
> +return fromHexTable[ch];
> +}

AFAICT, FromHex() may be used before fromHexTable is initialized. Same
concern for the other 5+ global constants this patch introduces. AFAICT,
you have suffered from strange side effects of initialization order more
than enough! How about converting all these constants into
always-safe-to-use functions? If done right, there will be no difference
in performance of the proposed code AFAICT.


> +extern const CharacterSet
> +GenDelims,// RFC 3986 gen-delims set
> +SubDelims,// RFC 3986 sub-delims set
> +Reserved, // RFC 3986 reserved characters set
> +Unreserved, // RFC 3986 unreserved characters set
> +All;

One full/stand-alone declaration per line please.


> +// XXX: SBuf lacking reserve(N)
> +// rv.reserve(s.length()*2); //TODO: optimize arbitrary constant

AFAICT, SBuf::reserveSpace() should work fine here and in all other
define-SBuf-and-immediately-reserve-space contexts. Am I missing something?


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Better support for unknown URL schemes

[Alex Rousskov]

On 03/15/2016 09:36 AM, Amos Jeffries wrote:
> Squid already contains AnyP::PROTO_UNKNOWN support for unknown protocols
> but currently does not preserve the actual string value received for them.
> 
> This adds a textual representation ('image') to the UriScheme object to
> fill that gap and ensure that all URL representatinos (ie cache keys,
> logs and outgoing messages) are generated with the scheme string as it
> was received.

Preserving textual representation is the right thing to do, but the
implementation itself needs a lot more work IMO.


>  /// convert the URL scheme to that given
>  void setScheme(const AnyP::ProtocolType ) {scheme_=p; touch();}
> +void setSchemeImage(const char *str) {scheme_.setImage(str); touch();}


The parameter type should be changed instead. Yes, that will require
more changes, but those changes are essential to properly support
foreign protocol names. Without those changes, you are creating an API
bug in addition to adding a useful feature.

We (mostly you) have already done similar work for foreign HTTP request
methods. This is similar -- UriScheme should be used nearly everywhere
(instead of ProtocolType that does not carry enough information). It
could be argued that the UriScheme class itself should be renamed to
something like Protocol, but I do not want to argue about that.


> +/// Sets the string representation of this scheme.
> +/// Only needed if the scheme type is PROTO_UNKNOWN.
> +void setImage(const char *str) {assert(theScheme_ == 
> AnyP::PROTO_UNKNOWN); schemeImage_ = str;}

This method should be a constructor.


>  char const *
>  AnyP::UriScheme::c_str() const
>  {

This method should be replaced with AnyP::UriScheme::image() returning SBuf.


> +if (!schemeImage_.isEmpty())
> +return schemeImage_.c_str();
> +

Why are we not lowering the case of a foreign protocol image if we are
lowering the case of standard protocol images below? Either both should
be lowered or there should be a source code comment explaining the
discrepancy.

If both should be lowered, the lowering should be done in the
constructor to avoid lowering the same string multiple times.


>  if (theScheme_ > AnyP::PROTO_NONE && theScheme_ < AnyP::PROTO_MAX) {
> -const char *in = AnyP::ProtocolType_str[theScheme_];
> -for (; p < (BUFSIZ-1) && in[p] != '\0'; ++p)
> -out[p] = xtolower(in[p]);
> +schemeImage_ = AnyP::ProtocolType_str[theScheme_];
> +schemeImage_.toLower();
>  }

This slow code may not be needed at all if the rest of the changes are
implemented. If this code stays, please add an "XXX: Slow." or a similar
comment after toLower(). There are many ways to fix/optimize this, but
the best way probably depends on the rest of the changes so I am not
going to suggest any specific TODO here.


> @@ -157,6 +158,9 @@
>  if (strncasecmp(b, "whois", len) == 0)
>  return AnyP::PROTO_WHOIS;
>  
> +if (len > 0)
> +return AnyP::PROTO_UNKNOWN;
> +
>  return AnyP::PROTO_NONE;
>  }

This function should return a UriScheme object instead. Ideally, using
created-once constants for common protocols (so that we do not have to
deal with to-lower-case conversion every time a scheme image is needed).


> @@ -27,7 +28,7 @@
>  ~UriScheme() {}
...
> +/// the string representation to use for theScheme_
> +mutable SBuf schemeImage_;

Please do not name Foo members FooX. One Foo is enough!


> +/// the string representation to use for theScheme_

s/to use for theScheme_//
or
s/to use for theScheme_/of the scheme//

If you can make such a guarantee after all other changes, please add
something like "; always in lower case".


> +mutable SBuf schemeImage_;

Mutable may not be needed after all other changes.


It is difficult for me to review 4-line no-function-names diff, so
forgive me if I missed any context-dependent caveats.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [PATCH] Better support for unknown URL schemes

[Amos Jeffries]

Squid already contains AnyP::PROTO_UNKNOWN support for unknown protocols
but currently does not preserve the actual string value received for them.

This adds a textual representation ('image') to the UriScheme object to
fill that gap and ensure that all URL representatinos (ie cache keys,
logs and outgoing messages) are generated with the scheme string as it
was received.

Future work:
* add ACL support for arbitrary scheme names
* support for comparisons of unknown schemes

Amos
=== modified file 'src/URL.h'
--- src/URL.h   2016-02-23 08:51:22 +
+++ src/URL.h   2016-03-15 11:03:44 +
@@ -43,6 +43,7 @@
 
 /// convert the URL scheme to that given
 void setScheme(const AnyP::ProtocolType ) {scheme_=p; touch();}
+void setSchemeImage(const char *str) {scheme_.setImage(str); touch();}
 
 void userInfo(const SBuf ) {userInfo_=s; touch();}
 const SBuf () const {return userInfo_;}

=== modified file 'src/anyp/UriScheme.cc'
--- src/anyp/UriScheme.cc   2016-01-01 00:12:18 +
+++ src/anyp/UriScheme.cc   2016-03-15 15:10:17 +
@@ -14,19 +14,17 @@
 char const *
 AnyP::UriScheme::c_str() const
 {
+if (!schemeImage_.isEmpty())
+return schemeImage_.c_str();
+
 if (theScheme_ == AnyP::PROTO_UNKNOWN)
-return "(unknown)";
-
-static char out[BUFSIZ];
-int p = 0;
+return "(unknown)"; // should have been set explicitly
 
 if (theScheme_ > AnyP::PROTO_NONE && theScheme_ < AnyP::PROTO_MAX) {
-const char *in = AnyP::ProtocolType_str[theScheme_];
-for (; p < (BUFSIZ-1) && in[p] != '\0'; ++p)
-out[p] = xtolower(in[p]);
+schemeImage_ = AnyP::ProtocolType_str[theScheme_];
+schemeImage_.toLower();
 }
-out[p] = '\0';
-return out;
+return schemeImage_.c_str();
 }
 
 unsigned short

=== modified file 'src/anyp/UriScheme.h'
--- src/anyp/UriScheme.h2016-01-01 00:12:18 +
+++ src/anyp/UriScheme.h2016-03-15 11:19:04 +
@@ -10,6 +10,7 @@
 #define SQUID_ANYP_URISCHEME_H
 
 #include "anyp/ProtocolType.h"
+#include "sbuf/SBuf.h"
 
 #include 
 
@@ -27,7 +28,7 @@
 ~UriScheme() {}
 
 operator AnyP::ProtocolType() const { return theScheme_; }
-
+// XXX: does not account for comparison of unknown schemes (by image)
 bool operator != (AnyP::ProtocolType const & aProtocol) const { return 
theScheme_ != aProtocol; }
 
 /** Get a char string representation of the scheme.
@@ -41,9 +42,16 @@
 
 unsigned short defaultPort() const;
 
+/// Sets the string representation of this scheme.
+/// Only needed if the scheme type is PROTO_UNKNOWN.
+void setImage(const char *str) {assert(theScheme_ == AnyP::PROTO_UNKNOWN); 
schemeImage_ = str;}
+
 private:
 /// This is a typecode pointer into the enum/registry of protocols handled.
 AnyP::ProtocolType theScheme_;
+
+/// the string representation to use for theScheme_
+mutable SBuf schemeImage_;
 };
 
 } // namespace AnyP

=== modified file 'src/url.cc'
--- src/url.cc  2016-01-01 00:12:18 +
+++ src/url.cc  2016-03-15 13:52:00 +
@@ -18,6 +18,7 @@
 
 static HttpRequest *urlParseFinish(const HttpRequestMethod& method,
const AnyP::ProtocolType protocol,
+   const char *const protoStr,
const char *const urlpath,
const char *const host,
const SBuf ,
@@ -157,6 +158,9 @@
 if (strncasecmp(b, "whois", len) == 0)
 return AnyP::PROTO_WHOIS;
 
+if (len > 0)
+return AnyP::PROTO_UNKNOWN;
+
 return AnyP::PROTO_NONE;
 }
 
@@ -215,7 +219,7 @@
URL::Asterisk().cmp(url) == 0) {
 protocol = AnyP::PROTO_HTTP;
 port = AnyP::UriScheme(protocol).defaultPort();
-return urlParseFinish(method, protocol, url, host, SBuf(), port, 
request);
+return urlParseFinish(method, protocol, proto, url, host, SBuf(), 
port, request);
 } else if (!strncmp(url, "urn:", 4)) {
 return urnParse(method, url, request);
 } else {
@@ -420,7 +424,7 @@
 }
 }
 
-return urlParseFinish(method, protocol, urlpath, host, SBuf(login), port, 
request);
+return urlParseFinish(method, protocol, proto, urlpath, host, SBuf(login), 
port, request);
 }
 
 /**
@@ -431,6 +435,7 @@
 static HttpRequest *
 urlParseFinish(const HttpRequestMethod& method,
const AnyP::ProtocolType protocol,
+   const char *const protoStr, // for unknown protocols
const char *const urlpath,
const char *const host,
const SBuf ,
@@ -443,6 +448,9 @@
 request->initHTTP(method, protocol, urlpath);
 }
 
+if (protocol == AnyP::PROTO_UNKNOWN)
+request->url.setSchemeImage(protoStr);
+
 request->url.host(host);
 request->url.userInfo(login);
 request->url.port(port);

Re: [squid-dev] [PATCH] implement RFC3986

[Kinkie]

> I do not think so. The crux of the issue is that you are defending a
> medium-size template function as a necessary and small evil, while I am
> attacking the principle or direction of accommodating helper needs by
> making Squid code worse.

Hi,
  the attached patch is a modification of what was currently shared,
de-templatized.
It relies on SBuf and provides a convenient std::string-based wrapper,
like you suggest.

>> In order to clarify, when I talk about couplign I mean link-time
>> coupling. This can be directly understood from the list of objects
>> needed for testSBuf: between headers, objects and stubs that's about
>> 30 files (on top of the testSBuf files themselves).
>
> If testSBuf has 30 dependencies, then I doubt testSBuf is a good example
> of how things should be done. Overall, if we want helpers to use Squid
> code, SBuf can be a part of the convenience library that can be linked
> with helpers without 30 "extra" or "stub" files. The number of files in
> that convenience library is pretty much irrelevant.

The introduction of libsbuf that has happened in the meanwhile causes
less dependency issues: it now only requires libsbuf, libmem, libbase,
libmisccontainers and libmiscutil, as _LDADD instead of _SOURCE files.
I'll work on removing the dependencies from misccontainers which is
required for trie by seeing if a std:: container behaves at least
comparably well for mempools.

-- 
Francesco


rfc3986.bundle
Description: Binary data
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev