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 b87cf65183a0de28803e3f63748fe4dc8f90918c..6e2a1287386214904f73db8dfd45dc8e4873faae 100644 (file)
--- 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 7f5b3da6260e626ae210dfa71c82e13777a55240..3de3030b9a820bbde3c53caef74a27ac1b769482 100644 (file)
--- 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 a61fe1022f551ccc84b9fef03c860cfe16c51a30..fa7da0c9816679975cd9de02722dd3192e546d89 100644 (file)
--- 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 3e5cd47c3cbf4de4438bc6e3e0e2585a9be72417..11e546271df70f8f3a4e0b0018234198ff9e5fa1 100644 (file)
--- 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 61d8b4086b8a6670959ff51c1dcb157a8ce562c2..172584d8764d2532d41353fa1def11a1963db3e9 100644 (file)
--- 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 8f8b734216fb2a9413d1ffde668d2ff5fb40b5f1..eb53af7943a54c4396280c3f5d636574d751aaa5 100644 (file)
--- 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 dcf7d5faaa78b89fb68f866a979d3f5c55d7b4af..2fc1e8debe0c10ebf5846676f31bb6c10a8ca00d 100644 (file)
--- 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 8ed0e5ff2bcd37796511c3094689ce3b7dfb6da8..df29d15198d470e73cab6a200121aedfb5236769 100644 (file)
--- 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 cadb0aa3d963b69691092b8dd11a75bf57ea84d6..efa4dba49456dddcb8b0b580083e5fb9b1a2ad08 100644 (file)
--- 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 36fdfbedb310626afc2653505639774d4efa7e78..ff2ccb7adc91b3c0478502df6f5f4cd8c33d701e 100644 (file)
--- 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;