==> Regarding Re: [patch rfc] address regression in duplicate map entry order;
Ian Kent <[EMAIL PROTECTED]> adds:
raven> On Mon, 22 Nov 2004, Jeff Moyer wrote:
>> ==> Regarding Re: [patch rfc] address regression in duplicate map entry
>> order; [EMAIL PROTECTED] adds:
>>
raven> On Fri, 19 Nov 2004, Jeff Moyer wrote:
>> >> Hi, Ian, list,
>> >>
>> >> In my proposed fix, I meant to say:
>> >>
>> >> We could expire the cache in lookup_ghost, and then go ahead and only
>> >> insert new entries (instead of overwriting what was there).
>> >>
>> >> So, I've implemented this, though it isn't tested just yet. There
>> were >> some other bugs.
>> >>
>> >> In lookup_multi, we iterate through each map's lookup_ghost function.
>> >> The problem here is that you will end up doing the following:
>> >>
>> >> static int read_map(const char *root, struct lookup_context *ctxt) {
>> >> char key[KEY_MAX_LEN + 1]; char mapent[MAPENT_MAX_LEN + 1]; char >>
>> *mapname; FILE *f; int entry; time_t age = time(NULL); >>
>> <=================
>> >>
>> >> mapname = alloca(strlen(ctxt->mapname) + 6); sprintf(mapname,
>> "file:%s", >> ctxt->mapname);
>> >>
>> >> f = fopen(ctxt->mapname, "r"); if (!f) { error(MODPREFIX "could not
>> open >> map file %s", ctxt->mapname); return 0; }
>> >>
>> >> while(1) { entry = read_one(f, key, mapent); if (entry) >>
>> cache_update(root, key, mapent, age); <==========
>> >>
>> >> if (feof(f)) break; }
>> >>
>> >> fclose(f);
>> >>
>> >> /* Clean stale entries from the cache */ cache_clean(root, age); >>
>> <=============
>> >>
>> >> return 1; }
>> >>
>> >> Notice the lines I've pointed out. We decide what the expiration age
>> >> should be at the beginning of the function. We update entries and
>> give >> them that "age." And finally, we expire all entries that were
>> made >> prior to that. The problem arises in that we call the read_map
>> routine >> for each map in the list in order. What will happen is that
>> each map, >> as it is processed, will end up with a new value for age.
>> Thus, we >> expire all of the work done by the previous read_maps! This
>> is bad. To >> fix this, I've passed in the age field from the
>> top-level.
>> >>
>> >> I've changed ldap's lookup_ghost routine (well, a function it calls,
>> >> anyway) to use cache_insert_unique instead of cache_add.
>> >>
>> >> Please let me know what you think. If you only like some parts, let
>> me >> know which ones and I'll break them out from the rest of the
>> patch. >> BTW, this patch is applied on top of your map expiration
>> changes. I can >> rediff the thing to any version of the code you want.
>>
raven> Understand the problem.
>>
raven> I've had a look at the patch and it looks good.
>>
raven> I had a slightly different idea though.
>>
raven> I think the cache_insert_unique might be a problem later as if
raven> people add multiple attributes (map entries) for multi-mount keys,
raven> instead of one attribute containing multiple mounts, in what they
raven> think is "the LDAP way" it probably won't work. I can't say we've
raven> seen this problem yet but I have been trying to allow for it in the
raven> code.
>>
raven> The other thing that occured to me was that, together with passing
raven> the time to keep things consistent, processing the maps in reverse
raven> order in lookup_ghost of the multi map module would fix most of the
raven> issues. Along with modifying cache_add to append to any existing
raven> entries, rather than prepend.
>>
raven> I spotted something you missed in the yp module. Have a look at the
raven> lookup_yp patch chunks in the attached map-entry-order patch.
>>
raven> Have a look at what I've tried to do and let me know what you
raven> think. I've done some basic testing but not against the issue we are
raven> trying to fix. So let me know how it goes.
>>
raven> Also, I found a couple of other bugs and have attached a couple of
raven> other patches as well.
>> Thanks, Ian! This all looks good to me, though I haven't been able to
>> test. What version of the tree was this applied to?
raven> That's a rather interesting question.
raven> I originally made the patch against your 55.1 but couldn't get
raven> rpmbuild to work OK under Gentoo so I added approiate patches to my
raven> Gentoo portage install and built there. Only minor changes were
raven> needed, which I believe would have been needed for the rpm as well.
raven> Hopefully, it will also apply to earlier rpms without much trouble
raven> as the changes appear to be mostly outside of the areas with lots of
raven> change.
In testing 4.1.4 beta 1, this problem still isn't addressed. Automount
will use the last of the duplicate entries in an indirect map, precisely
the opposite behaviour as before.
-Jeff
_______________________________________________
autofs mailing list
[email protected]
http://linux.kernel.org/mailman/listinfo/autofs
