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

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()

Regards,
  Dmitry

>
> Thank you,
>
> Alex.
Received on Tue Sep 20 2011 - 10:30:21 MDT