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.
Understand the problem.
I've had a look at the patch and it looks good.
I had a slightly different idea though.
I think the cache_insert_unique might be a problem later as if people add
multiple attributes (map entries) for multi-mount keys, instead of one
attribute containing multiple mounts, in what they think is "the LDAP way"
it probably won't work. I can't say we've seen this problem yet but I have
been trying to allow for it in the code.
The other thing that occured to me was that, together with passing the
time to keep things consistent, processing the maps in reverse order in
lookup_ghost of the multi map module would fix most of the issues. Along
with modifying cache_add to append to any existing entries, rather than
prepend.
I spotted something you missed in the yp module. Have a look at the
lookup_yp patch chunks in the attached map-entry-order patch.
Have a look at what I've tried to do and let me know what you think. I've
done some basic testing but not against the issue we are trying to fix. So
let me know how it goes.
Also, I found a couple of other bugs and have attached a couple of other
patches as well.
Ian
--- autofs-4.1.3/daemon/automount.c.map-entry-order 2004-11-21
12:55:38.000000000 +0800
+++ autofs-4.1.3/daemon/automount.c 2004-11-21 12:55:39.000000000 +0800
@@ -825,7 +825,7 @@ static int st_readmap(void)
{
int status;
- status = ap.lookup->lookup_ghost(ap.path, ap.ghost, ap.lookup->context);
+ status = ap.lookup->lookup_ghost(ap.path, ap.ghost, 0,
ap.lookup->context);
debug("st_readmap: status %d\n", status);
@@ -1502,7 +1502,7 @@ static void sig_supervisor(int sig)
break;
case SIGHUP:
- ap.lookup->lookup_ghost(ap.path, ap.ghost, ap.lookup->context);
+ ap.lookup->lookup_ghost(ap.path, ap.ghost, 0,
ap.lookup->context);
/* Pass on the reread event and ignore self signal */
kill(0, SIGHUP);
@@ -1520,7 +1520,7 @@ int supervisor(char *path)
ap.path = alloca(strlen(path) + 1);
strcpy(ap.path, path);
- map = ap.lookup->lookup_ghost(ap.path, ap.ghost, ap.lookup->context);
+ map = ap.lookup->lookup_ghost(ap.path, ap.ghost, 0, ap.lookup->context);
if (map & LKP_FAIL) {
error("failed to load map exiting");
cleanup_exit(ap.path, 1);
@@ -1588,7 +1588,7 @@ int handle_mounts(char *path)
alarm(ap.exp_runfreq + my_pid % ap.exp_runfreq);
}
- map = ap.lookup->lookup_ghost(ap.path, ap.ghost, ap.lookup->context);
+ map = ap.lookup->lookup_ghost(ap.path, ap.ghost, 0, ap.lookup->context);
if (map & LKP_FAIL) {
if (map & LKP_INDIRECT) {
error("bad map format: found indirect, "
--- autofs-4.1.3/include/automount.h.map-entry-order 2004-11-21
12:55:37.000000000 +0800
+++ autofs-4.1.3/include/automount.h 2004-11-21 12:55:39.000000000 +0800
@@ -140,12 +140,12 @@ int is_mounted(const char *path);
#ifdef MODULE_LOOKUP
int lookup_init(const char *mapfmt, int argc, const char *const *argv, void
**context);
-int lookup_ghost(const char *, int, void *);
+int lookup_ghost(const char *, int, time_t, void *);
int lookup_mount(const char *, const char *, int, void *);
int lookup_done(void *);
#endif
typedef int (*lookup_init_t) (const char *, int, const char *const *, void **);
-typedef int (*lookup_ghost_t) (const char *, int, void *);
+typedef int (*lookup_ghost_t) (const char *, int, time_t, void *);
typedef int (*lookup_mount_t) (const char *, const char *, int, void *);
typedef int (*lookup_done_t) (void *);
--- autofs-4.1.3/lib/cache.c.map-entry-order 2004-11-21 12:55:37.000000000
+0800
+++ autofs-4.1.3/lib/cache.c 2004-11-21 12:58:08.000000000 +0800
@@ -165,7 +165,7 @@ struct mapent_cache *cache_partial_match
int cache_add(const char *root, const char *key, const char *mapent, time_t
age)
{
- struct mapent_cache *me = NULL;
+ struct mapent_cache *me = NULL, *existing = NULL;
char *pkey, *pent;
unsigned int hashval = hash(key);
char *path;
@@ -191,8 +191,27 @@ int cache_add(const char *root, const ch
me->mapent = strcpy(pent, mapent);
me->age = age;
- me->next = mapent_hash[hashval];
- mapent_hash[hashval] = me;
+ /*
+ * We need to add to the end if values exist in order to
+ * preserve the order in which the map was read on lookup.
+ */
+ existing = cache_lookup(key);
+ if (!existing) {
+ me->next = mapent_hash[hashval];
+ mapent_hash[hashval] = me;
+ } else {
+ while (1) {
+ struct mapent_cache *next;
+
+ next = cache_lookup_next(existing);
+ if (!next)
+ break;
+
+ existing = next;
+ }
+ me->next = existing->next;
+ existing->next = me;
+ }
return CHE_OK;
}
--- autofs-4.1.3/modules/lookup_file.c.map-entry-order 2004-11-21
12:55:38.000000000 +0800
+++ autofs-4.1.3/modules/lookup_file.c 2004-11-21 12:55:39.000000000 +0800
@@ -227,14 +227,14 @@ static int read_one(FILE *f, char *key,
return 0;
}
-static int read_map(const char *root, struct lookup_context *ctxt)
+static int read_map(const char *root, time_t now, 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);
+ time_t age = now ? now : time(NULL);
mapname = alloca(strlen(ctxt->mapname) + 6);
sprintf(mapname, "file:%s", ctxt->mapname);
@@ -262,14 +262,14 @@ static int read_map(const char *root, st
return 1;
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
struct lookup_context *ctxt = (struct lookup_context *) context;
struct mapent_cache *me;
struct stat st;
int status = 1;
- if (!read_map(root, ctxt))
+ if (!read_map(root, now, ctxt))
return LKP_FAIL;
if (stat(ctxt->mapname, &st)) {
--- autofs-4.1.3/modules/lookup_hesiod.c.map-entry-order 2004-11-21
12:55:37.000000000 +0800
+++ autofs-4.1.3/modules/lookup_hesiod.c 2004-11-21 12:55:39.000000000
+0800
@@ -66,7 +66,7 @@ int lookup_init(const char *mapfmt, int
return !(ctxt->parser = open_parse(mapfmt, MODPREFIX, argc - 1, argv +
1));
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
return LKP_NOTSUP;
}
--- autofs-4.1.3/modules/lookup_ldap.c.map-entry-order 2004-11-21
12:55:38.000000000 +0800
+++ autofs-4.1.3/modules/lookup_ldap.c 2004-11-21 12:55:39.000000000 +0800
@@ -174,10 +174,9 @@ static int read_one_map(const char *root
const char *class, char *key,
const char *keyval, int keyvallen, char *type,
struct lookup_context *ctxt,
- int *result_ldap)
+ time_t age, int *result_ldap)
{
int rv, i, l, count;
- time_t age = time(NULL);
char *query;
LDAPMessage *result, *e;
char **keyValue = NULL;
@@ -280,17 +279,16 @@ static int read_one_map(const char *root
}
static int read_map(const char *root, struct lookup_context *ctxt,
- const char *key, int keyvallen, int *result_ldap)
+ const char *key, int keyvallen, time_t age, int
*result_ldap)
{
- time_t age = time(NULL);
int rv = LDAP_SUCCESS;
/* all else fails read entire map */
if (!read_one_map(root, "nisObject", "cn",
- key, keyvallen, "nisMapEntry", ctxt, &rv)) {
+ key, keyvallen, "nisMapEntry", ctxt, age, &rv)) {
if ((rv != LDAP_SUCCESS) ||
!read_one_map(root, "automount", "cn", key, keyvallen,
- "automountInformation", ctxt, &rv)) {
+ "automountInformation", ctxt, age, &rv)) {
if (result_ldap != NULL)
*result_ldap = rv;
return 0;
@@ -303,16 +301,17 @@ static int read_map(const char *root, st
return 1;
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
struct lookup_context *ctxt = (struct lookup_context *) context;
struct mapent_cache *me;
int status = 1, rv = LDAP_SUCCESS;
char *mapname;
+ time_t age = now ? now : time(NULL);
chdir("/");
- if (!read_map(root, ctxt, NULL, 0, &rv))
+ if (!read_map(root, ctxt, NULL, 0, age, &rv))
switch (rv) {
case LDAP_SIZELIMIT_EXCEEDED:
case LDAP_UNWILLING_TO_PERFORM:
--- autofs-4.1.3/modules/lookup_multi.c.map-entry-order 2004-11-21
12:55:38.000000000 +0800
+++ autofs-4.1.3/modules/lookup_multi.c 2004-11-21 12:55:39.000000000 +0800
@@ -111,13 +111,17 @@ int lookup_init(const char *my_mapfmt, i
return 1;
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
struct lookup_context *ctxt = (struct lookup_context *) context;
int i, ret, at_least_1 = 0;
- for (i = 0; i < ctxt->n; i++) {
- ret = ctxt->m[i].mod->lookup_ghost(root, ghost,
+ /*
+ * Read each map in reverse order to ensure that when we lookup a key
+ * in the cache we get what would be seen first.
+ */
+ for (i = ctxt->n - 1; i >= 0 ; i--) {
+ ret = ctxt->m[i].mod->lookup_ghost(root, ghost, now,
ctxt->m[i].mod->context);
if (ret & LKP_FAIL)
continue;
--- autofs-4.1.3/modules/lookup_nisplus.c.map-entry-order 2004-01-30
00:01:22.000000000 +0800
+++ autofs-4.1.3/modules/lookup_nisplus.c 2004-11-21 12:55:39.000000000
+0800
@@ -59,7 +59,7 @@ int lookup_init(const char *mapfmt, int
return !(ctxt->parse = open_parse(mapfmt, MODPREFIX, argc - 1, argv +
1));
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
return LKP_NOTSUP;
}
--- autofs-4.1.3/modules/lookup_program.c.map-entry-order 2004-11-21
12:55:38.000000000 +0800
+++ autofs-4.1.3/modules/lookup_program.c 2004-11-21 12:55:39.000000000
+0800
@@ -76,7 +76,7 @@ int lookup_init(const char *mapfmt, int
return !(ctxt->parse = open_parse(mapfmt, MODPREFIX, argc - 1, argv +
1));
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
return LKP_NOTSUP;
}
--- autofs-4.1.3/modules/lookup_userhome.c.map-entry-order 2004-01-30
00:01:22.000000000 +0800
+++ autofs-4.1.3/modules/lookup_userhome.c 2004-11-21 12:55:39.000000000
+0800
@@ -36,7 +36,7 @@ int lookup_init(const char *mapfmt, int
return 0; /* Nothing to do */
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
return LKP_NOTSUP;
}
--- autofs-4.1.3/modules/lookup_yp.c.map-entry-order 2004-11-21
12:55:37.000000000 +0800
+++ autofs-4.1.3/modules/lookup_yp.c 2004-11-21 12:55:39.000000000 +0800
@@ -44,16 +44,8 @@ struct lookup_context {
};
struct callback_data {
- int status;
- int map;
const char *root;
- char direct_base[KEY_MAX_LEN + 1];
- const char *name;
- int name_len;
- unsigned long type;
- const char *mapname;
time_t age;
- struct lookup_context *context;
};
int lookup_version = AUTOFS_LOOKUP_VERSION; /* Required by protocol */
@@ -93,8 +85,9 @@ int lookup_init(const char *mapfmt, int
int yp_all_callback(int status, char *ypkey, int ypkeylen,
char *val, int vallen, char *ypcb_data)
{
- time_t age = time(NULL);
- char *root = ypcb_data;
+ struct callback_data *cbdata = (struct callback_data *) ypcb_data;
+ char *root = cbdata->root;
+ time_t age = cbdata->age;
char *key;
char *mapent;
@@ -114,15 +107,18 @@ int yp_all_callback(int status, char *yp
return 0;
}
-static int read_map(const char *root, struct lookup_context *context)
+static int read_map(const char *root, time_t age, struct lookup_context
*context)
{
struct lookup_context *ctxt = (struct lookup_context *) context;
struct ypall_callback ypcb;
- time_t age = time(NULL);
+ struct callback_data ypcb_data;
int err;
+ ypcb_data.root = root;
+ ypcb_data.age = age;
+
ypcb.foreach = yp_all_callback;
- ypcb.data = (char *) root;
+ ypcb.data = (char *) &ypcb_data;
err = yp_all((char *) ctxt->domainname, (char *) ctxt->mapname, &ypcb);
@@ -138,13 +134,14 @@ static int read_map(const char *root, st
return 1;
}
-int lookup_ghost(const char *root, int ghost, void *context)
+int lookup_ghost(const char *root, int ghost, time_t now, void *context)
{
struct lookup_context *ctxt = (struct lookup_context *) context;
+ time_t age = now ? now : time(NULL);
struct mapent_cache *me;
int status = 1;
- if (!read_map(root, ctxt))
+ if (!read_map(root, age, ctxt))
return LKP_FAIL;
status = cache_ghost(root, ghost, ctxt->mapname, "yp", ctxt->parse);
--- autofs-4.1.3/modules/lookup_yp.c.direct-lookup 2004-11-21
16:29:49.000000000 +0800
+++ autofs-4.1.3/modules/lookup_yp.c 2004-11-21 16:30:26.000000000 +0800
@@ -235,7 +235,7 @@ int lookup_mount(const char *root, const
return 1;
/* check map and if change is detected re-read map */
- ret = lookup_one(root, name, name_len, ctxt);
+ ret = lookup_one(root, key, key_len, ctxt);
debug("ret = %d", ret);
--- autofs-4.1.3/modules/lookup_file.c.map-expiry-fix 2004-11-21
21:35:52.000000000 +0800
+++ autofs-4.1.3/modules/lookup_file.c 2004-11-21 21:37:08.000000000 +0800
@@ -323,8 +323,10 @@ static int lookup_one(const char *root,
while(1) {
entry = read_one(f, mkey, mapent);
if (entry)
- if (strncmp(mkey, key, key_len) == 0)
+ if (strncmp(mkey, key, key_len) == 0) {
+ fclose(f);
return cache_update(root, key, mapent, age);
+ }
if (feof(f))
break;
_______________________________________________
autofs mailing list
[EMAIL PROTECTED]
http://linux.kernel.org/mailman/listinfo/autofs
