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

On Tue, 20 Sep 2011 09:08:33 -0600, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:
> 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.
>

Attached patch without the path check.

Regards,
  Dmitry

>
> Thank you,
>
> Alex.