[Nmh-workers] about folder_addmsg.c

[markus schnalke]

Hoi,

Peter told me to test his changes to sbr/folder_addmsg.c some weeks
ago. I had a look at the code and came to the conclusion that
everything is fine with it. I also tested some corner-cases. I wanted
to write an automated test but unfortunately it never came to happen.

While digging through folder_addmsg.c I found the code unclear at some
places and there might also be logical bugs (maybe without effect). I
don't know. I haven't had enough time to dig further.

What I did stumble upon:

            /*
             * Check if the file in our desired location is the same
             * as the source file.  If so, then just leave it alone
             * and return.  Otherwise, we will continue the main loop
             * and try again at another slot (hghmsg+1).
             */
            if (linkerr == EEXIST) {
                if (stat (msgfile, &st2) == 0 && stat (newmsg, &st1) == 0
                    && st2.st_ino == st1.st_ino) {
                    return msgnum;
                } else {
                    continue;
                }
            }

What's the sense of this? It appears to be a check if the link had
already be done. Like if there would exist a second process that tries
to do the same at the same time but one wantes the action to happen
only once. This also could be a safety check from some earlier version
of the code.

The point is that I could not figure out why this check is needed. I
removed it and everything I had testen went still well. However, it
was just manual testing.

The VCS log shows nothing about it. It probably is very old.

Can someone explain?


