Re: serious bug. Evolution and Microsoft mentality.
On Wed, 2002-01-09 at 01:43, Jonathan Walther wrote: > On Mon, Jan 07, 2002 at 01:56:24PM -0500, Jeffrey Stedfast wrote: > >Yea, this is kinda painful currently but hopefully by 1.2 this will be > >much easier. We plan on making it so that you can add a new account > >using "Standard Unix Mail Spool" as the source type and pointing it at a > >directory and have our code automatically show all the mailboxes in the > >directory. Currently it will only accept single mbox files as sources. > > That will be very nice. Will you also let us point to Maildir's and > have it Just Work? Maildir format allows us to continue using procmail > and related programs without worrying about locking issues between > Evolution and outside mail delivery programs and filters. Actually, Evolution already supports pointing to outside Maildir directories ;-) > > >It's not really a good idea to use symlinks because Evolution will lock > >the symlink file and if you have procmail running, it will lock the real > >mbox file and so if you try and access the same mbox file with both > >Evolution and procmail, things will end badly for you (ie corrupt > >mailbox). > > Correct. Thats why I plan to switch over completely to Maildir format. > > >> What happened? My symlink was erased! Gone! A new mbox file had been > >> created from the contents of the original. > > > >Did you delete/expunge messages? I presume that you must have because > >otherwise the mbox would not have been rewritten. The only way to remove > > Unfortunately, no. I made no changes whatsoever to the mailboxes. I > just entered them to see if the messages showed up, they did, then I > exited. Thats when I noticed the symlinks had been blown away, and the > resulting "copied" mailbox was bigger in size than the original! Ah, I bet I know why... Evolution added an X-Evolution header to each message for status purposes. The X-Evolution header contains an encoded UID and message flags. That would also explain why it got bigger than the original file. > > >messages from an mbox file is to rewrite it to a new tmp file (minus the > >messages to be deleted) and then rename that tmp file back to its > >original name. Unfortunately, UNIX's rename() function clobbers symlinks > >and thus your problem. > > Well, alright. I did some research tonight. Before calling "rename()", > a call to realpath() would resolve the symlink to the correct "real" > filename, and using that would give the correct result. An extra 3 > lines of code (to find out how much storage is needed for the real path, > to allocate it, then finally to call realpath()). Actually, this isn't guaranteed to work :-( rename() is unfortunately limited to renaming files on the same file system, once you add symlinks to the equation - you could be leaping across file system boundaries and there isn't a way to tell (well, ok, so you can check st.st_dev and compare the 2). The only way I can think of to get around this is to create the tmp mbox file in the same directory as the original (after being realpath()'d). This may also have problems - what if you don't have write-permission in that directory? > > >Actually, Evolution plays very nice with other mail software on the > >system - you just have to not use symlinks. > > Supporting symlinks is easy. Not supporting them is un-Unixy. Not as easy as you seem to think, you missed that rename doesn't work across file systems ;-) Anyways, I'll look into it... > > >Evolution locks all > >mailboxes when it goes to access them to avoid mailbox corruption and so > >as long as the other system mail software does the same (which it > >should) then they will all live in harmony. > > Again, provided one uses Maildir mailboxes, things will be fine. But > the thought occurs, Evolution should do its "locking" on the file > returned from realpath() too. You are probably right. -- Jeffrey Stedfast Evolution Hacker - Ximian, Inc. [EMAIL PROTECTED] - www.ximian.com
Re: serious bug. Evolution and Microsoft mentality.
On Wed, 2002-01-09 at 16:20, Jonathan Walther wrote:
> On Wed, Jan 09, 2002 at 01:41:28PM -0500, Jeffrey Stedfast wrote:
> >> Unfortunately, no. I made no changes whatsoever to the mailboxes. I
> >> just entered them to see if the messages showed up, they did, then I
> >> exited. Thats when I noticed the symlinks had been blown away, and the
> >> resulting "copied" mailbox was bigger in size than the original!
> >
> >Ah, I bet I know why... Evolution added an X-Evolution header to each
> >message for status purposes. The X-Evolution header contains an encoded
> >UID and message flags. That would also explain why it got bigger than
> >the original file.
>
> Fair enough. Pine does something similar. Can it be gauranteed that
> the Quoted Printable encoding hasn't been decoded then reencoded in
> the process?
not easily. Michael Zucchi and I will have to redesign libcamel to be
able to guarantee this and so must wait until we have time.
>
> >Actually, this isn't guaranteed to work :-(
> >rename() is unfortunately limited to renaming files on the same file
> >system, once you add symlinks to the equation - you could be leaping
> >across file system boundaries and there isn't a way to tell (well, ok,
> >so you can check st.st_dev and compare the 2).
> >
> >The only way I can think of to get around this is to create the tmp mbox
> >file in the same directory as the original (after being realpath()'d).
> >This may also have problems - what if you don't have write-permission in
> >that directory?
>
> You solved the problem. That is the correct solution. After running
> realpath(), use dirname() and make the tmpfile in the same directory
> as the mailbox.
>
[snip]
The attached patch will fix this issue.
>
> >> Again, provided one uses Maildir mailboxes, things will be fine. But
> >> the thought occurs, Evolution should do its "locking" on the file
> >> returned from realpath() too.
> >
> >You are probably right.
>
> I've thought about it some more, and I'm upgrading my "maybe" to a
> strong "this is the proper way to do it". Symlinks should not be
> locked. They should be followed with realpath() and the real mailbox
> should be locked, like other MUA's do. This will truly make it
> compatible and play nicely on the Unix system.
It also fixes this.
--
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
[EMAIL PROTECTED] - www.ximian.com
Index: ChangeLog
=======
RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
retrieving revision 1.1311.2.11
diff -u -r1.1311.2.11 ChangeLog
--- ChangeLog 2002/01/04 22:17:52 1.1311.2.11
+++ ChangeLog 2002/01/09 21:44:15
@@ -1,3 +1,11 @@
+2002-01-09 Jeffrey Stedfast <[EMAIL PROTECTED]>
+
+ * providers/local/camel-local-folder.c
+ (camel_local_folder_construct): If the mbox file is a symlink,
+ follow the symlink and get the One True Path so that we can
+ rewrite the mbox later without worrying about clobbering the
+ symlink.
+
2001-12-12 Jeffrey Stedfast <[EMAIL PROTECTED]>
* camel-folder-summary.c (content_info_load): Don't try setting a
Index: providers/local/camel-local-folder.c
===
RCS file: /cvs/gnome/evolution/camel/providers/local/camel-local-folder.c,v
retrieving revision 1.22
diff -u -r1.22 camel-local-folder.c
--- providers/local/camel-local-folder.c2001/10/28 05:10:55 1.22
+++ providers/local/camel-local-folder.c2002/01/09 21:44:15
@@ -24,6 +24,7 @@
#endif
#include
+#include
#include
#include
#include
@@ -191,7 +192,14 @@
lf->folder_path = g_strdup_printf("%s/%s", root_dir_path, full_name);
lf->summary_path = g_strdup_printf("%s/%s.ev-summary", root_dir_path,
full_name);
lf->index_path = g_strdup_printf("%s/%s.ibex", root_dir_path,
full_name);
-
+
+ /* follow any symlinks to the mailbox */
+ if (lstat (lf->folder_path, &st) != -1 && S_ISLNK (st.st_mode) &&
+ realpath (lf->folder_path, folder_path) != NULL) {
+ g_free (lf->folder_path);
+ lf->folder_path = g_strdup (folder_path);
+ }
+
lf->changes = camel_folder_change_info_new();
/* if we have no index file, force it */
Re: serious bug. Evolution and Microsoft mentality.
Oops, copy/paste-o when migrating the patch to the 1-0 code base. Jeff On Wed, 2002-01-09 at 19:28, Jonathan Walther wrote: > Thank you for the patch. To make it work, you need to define the > variable "folder_path". I would recommend this: > > char folder_path[4096]; > > And then before using it, do this: > > memset(folder_path, 0, sizeof folder_path); > > Cheers. > > Jonathan > > On Wed, Jan 09, 2002 at 04:57:34PM -0500, Jeffrey Stedfast wrote: > >> You solved the problem. That is the correct solution. After running > >> realpath(), use dirname() and make the tmpfile in the same directory > >> as the mailbox. > >> > >[snip] > > > >The attached patch will fix this issue. > > > >> > >> >> Again, provided one uses Maildir mailboxes, things will be fine. But > >> >> the thought occurs, Evolution should do its "locking" on the file > >> >> returned from realpath() too. > >> > > >> >You are probably right. > >> > >> I've thought about it some more, and I'm upgrading my "maybe" to a > >> strong "this is the proper way to do it". Symlinks should not be > >> locked. They should be followed with realpath() and the real mailbox > >> should be locked, like other MUA's do. This will truly make it > >> compatible and play nicely on the Unix system. > > > >It also fixes this. -- Jeffrey Stedfast Evolution Hacker - Ximian, Inc. [EMAIL PROTECTED] - www.ximian.com
Re: serious bug. Evolution and Microsoft mentality.
Actually the attached patch is the "correct one". There is no need to
memset and you should use PATH_MAX rather than 4096.
Jeff
On Wed, 2002-01-09 at 23:36, Jonathan Walther wrote:
> On Wed, Jan 09, 2002 at 07:38:43PM -0500, Jeffrey Stedfast wrote:
> >Oops, copy/paste-o when migrating the patch to the 1-0 code base.
>
> Here is the correct patch for the 1.0.x branch. Hopefully the Debian
> maintainer will apply it? I am creating an Evolution 1.0-5.1 package
> on my system with the patch applied. I haven't seen so many signal 11's
> in aeons.
>
> Index: camel/providers/local/camel-local-folder.c
> ===
> RCS file: /cvs/gnome/evolution/camel/providers/local/camel-local-folder.c,v
> retrieving revision 1.22
> diff -u -r1.22 camel-local-folder.c
> --- evolution/camel/providers/local/camel-local-folder.c2001/10/28
> 05:10:55 1.22
> +++ evolution/camel/providers/local/camel-local-folder.c2002/01/09
> 21:44:15
> @@ -23,8 +23,9 @@
> #include
> #endif
>
> #include
> +#include
> #include
> #include
> #include
> #include
> @@ -173,8 +174,9 @@
> CamelFolder *folder;
> const char *root_dir_path, *name;
> struct stat st;
> int forceindex;
> + char folder_path[4096];
>
> folder = (CamelFolder *)lf;
>
> name = strrchr(full_name, '/');
> @@ -190,8 +192,16 @@
> lf->base_path = g_strdup(root_dir_path);
> lf->folder_path = g_strdup_printf("%s/%s", root_dir_path, full_name);
> lf->summary_path = g_strdup_printf("%s/%s.ev-summary", root_dir_path,
> full_name);
> lf->index_path = g_strdup_printf("%s/%s.ibex", root_dir_path,
> full_name);
> +
> + /* follow any symlinks to the mailbox */
> + memset(folder_path, 0, sizeof folder_path);
> + if (lstat (lf->folder_path, &st) != -1 && S_ISLNK (st.st_mode) &&
> + realpath (lf->folder_path, folder_path) != NULL) {
> + g_free (lf->folder_path);
> + lf->folder_path = g_strdup (folder_path);
> + }
>
> lf->changes = camel_folder_change_info_new();
>
> /* if we have no index file, force it */
--
Jeffrey Stedfast
Evolution Hacker - Ximian, Inc.
[EMAIL PROTECTED] - www.ximian.com
Index: ChangeLog
=======
RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
retrieving revision 1.1311.2.11
diff -u -r1.1311.2.11 ChangeLog
--- ChangeLog 2002/01/04 22:17:52 1.1311.2.11
+++ ChangeLog 2002/01/10 17:30:10
@@ -1,3 +1,11 @@
+2002-01-09 Jeffrey Stedfast <[EMAIL PROTECTED]>
+
+ * providers/local/camel-local-folder.c
+ (camel_local_folder_construct): If the mbox file is a symlink,
+ follow the symlink and get the One True Path so that we can
+ rewrite the mbox later without worrying about clobbering the
+ symlink.
+
2001-12-12 Jeffrey Stedfast <[EMAIL PROTECTED]>
* camel-folder-summary.c (content_info_load): Don't try setting a
Index: providers/local/camel-local-folder.c
===
RCS file: /cvs/gnome/evolution/camel/providers/local/camel-local-folder.c,v
retrieving revision 1.22
diff -u -r1.22 camel-local-folder.c
--- providers/local/camel-local-folder.c2001/10/28 05:10:55 1.22
+++ providers/local/camel-local-folder.c2002/01/10 17:30:10
@@ -24,6 +24,7 @@
#endif
#include
+#include
#include
#include
#include
@@ -172,6 +173,7 @@
CamelFolderInfo *fi;
CamelFolder *folder;
const char *root_dir_path, *name;
+ char folder_path[PATH_MAX];
struct stat st;
int forceindex;
@@ -191,7 +193,14 @@
lf->folder_path = g_strdup_printf("%s/%s", root_dir_path, full_name);
lf->summary_path = g_strdup_printf("%s/%s.ev-summary", root_dir_path,
full_name);
lf->index_path = g_strdup_printf("%s/%s.ibex", root_dir_path,
full_name);
-
+
+ /* follow any symlinks to the mailbox */
+ if (lstat (lf->folder_path, &st) != -1 && S_ISLNK (st.st_mode) &&
+ realpath (lf->folder_path, folder_path) != NULL) {
+ g_free (lf->folder_path);
+ lf->folder_path = g_strdup (folder_path);
+ }
+
lf->changes = camel_folder_change_info_new();
/* if we have no index file, force it */
Re: serious bug. Evolution and Microsoft mentality.
While I agree with your statements, it's strictly not possible in this case - realpath() requires that the second argument be a string buffer of size PATH_MAX (read the manpage). SYNOPSIS #include char *realpath(const char *file_name, char *resolved_name); DESCRIPTION The realpath() function derives, from the pathname pointed to by file_name, an absolute pathname that names the same file, whose resolution does not involve ".", "..", or sym- bolic links. The generated pathname is stored, up to a max- imum of PATH_MAX bytes, in the buffer pointed to by resolved_name. Jeff On Thu, 2002-01-10 at 13:27, Jeroen Dekkers wrote: > On Thu, Jan 10, 2002 at 12:38:30PM -0500, Jeffrey Stedfast wrote: > > Actually the attached patch is the "correct one". There is no need to > > memset and you should use PATH_MAX rather than 4096. > > Both is incorrect. PATH_MAX isn't required by POSIX and some systems > don't have it (the Hurd for example). The Hurd simply doesn't have a > limit (the GNU Coding Standards says arbitrary limits should be > avoided, they are bad (think about the 640k limit, 1024 cylinder > limit, etc)). If we have to define some limit it would be so large > that it is not suitable for static allocation. You should use dynamic > allocation and not limit you in any way. > > Jeroen Dekkers > -- > Jabber supporter - http://www.jabber.org Jabber ID: [EMAIL PROTECTED] > Debian GNU supporter - http://www.debian.org http://www.gnu.org > IRC: [EMAIL PROTECTED] -- Jeffrey Stedfast Evolution Hacker - Ximian, Inc. [EMAIL PROTECTED] - www.ximian.com
