Aloha Timo.
I found a few odd things while playing with dovecot. At least one of them bugs me, but it might be a result of my own patches - I haven't testet it with mbox or maildir, yet.
The details :)
- OK dovecot ready. A = (<return> Connection closed by foreign host.
- OK dovecot ready. A001 LOGIN xxxxxx xxxxxx A001 OK Logged in. A002 COPY 1 INBOX Connection closed by foreign host. (segfault)
[...] A003 SEARCH SUBJECT "test"
- SEARCH 1 A003 OK Search completed. A004 SEARCH SUBJECT {6}
- OK "test"
- SEARCH A004 OK Search completed.
I think these two searches should give identical results.
- (my current nemesis)
A002 CREATE bla A003 SELECT INBOX A004 COPY 1 bla A005 SELECT bla A006 COPY 1 bla
- NO Internal error [2003-10-16 16:07:21] A006 OK Copy completed.
If I copy from the current folder to the current folder I get a scrambled mailbox and an error. This might be selfmade, though.
I'm currently trying to find this bug, but the various layers of [_oi]streams and callback and function pointer structs do not help :) I think the stream routines hide the mstream(istream(realstream(iostream))) and the ostream(_ostream(iostream)) behind the same file descriptor and the data gets written to the wrong position.
BTW: what exactly is "max_pos"?
Greetings and thanks! Andy
On Thu, 2003-10-16 at 17:14, Andreas Jaekel wrote:
- OK dovecot ready. A = (<return> Connection closed by foreign host.
Fixed. It always disconnected if there was an error in the IMAP command syntax.
- OK dovecot ready. A001 LOGIN xxxxxx xxxxxx A001 OK Logged in. A002 COPY 1 INBOX Connection closed by foreign host. (segfault)
Fixed.
[...] A003 SEARCH SUBJECT "test"
- SEARCH 1 A003 OK Search completed. A004 SEARCH SUBJECT {6}
- OK "test"
- SEARCH A004 OK Search completed.
I think these two searches should give identical results.
No. First you are searching for test without quotes. Then you are searching for "test" with quotes. Correct way would be {4}\r\ntest, or SEARCH SUBJECT "\"test\"".
- (my current nemesis)
A002 CREATE bla A003 SELECT INBOX A004 COPY 1 bla A005 SELECT bla A006 COPY 1 bla
- NO Internal error [2003-10-16 16:07:21] A006 OK Copy completed.
If I copy from the current folder to the current folder I get a scrambled mailbox and an error. This might be selfmade, though.
Yeah. It's in my TODO list too. Problem is that it uses the same file descriptor for both reading and writing and the seek offsets get mixed up. This could be fixed by always seeking to correct position, but that's a bit annoying since either you have to do it always (unnecessary nearly always) or add some extra kludgy checks. You could also be just reading with mmap() or pread() but I'm not sure if it's a good idea to rely on them either.
I'm currently trying to find this bug, but the various layers of [_oi]streams and callback and function pointer structs do not help :) I think the stream routines hide the mstream(istream(realstream(iostream))) and the ostream(_ostream(iostream)) behind the same file descriptor and the data gets written to the wrong position.
Yes, the stream stuff is kind of ugly and difficult to follow. Maybe still has some bugs too.. Cleaning them up would be a good idea, but I'm not sure if I know how. I don't think the class system is too bad though, you can't do it much better with C.
BTW: what exactly is "max_pos"?
You mean high_pos? It's .. um.. some kludge.. that I added to fix some problem.. :) I think it went:
Normally "pos" contains the amount of data that has been read from the underlying stream. But when you temporarily set a read limit, it has to shrink "pos" as well or things break. So "high_pos" is there just to remember what the "pos" was before any read limits.
It should work so that istream.c does every stream's buffering. The istream-*.c then just provide a way to fill the buffer and seek around in the stream.
Aloha Timo.
Thanks for the other fixes.
If I copy from the current folder to the current folder I get a scrambled mailbox and an error. This might be selfmade, though.
Yeah. It's in my TODO list too. Problem is that it uses the same file descriptor for both reading and writing and the seek offsets get mixed up. This could be fixed by always seeking to correct position, but that's a bit annoying since either you have to do it always (unnecessary nearly always) or add some extra kludgy checks. You could also be just reading with mmap() or pread() but I'm not sure if it's a good idea to rely on them either.
Well, always seeking on input and output works, (I tried) except it breaks APPEND, which reads from a socket, which fails seeking, which I have no clean way of knowing since it's all hidden behind the stream wrappers.
I think a clean way may be to change the stream wrappers so they remember which streams are dup()s of other streams. Then, on each read or write operation they could consider the 'original stream' a kind of master record in order to store the current cursor position there on every operation. And check -before- every operation wether the master record's pos is equal to their own wanted position. If not, a seek is needed.
Somewhat like:
if (stream->master_copy->current_pos != stream->current_pos) stream->seek(stream->current_pos);
After that the only problem would be to find the master record when creating new streams, but that should be possible.
Another possible solution is to change output to always use pwrite(), avoiding the need for seeks completly.
Yes, the stream stuff is kind of ugly and difficult to follow. Maybe still has some bugs too.. Cleaning them up would be a good idea, but I'm not sure if I know how. I don't think the class system is too bad though, you can't do it much better with C.
Well, it's quite confusing sometimes, but it's also quite handy. It took me a while to figure out that mstreams make a difference between mail header read() and mail body read(), though :)
BTW: what exactly is "max_pos"?
You mean high_pos? It's .. um.. some kludge.. that I added to fix some problem.. :) I think it went:
Normally "pos" contains the amount of data that has been read from the underlying stream. But when you temporarily set a read limit, it has to shrink "pos" as well or things break. So "high_pos" is there just to remember what the "pos" was before any read limits.
It should work so that istream.c does every stream's buffering. The istream-*.c then just provide a way to fill the buffer and seek around in the stream.
Hmm... when I made my copy of the mbox backend I had to modify my version of i_stream_create_mbox() to add a call to i_stream_seek() after i_stream_set_read_limit(), or else mail_storage_save() would break with unexpected EOF. The reason was that high_pos was != 0, but I don't remember the details anymore. :)
On Tue, 2003-10-21 at 11:50, Andreas Jaekel wrote:
that's a bit annoying since either you have to do it always (unnecessary nearly always) or add some extra kludgy checks. You could also be just reading with mmap() or pread() but I'm not sure if it's a good idea to rely on them either.
Well, always seeking on input and output works, (I tried) except it breaks APPEND, which reads from a socket, which fails seeking, which I have no clean way of knowing since it's all hidden behind the stream wrappers.
There's fstream->file which tells if it's a regular file :)
I think a clean way may be to change the stream wrappers so they remember which streams are dup()s of other streams.
They're not dup()ed, they just use the same file descriptors. But .. hmm. that would be possible, but that's still quite a lot of code for cases which are rarely needed..
Another possible solution is to change output to always use pwrite(), avoiding the need for seeks completly.
Changing ostream to use pwrite() would require replacing the one writev() call too with multiple pwrite()s, not nice .. :) How about: diff -u -r1.9 istream-file.c --- istream-file.c 26 Aug 2003 21:18:16 -0000 1.9 +++ istream-file.c 21 Oct 2003 10:11:25 -0000 @@ -2,6 +2,8 @@ /* @UNSAFE: whole file */ +#define _XOPEN_SOURCE 500 /* for pread() / Linux */ + #include "lib.h" #include "alarm-hup.h" #include "istream-internal.h" @@ -169,7 +171,15 @@ return -1; } - ret = read(stream->fd, stream->w_buffer + stream->pos, size); + if (fstream->file) { + ret = pread(stream->fd, + stream->w_buffer + stream->pos, size, + stream->istream.start_offset + + stream->istream.v_offset); + } else { + ret = read(stream->fd, + stream->w_buffer + stream->pos, size); + } if (ret == 0) { /* EOF */ stream->istream.stream_errno = 0;
Aloha.
here's fstream->file which tells if it's a regular file :)
Oooh, nice! But does that work outside ostream.c? :)
They're not dup()ed, they just use the same file descriptors. But .. hmm. that would be possible, but that's still quite a lot of code for cases which are rarely needed..
But it'd be clean and the way to avoid the largest number of syscalls if pread/pwrite are not an option.
Another possible solution is to change output to always use pwrite(), avoiding the need for seeks completly.
Changing ostream to use pwrite() would require replacing the one writev() call too with multiple pwrite()s, not nice .. :)
How about: [...]
Thank you for the patch. It worked, except for two little things: 1. I don't use Linux, so the #define for pseek broke the compile and 2. I had to remove the lseek() call in _read(), which was the whole point in using pread() :) In mbox folders this wouldn't have mattered since the loop is written in a way that assumes mail 4 starts exactly where mail 3 ends, which is not true for my case. This worked for me: --- ../dovecot/src/lib/istream-file.c Tue Aug 26 23:18:16 2003 +++ ./src/lib/istream-file.c Tue Oct 21 16:18:55 2003 @@ -169,7 +169,15 @@ return -1; } - ret = read(stream->fd, stream->w_buffer + stream->pos, size); + if (fstream->file) { + ret = pread(stream->fd, + stream->w_buffer + stream->pos, size, + stream->istream.start_offset + + stream->istream.v_offset); + } else { + ret = read(stream->fd, + stream->w_buffer + stream->pos, size); + } if (ret == 0) { /* EOF */ stream->istream.stream_errno = 0; @@ -228,7 +236,7 @@ stream->istream.stream_errno = EOVERFLOW; ret = -1; } else { - ret = lseek(stream->fd, (off_t)real_offset, SEEK_SET); + ret = (off_t)real_offset; if (ret < 0) stream->istream.stream_errno = errno; else if (ret != (off_t)real_offset) {
On Tuesday, Oct 21, 2003, at 17:25 Europe/Helsinki, Andreas Jaekel wrote:
here's fstream->file which tells if it's a regular file :)
Oooh, nice! But does that work outside ostream.c? :)
I haven't seen a reason to make it visible outside ..
They're not dup()ed, they just use the same file descriptors. But .. hmm. that would be possible, but that's still quite a lot of code for cases which are rarely needed..
But it'd be clean and the way to avoid the largest number of syscalls if pread/pwrite are not an option.
Yes, but is it really a problem? I just started thinking that pread/pwrite are probably supported by pretty much all new operating systems. Do we need to add lots of kludgeing for some old systems?
hank you for the patch. It worked, except for two little things:
- I don't use Linux, so the #define for pseek broke the compile and
Oh .. I was thinking too that it might cause problems somewhere..
- I had to remove the lseek() call in _read(), which was the whole point in using pread() :)
Well .. I left it there because I thought it might be useful in some cases .. well, maybe not :)
- I had to remove the lseek() call in _read(), which was the whole point in using pread() :)
Well .. I left it there because I thought it might be useful in some cases .. well, maybe not :)
I get some odd behaviour now which will have me testing and searching for a while, I guess. :)
BTW: is it possible that the current CVS checkout does not include the pwrite() and can not COPY mails do empty folders?
participants (2)
-
Andreas Jaekel
-
Timo Sirainen