Re: [PATCH]proposal for a fake_strore_url helper wrapping.

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
Received on Mon Sep 10 2012 - 22:52:35 MDT