Re: [PATCH] Ignore and warn about attempts to reconfigure static Rock store options.

On 09/20/2011 04:30 AM, Dmitry Kurochkin wrote:
> Hi Alex, Amos.
>
> On Mon, 19 Sep 2011 23:07:43 -0600, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:
>> On 09/19/2011 08:10 PM, Amos Jeffries wrote:
>>> On Tue, 20 Sep 2011 02:40:13 +0400, Dmitry Kurochkin wrote:
>>>> Attached patch does some polishing for Rock store cache_dir
>>>> reconfiguration.
>>>>
>>>>
>>>> Some Rock store options cannot be changed dynamically: path, size, and
>>>> max-size. Before the change, there were no checks during reconfigure
>>>> to prevent changing these options. This may lead to Rock cache
>>>> corruption and other bugs. The patch adds necessary checks to Rock
>>>> store code. If user tries to change an option that cannot be updated
>>>> dynamically, a warning is reported and the value is left unchanged.
>>
>>
>>
>>> Of itself the patch looks okay.
>>>
>>> BUT... IMO _path_ should never be re-configurable.
>>
>> Good point! I wonder whether admins expect cache_dir lines to be tied to
>> paths (e.g., changing cache_dir order changes nothing) or to position
>> (i.e., changing path can be supported). We should document what we
>> actually support (but that documentation change, if needed, is outside
>> this patch scope).
>>
>>
>>> If rock store can't handle multiple cache_dir of type rock with unique
>>> paths there is something deeper wrong.
>>
>> It can. Nothing special here.
>>
>>
>>> AFAIK path should be the unique key to identify different cache_dir.
>>> Changing the path means a completely different dir is now being
>>> referenced. If not already loaded the dir needs to be loaded as if on
>>> startup. If any existing are no longer referenced the old cache_dir
>>> details needs discarding.
>>
>> I tend to agree: Path-matching is probably better than position-matching.
>>
>> Dmitry, could you please check whether current code maps cache_dir lines
>> to SwapDir objects using cache_dir position in squid.conf or using
>> cache_dir paths? If it is the latter, you can remove the path-related
>> warning from the patch, right?
>>
>
> Cache dir mapping uses paths (though it ignores case). So it works as
> you would expect. I knew that when I was working on the patch and I
> still included the path check. Because it feels to me like we should
> check the path if it is passed to SwapDir::reconfigure(). Otherwise we
> should not pass path to SwapDir::reconfigure() at all. (There is also
> index parameter which should be checked or removed as well following the
> same arguments.) What do you think about:
>
> * remove path check from this patch
>
> * prepare another patch that removes index and path parameters from
> SwapDir::reconfigure()

Sounds like a good plan to me.

Thank you,

Alex.
Received on Tue Sep 20 2011 - 15:09:19 MDT