On 11.09.2012 04:16, Eliezer Croitoru wrote:
On 09/10/2012 05:39 PM, Alex Rousskov wrote:
On 09/09/2012 04:08 AM, Eliezer Croitoru wrote:
I took the url_rewrite helper and used what ever seems needed to
create
a url_store_rewrite helper.
I also added the needed variables for the next step and freeing
them
where and if needed.
this is a more of a basic sketch.
Please do not mark sketch patches not meant for commit with a
[PATCH]
prefix. [RFC] or no prefix would be better.
OK
When you think these changes are ready for commit, please include a
proposed commit message. It is a good idea to do that even before
the
last stage, of course -- it tells us what the patch is supposed to
do.
"what ever seems needed to create a url_store_rewrite helper" is too
vague. If the code is committed with such a message, most of the
knowledge gained during recent discussions will be lost and somebody
would have to start from scratch when they want to understand how
the
new feature works and, more importantly, why it should work that
way.
it's not that vague if you do ask me.
the process is pretty simple if you do ask me (after reading XK++
lines)
I dont want to maintain a branch but to push it into squid as a
feature.
I proposed from the beginning to make it in stages:
1. embed a fake store url rewrite fake.(naming is not really
important) as grounds to the next steps.
If by this you mean the helper/ section fake helper. The
helper/url_rewrite/fake should be sufficient. url-rewrite, store-url,
and location-rewrite (also not ported) all have the same API limitations
and format so the helpers can all be in helper/url_rewrite/ together.
this part is pretty important by itself because no one even done that
in the 3+.
2. embed the code that actually handles the data from the external
program when storing it in squid process.
3. use it at every point needed in squid while handling the requests.
Please do not forget to document all new members (using doxygen
comments) -- this is one of the mandatory requirements for patches.
And,
as Amos pointed out, remove "for storeurl" marks -- everything you
change in this patch is for the storeurl feature (or at least it
should
be) so the diff blobs themselves can be used as such a mark.
Since I Have never used doxygen and friends in my code before I will
try to learn them on the way.
Basically C/C++ comments, just add an extra / or * ( "///..." or
"/** ... */") to start the comment and ensure clear paragraph
separation.
For functions/methods with return values or special mention required on
parameters use \param and \return or \retval. You can see examples of
these around the code or we can assist with corrections during audit.
If this code is not sufficient to support the storeurl feature, I do
not
think it should be committed to trunk -- it has no stand-alone
value.
Commit it to your own branch so that you can maintain the progress
history and post incremental patches for discussion. When the
feature is
completed, your branch can then be merged into trunk.
I do have a plan since From all the data I gathered and code review I
did until now it's pretty straight forward.
I will look at the guidelines of squid3 code (started already)
So you do remind me that I didnt proposed *yet* the basic idea which
is critical.
what I wanted is to get all the patches ready and then commit them.
I must state I'm testing my code and not waiting until it breaks
something.
If you do want to know exactly what is being done or what I propose:
get the rewritten url from the external software.
put it in a char * variable (while NULLED at initialization).
I've been wondering if it has to be a char* for any particular reason
(likely deep in store code). If not please use String type provided by
Squid which will manage all the allocation/deallocation complexity for
you.
the access to it will be using the
mem_obj->(name:strore_uri,storeurl.. whatever)
the only really important place I have seen that any code should be
committed in other parts then redirect and friends is at the
getpublickey and friends.
The above is since squid only finds HIT by hash(has was discussed
before).
So the way to do it is pretty simple like it was before:
while accessing any place mem_obj->url (needed for has and friends)
it will transform into:
if (mem_obj->store__uri\url){
use store_uri\url in operation
}
else {
use uri\url in operation
}
if you\someone need me to explain anything I'm here for it.
Doing it the *right *way?
we are deciding if it's the right way so I will propose a working way
that seems efficient enough and that will work.
and a question:
since the http object has uri,log_uri we used store_uri to match the
others.
but at the mem-obj there is a url methods.
so do we want to stick with anything on the way? nameing the method
at mem_obj->storeurl ?
The 2.7 user storeurl for everything and I mean everything till the
last column.
So what do you think?
It seems to me like giving it a public name of storeurl is the more
convenient way for public naming.(config etc)
Thank you,
Alex.
NP
Eliezer
Amos
