From: Ralph Boehme Date: Fri, 23 Nov 2012 17:23:24 +0000 (+0100) Subject: Reloading volumes from config file was broken X-Git-Url: https://arthur.barton.de/cgi-bin/gitweb.cgi?p=netatalk.git;a=commitdiff_plain;h=528159030a49a14bc9963b1b874c29080211c34c Reloading volumes from config file was broken load_volumes() is supposed to be a "reenetrant" function called from various places in different programs (afpd, cnid_metad) to reload the config for updating the volume list. Fix the loop freeing volumes that are deleted. Simplify the loop checking for paths and volume names in createvol(). This is also the loop which checks if a volume is already loaded when config has been loaded before. Remove checks for nested paths. Fixes bug #474. --- diff --git a/NEWS b/NEWS index b87cf651..6e2a1287 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,8 @@ Changes in 3.0.2 If /home -> /usr/home, set "basedir regex = /usr/home". * FIX: Memory leak * FIX: Copying packages to a Netatalk share could fail, bug #469 +* FIX: Reloading volumes from config file was broken. + Fixes bug #474. Changes in 3.0.1 ================ diff --git a/bin/ad/ad.c b/bin/ad/ad.c index 7f5b3da6..3de3030b 100644 --- a/bin/ad/ad.c +++ b/bin/ad/ad.c @@ -58,7 +58,7 @@ int main(int argc, char **argv) setuplog("default:note", "/dev/tty"); - if (load_volumes(&obj, NULL) != 0) + if (load_volumes(&obj) != 0) return 1; if (STRCMP(argv[1], ==, "ls")) diff --git a/etc/afpd/afp_config.c b/etc/afpd/afp_config.c index a61fe102..fa7da0c9 100644 --- a/etc/afpd/afp_config.c +++ b/etc/afpd/afp_config.c @@ -122,7 +122,7 @@ int configinit(AFPObj *obj) /* Now register with zeroconf, we also need the volumes for that */ if (! (obj->options.flags & OPTION_NOZEROCONF)) { - load_volumes(obj, NULL); + load_volumes(obj); zeroconf_register(obj); } diff --git a/etc/afpd/afp_dsi.c b/etc/afpd/afp_dsi.c index 3e5cd47c..11e54627 100644 --- a/etc/afpd/afp_dsi.c +++ b/etc/afpd/afp_dsi.c @@ -549,7 +549,7 @@ void afp_over_dsi(AFPObj *obj) if (reload_request) { reload_request = 0; - load_volumes(AFPobj, closevol); + load_volumes(AFPobj); } /* The first SIGINT enables debugging, the next restores the config */ diff --git a/etc/afpd/volume.c b/etc/afpd/volume.c index 61d8b408..172584d8 100644 --- a/etc/afpd/volume.c +++ b/etc/afpd/volume.c @@ -527,7 +527,7 @@ int afp_getsrvrparms(AFPObj *obj, char *ibuf _U_, size_t ibuflen _U_, char *rbuf int vcnt; size_t len; - load_volumes(obj, closevol); + load_volumes(obj); data = rbuf + 5; for ( vcnt = 0, volume = getvolumes(); volume; volume = volume->v_next ) { @@ -704,7 +704,7 @@ int afp_openvol(AFPObj *obj, char *ibuf, size_t ibuflen _U_, char *rbuf, size_t if ((len + 1) & 1) /* pad to an even boundary */ ibuf++; - load_volumes(obj, closevol); + load_volumes(obj); for ( volume = getvolumes(); volume; volume = volume->v_next ) { if ( strcasecmp_w( (ucs2_t*) volname, volume->v_name ) == 0 ) { diff --git a/etc/cnid_dbd/cmd_dbd.c b/etc/cnid_dbd/cmd_dbd.c index 8f8b7342..eb53af79 100644 --- a/etc/cnid_dbd/cmd_dbd.c +++ b/etc/cnid_dbd/cmd_dbd.c @@ -263,7 +263,7 @@ int main(int argc, char **argv) else setuplog("default:debug", "/dev/tty"); - if (load_volumes(&obj, NULL) != 0) { + if (load_volumes(&obj) != 0) { dbd_log( LOGSTD, "Couldn't load volumes"); exit(EXIT_FAILURE); } diff --git a/etc/cnid_dbd/cnid_metad.c b/etc/cnid_dbd/cnid_metad.c index dcf7d5fa..2fc1e8de 100644 --- a/etc/cnid_dbd/cnid_metad.c +++ b/etc/cnid_dbd/cnid_metad.c @@ -502,7 +502,7 @@ int main(int argc, char *argv[]) if (afp_config_parse(&obj, "cnid_metad") != 0) daemon_exit(1); - if (load_volumes(&obj, NULL) != 0) + if (load_volumes(&obj) != 0) daemon_exit(1); (void)setlimits(); @@ -604,7 +604,7 @@ int main(int argc, char *argv[]) LOG(log_debug, logtype_cnid, "main: request for volume: %s", volpath); - if (load_volumes(&obj, NULL) != 0) { + if (load_volumes(&obj) != 0) { LOG(log_severe, logtype_cnid, "main: error reloading config"); goto loop_end; } diff --git a/etc/cnid_dbd/main.c b/etc/cnid_dbd/main.c index 8ed0e5ff..df29d151 100644 --- a/etc/cnid_dbd/main.c +++ b/etc/cnid_dbd/main.c @@ -314,7 +314,7 @@ int main(int argc, char *argv[]) EC_ZERO( afp_config_parse(&obj, "cnid_dbd") ); - EC_ZERO( load_volumes(&obj, NULL) ); + EC_ZERO( load_volumes(&obj) ); EC_NULL( vol = getvolbypath(&obj, volpath) ); EC_ZERO( load_charset(vol) ); pack_setvol(vol); diff --git a/include/atalk/netatalk_conf.h b/include/atalk/netatalk_conf.h index cadb0aa3..efa4dba4 100644 --- a/include/atalk/netatalk_conf.h +++ b/include/atalk/netatalk_conf.h @@ -23,7 +23,7 @@ extern int afp_config_parse(AFPObj *obj, char *processname); extern int load_charset(struct vol *vol); -extern int load_volumes(AFPObj *obj, void (*delvol_fn)(const AFPObj *obj, struct vol *)); +extern int load_volumes(AFPObj *obj); extern void unload_volumes(AFPObj *obj); extern struct vol *getvolumes(void); extern struct vol *getvolbyvid(const uint16_t); diff --git a/libatalk/util/netatalk_conf.c b/libatalk/util/netatalk_conf.c index 36fdfbed..ff2ccb7a 100644 --- a/libatalk/util/netatalk_conf.c +++ b/libatalk/util/netatalk_conf.c @@ -567,7 +567,6 @@ static struct vol *creatvol(AFPObj *obj, { EC_INIT; struct vol *volume = NULL; - size_t current_pathlen, another_pathlen; int i, suffixlen, vlen, tmpvlen, u8mvlen, macvlen; char tmpname[AFPVOL_U8MNAMELEN+1]; char path[MAXPATHLEN + 1]; @@ -592,37 +591,26 @@ static struct vol *creatvol(AFPObj *obj, /* Once volumes are loaded, we never change options again, we just delete em when they're removed from afp.conf */ - /* - * Check for duplicated or nested volumes, eg: - * /Volumes/name /Volumes/name [duplicate], and - * /Volumes/name /Volumes/name/dir [nested], but beware of simple strncmp test: - * /Volumes/name /Volumes/name1 [strncmp match if n=strlen("Volumes/name") -> false positive] - */ - current_pathlen = strlen(path); for (struct vol *vol = Volumes; vol; vol = vol->v_next) { - another_pathlen = strlen(vol->v_path); - if (strncmp(path, vol->v_path, MIN(current_pathlen, another_pathlen)) == 0) { - if (current_pathlen == another_pathlen) { - LOG(log_error, logtype_afpd, "volume \"%s\" paths is duplicated: \"%s\"", name, path); - vol->v_deleted = 0; - volume = vol; - goto EC_CLEANUP; - } else { - const char *shorter_path, *longer_path; - int shorter_len; - if (another_pathlen > current_pathlen) { - shorter_len = current_pathlen; - shorter_path = path; - longer_path = vol->v_path; - } else { - shorter_len = another_pathlen; - shorter_path = vol->v_path; - longer_path = path; - } - if (longer_path[shorter_len] == '/') - LOG(log_info, logtype_afpd, "volume \"%s\" paths are nested: \"%s\" and \"%s\"", name, path, vol->v_path); - } + if (STRCMP(name, ==, vol->v_localname) && vol->v_deleted) { + /* + * reloading config, volume still present, nothing else to do, + * we don't change options for volumes once they're loaded + */ + vol->v_deleted = 0; + EC_EXIT_STATUS(0); } + if (STRCMP(path, ==, vol->v_path)) { + LOG(log_note, logtype_afpd, "volume \"%s\" path \"%s\" is the same as volumes \"%s\" path", + name, path, vol->v_configname); + EC_EXIT_STATUS(0); + + } + /* + * We could check for nested volume paths here, but + * nobody was able to come up with an implementation yet, + * that is simple, fast and correct. + */ } /* @@ -944,10 +932,8 @@ static struct vol *creatvol(AFPObj *obj, EC_CLEANUP: LOG(log_debug, logtype_afpd, "createvol: END: %d", ret); if (ret != 0) { - if (volume) { + if (volume) volume_free(volume); - free(volume); - } return NULL; } return volume; @@ -1268,12 +1254,14 @@ void volume_unlink(struct vol *volume) } /*! - * Free all resources allocated in a struct vol, only struct dir *v_root can't be freed + * Free all resources allocated in a struct vol in load_volumes() + * + * Actually opening a volume (afp_openvol()) will allocate additional + * ressources which are freed in closevol() */ void volume_free(struct vol *vol) { - LOG(log_debug, logtype_afpd, "volume_free('%s'): BEGIN", vol->v_localname); - + free(vol->v_configname); free(vol->v_localname); free(vol->v_u8mname); free(vol->v_macname); @@ -1288,10 +1276,12 @@ void volume_free(struct vol *vol) free(vol->v_uuid); free(vol->v_cnidserver); free(vol->v_cnidport); + free(vol->v_preexec); free(vol->v_root_preexec); free(vol->v_postexec); + free(vol->v_root_postexec); - LOG(log_debug, logtype_afpd, "volume_free: END"); + free(vol); } /*! @@ -1320,7 +1310,7 @@ int load_charset(struct vol *vol) * @param obj (r) handle * @param delvol_fn (r) callback called for deleted volumes */ -int load_volumes(AFPObj *obj, void (*delvol_fn)(const AFPObj *obj, struct vol *)) +int load_volumes(AFPObj *obj) { EC_INIT; int fd = -1; @@ -1372,12 +1362,24 @@ int load_volumes(AFPObj *obj, void (*delvol_fn)(const AFPObj *obj, struct vol *) EC_ZERO_LOG( readvolfile(obj, pwent) ); - for ( vol = Volumes; vol; vol = vol->v_next ) { - if (vol->v_deleted) { + struct vol *p, *prevvol; + + vol = Volumes; + prevvol = NULL; + + while (vol) { + if (vol->v_deleted && !(vol->v_flags & AFPVOL_OPEN)) { LOG(log_debug, logtype_afpd, "load_volumes: deleted: %s", vol->v_localname); - if (delvol_fn) - delvol_fn(obj, vol); - vol = Volumes; + if (prevvol) + prevvol->v_next = vol->v_next; + else + Volumes = NULL; + p = vol->v_next; + volume_free(vol); + vol = p; + } else { + prevvol = vol; + vol = vol->v_next; } } @@ -1391,12 +1393,16 @@ EC_CLEANUP: void unload_volumes(AFPObj *obj) { - struct vol *vol; + struct vol *vol, *p; LOG(log_debug, logtype_afpd, "unload_volumes: BEGIN"); - for (vol = Volumes; vol; vol = vol->v_next) + p = Volumes; + while (p) { + vol = p; + p = vol->v_next; volume_free(vol); + } Volumes = NULL; obj->options.volfile.mtime = 0;