Further more this code (following the above):

            /*
             * If link failed because we are trying to link
             * across devices, then check if there is a message
             * already in the desired location.  If so, then return
             * error, else just copy the message.
             */
            if (linkerr == EXDEV) {
                if (stat (newmsg, &st1) == 0) {
                    advise (NULL, "message %s:%s already exists", mp->foldpath, 
newmsg);
                    return -1;

Why the check for an existing message there? In all other cases we
would try again with the next number.

Again, the only explanation that sounds logical is that it is code to
handle some ancient cases which I am not aware of.



I also refacored the function heavily in order to make in clearer.
See attached patch. (Don't bug me because of the goto! I am convinced
that it is worthwhile there.)

Again I did some manual testing but can not guarantee that it behaves
exactly the same (modulo the two cases from above). A good test suite
would be helpful. :-)

I like to share this with you because I will be off for so long. Also
it is documented herewith and I can refer to it myself later on.


meillo


P.S.
folder_addmsg() is used in:
- uip/inc.c
- uip/mhstoresbr.c
- uip/rcvstore.c
- and uip/refile.c

diff -r ee0fbbb9f709 sbr/folder_addmsg.c
--- a/sbr/folder_addmsg.c       Tue Nov 30 15:30:45 2010 -0300
+++ b/sbr/folder_addmsg.c       Tue Dec 14 12:55:30 2010 -0300
@@ -14,6 +14,40 @@
 #include <errno.h>
 
 /*
+ * Copy the file if the link failed because of different devices.
+ * returns 1 on success, 0 on failure
+ */
+static int
+copymsg(char *msgfile, char *newmsg, int deleting, char* from_dir)
+{
+    int infd, outfd;
+    struct stat st1;
+    char oldmsg[BUFSIZ];
+
+    if ((infd = open (msgfile, O_RDONLY)) == -1) {
+        advise (msgfile, "unable to open message %s", msgfile);
+        return 0;
+    }
+    fstat (infd, &st1);
+    if ((outfd = creat (newmsg, (int) st1.st_mode & 0777)) == -1) {
+        advise (newmsg, "unable to create");
+        close (infd);
+        return 0;
+    }
+    cpydata (infd, outfd, msgfile, newmsg);
+    close (infd);
+    close (outfd);
+
+    if (deleting) {
+        (void)snprintf(oldmsg, sizeof (oldmsg), "%s/%s", from_dir, msgfile);
+        (void)ext_hook("ref-hook", oldmsg, newmsg);
+    } else
+        (void)ext_hook("add-hook", newmsg, (char *)0);
+
+    return 1;
+}
+
+/*
  * Link message into a folder.  Return the new number
  * of the message.  If an error occurs, return -1.
  */
@@ -22,198 +56,170 @@
 folder_addmsg (struct msgs **mpp, char *msgfile, int selected,
                int unseen, int preserve, int deleting, char *from_dir)
 {
-    int infd, outfd, linkerr, first_time, msgnum;
+    int linkerr, msgnum;
     char *nmsg, newmsg[BUFSIZ];
     char oldmsg[BUFSIZ];
     struct msgs *mp;
-    struct stat st1, st2;
+    struct stat st1;
 
-    first_time = 1;    /* this is first attempt */
     mp = *mpp;
 
     /*
+     * What number for the new message?
+     */
+    if (preserve && (msgnum = m_atoi (msgfile)) > 0) {
+       /* keep the same number */
+       ;
+    } else if (mp->nummsg == 0) {
+       /* we are adding to an empty folder */
+       msgnum = 1;
+    } else {
+       /* else use highest message number + 1 */
+       msgnum = mp->hghmsg + 1;
+    }
+
+    /*
      * We might need to make several attempts
      * in order to add the message to the folder.
+     * This is because the message number may appear used on later checks,
+     * though first thought unused.
      */
-    for (;;) {
-       /*
-        * Get the message number we will attempt to add.
-        */
-       if (first_time) {
-           /* should we preserve the numbering of the message? */
-           if (preserve && (msgnum = m_atoi (msgfile)) > 0) {
-               ;
-           } else if (mp->nummsg == 0) {
-               /* check if we are adding to empty folder */
-               msgnum = 1;
-           } else {
-               /* else use highest message number + 1 */
-               msgnum = mp->hghmsg + 1;
-           }
-           first_time = 0;
-       } else {
-           /* another attempt, so try next higher message number */
-           msgnum++;
+next_try:
+
+    /*
+     * See if we need more space.  If we need space at the
+     * end, then we allocate space for an addition 100 messages.
+     * (TODO: better increment. //meillo)
+     * If we need space at the beginning of the range, then just
+     * extend message status range to cover this message number.
+     */
+    if (msgnum > mp->hghoff) {
+       if ((mp = folder_realloc (mp, mp->lowoff, msgnum + 100)))
+           *mpp = mp;
+       else {
+           advise (NULL, "unable to allocate folder storage");
+           return -1;
        }
-
-       /*
-        * See if we need more space.  If we need space at the
-        * end, then we allocate space for an addition 100 messages.
-        * If we need space at the beginning of the range, then just
-        * extend message status range to cover this message number.
-         */
-       if (msgnum > mp->hghoff) {
-           if ((mp = folder_realloc (mp, mp->lowoff, msgnum + 100)))
-               *mpp = mp;
-           else {
-               advise (NULL, "unable to allocate folder storage");
-               return -1;
-           }
-       } else if (msgnum < mp->lowoff) {
-           if ((mp = folder_realloc (mp, msgnum, mp->hghoff)))
-               *mpp = mp;
-           else {
-               advise (NULL, "unable to allocate folder storage");
-               return -1;
-           }
-       }
-
-       /*
-        * If a message is already in that slot,
-        * then loop to next available slot.
-        */
-       if (does_exist (mp, msgnum))
-           continue;
-
-       /* setup the bit flags for this message */
-       clear_msg_flags (mp, msgnum);
-       set_exists (mp, msgnum);
-
-       /* should we set the SELECT_UNSEEN bit? */
-       if (unseen) {
-           set_unseen (mp, msgnum);
-       }
-
-       /* should we set the SELECTED bit? */
-       if (selected) {
-           set_selected (mp, msgnum);
-
-           /* check if highest or lowest selected */
-           if (mp->numsel == 0) {
-               mp->lowsel = msgnum;
-               mp->hghsel = msgnum;
-           } else {
-               if (msgnum < mp->lowsel)
-                   mp->lowsel = msgnum;
-               if (msgnum > mp->hghsel)
-                   mp->hghsel = msgnum;
-           }
-
-           /* increment number selected */
-           mp->numsel++;
-       }
-
-       /*
-        * check if this is highest or lowest message
-         */
-       if (mp->nummsg == 0) {
-           mp->lowmsg = msgnum;
-           mp->hghmsg = msgnum;
-       } else {
-           if (msgnum < mp->lowmsg)
-               mp->lowmsg = msgnum;
-           if (msgnum > mp->hghmsg)
-               mp->hghmsg = msgnum;
-       }
-
-       /* increment message count */
-       mp->nummsg++;
-
-       nmsg = m_name (msgnum);
-       snprintf (newmsg, sizeof(newmsg), "%s/%s", mp->foldpath, nmsg);
-
-       /*
-        * Now try to link message into folder.
-        * Then run the external hook on the message if one was specified in 
the context.
-        * Run the refile hook if we're moving the message from one place to 
another.
-        * We have to construct the from path name for this because it's not 
there.
-        * Run the add hook if the message is getting copied or linked 
somewhere else.
-        */
-       if (link (msgfile, newmsg) != -1) {
-
-           if (deleting) {
-               (void)snprintf(oldmsg, sizeof (oldmsg), "%s/%s", from_dir, 
msgfile);
-               (void)ext_hook("ref-hook", oldmsg, newmsg);
-           }
-           else
-               (void)ext_hook("add-hook", newmsg, (char *)0);
-
-           return msgnum;
-       } else {
-           linkerr = errno;
-
-#ifdef EISREMOTE
-           if (linkerr == EISREMOTE)
-               linkerr = EXDEV;
-#endif /* EISREMOTE */
-
-           /*
-            * Check if the file in our desired location is the same
-            * as the source file.  If so, then just leave it alone
-            * and return.  Otherwise, we will continue the main loop
-            * and try again at another slot (hghmsg+1).
-            */
-           if (linkerr == EEXIST) {
-               if (stat (msgfile, &st2) == 0 && stat (newmsg, &st1) == 0
-                   && st2.st_ino == st1.st_ino) {
-                   return msgnum;
-               } else {
-                   continue;
-               }
-           }
-
-           /*
-            * If link failed because we are trying to link
-            * across devices, then check if there is a message
-            * already in the desired location.  If so, then return
-            * error, else just copy the message.
-            */
-           if (linkerr == EXDEV) {
-               if (stat (newmsg, &st1) == 0) {
-                   advise (NULL, "message %s:%s already exists", mp->foldpath, 
newmsg);
-                   return -1;
-               } else {
-                   if ((infd = open (msgfile, O_RDONLY)) == -1) {
-                       advise (msgfile, "unable to open message %s", msgfile);
-                       return -1;
-                   }
-                   fstat (infd, &st1);
-                   if ((outfd = creat (newmsg, (int) st1.st_mode & 0777)) == 
-1) {
-                       advise (newmsg, "unable to create");
-                       close (infd);
-                       return -1;
-                   }
-                   cpydata (infd, outfd, msgfile, newmsg);
-                   close (infd);
-                   close (outfd);
-
-                   if (deleting) {
-                       (void)snprintf(oldmsg, sizeof (oldmsg), "%s/%s", 
from_dir, msgfile);
-                       (void)ext_hook("ref-hook", oldmsg, newmsg);
-                   }
-                   else
-                       (void)ext_hook("add-hook", newmsg, (char *)0);
-
-                   return msgnum;
-               }
-           }
-
-           /*
-            * Else, some other type of link error,
-            * so just return error.
-            */
-           advise (newmsg, "error linking %s to", msgfile);
+    } else if (msgnum < mp->lowoff) {
+       if ((mp = folder_realloc (mp, msgnum, mp->hghoff)))
+           *mpp = mp;
+       else {
+           advise (NULL, "unable to allocate folder storage");
            return -1;
        }
     }
+
+    /*
+     * If a message is already in that slot,
+     * then try again with next available slot.
+     */
+    if (does_exist (mp, msgnum)) {
+       msgnum++;
+       goto next_try;
+    }
+
+    /* setup the bit flags for this message */
+    clear_msg_flags (mp, msgnum);
+    set_exists (mp, msgnum);
+
+    /* should we set the SELECT_UNSEEN bit? */
+    if (unseen) {
+       set_unseen (mp, msgnum);
+    }
+
+    /* should we set the SELECTED bit? */
+    if (selected) {
+       set_selected (mp, msgnum);
+
+       /* check if highest or lowest selected */
+       if (mp->numsel == 0) {
+           mp->lowsel = msgnum;
+           mp->hghsel = msgnum;
+       } else {
+           if (msgnum < mp->lowsel)
+               mp->lowsel = msgnum;
+           if (msgnum > mp->hghsel)
+               mp->hghsel = msgnum;
+       }
+
+       /* increment number selected */
+       mp->numsel++;
+    }
+
+    /*
+     * check if this is highest or lowest message
+     */
+    if (mp->nummsg == 0) {
+       mp->lowmsg = msgnum;
+       mp->hghmsg = msgnum;
+    } else {
+       if (msgnum < mp->lowmsg)
+           mp->lowmsg = msgnum;
+       if (msgnum > mp->hghmsg)
+           mp->hghmsg = msgnum;
+    }
+
+    /* increment message count */
+    mp->nummsg++;
+
+    nmsg = m_name (msgnum);
+    snprintf (newmsg, sizeof(newmsg), "%s/%s", mp->foldpath, nmsg);
+
+    /*
+     * Now try to link message into folder.
+     * Then run the external hook on the message if one was specified in the 
context.
+     * Run the refile hook if we're moving the message from one place to 
another.
+     * We have to construct the from path name for this because it's not there.
+     * Run the add hook if the message is getting copied or linked somewhere 
else.
+     */
+    if (link (msgfile, newmsg) != -1) {
+
+       if (deleting) {
+           (void)snprintf(oldmsg, sizeof (oldmsg), "%s/%s", from_dir, msgfile);
+           (void)ext_hook("ref-hook", oldmsg, newmsg);
+       } else
+           (void)ext_hook("add-hook", newmsg, (char *)0);
+
+       return msgnum;
+    }
+
+    /* link failed */
+    linkerr = errno;
+
+    if (linkerr == EEXIST) {
+       /* The target existed, so try again with next slot. */
+       msgnum++;
+       goto next_try;
+    }
+
+#ifdef EISREMOTE
+    if (linkerr == EISREMOTE)
+       linkerr = EXDEV;
+#endif /* EISREMOTE */
+
+    if (linkerr == EXDEV) {
+       /*
+        * Link failed because we were trying to link
+        * across devices. Check if there is a message
+        * already in the desired location.  If so, then try again with
+        * next slot, else just copy the message.
+        */
+       if (stat (newmsg, &st1) == 0) {
+           msgnum++;
+           goto next_try;
+       }
+
+       if (copymsg(msgfile, newmsg, deleting, from_dir))
+           return msgnum;
+
+       return -1;
+    }
+
+    /*
+     * Some other type of link error,
+     * so just return error.
+     */
+    advise (newmsg, "error linking %s to", msgfile);
+    return -1;
+
 }