Re: [Evolution-hackers] Let the porting begin
On Thu, 2007-10-25 at 22:25 -0400, Jeffrey Stedfast wrote:
> On Fri, 2007-10-26 at 03:25 +0200, Philip Van Hoof wrote:
> > On Wed, 2007-10-24 at 11:58 -0400, Jeffrey Stedfast wrote:
> > > I took a look at the IDLE implementation last night and felt it went
> > > about it the wrong way.
> >
> > Yes, you are right. I think the right fix is to create a new API called
> > camel_tcp_stream_wait that works like this:
> >
> >
> > int bytes = camel_tcp_stream_wait (tcp_stream, stop_fd,
> > store->idle_sleep, line_buffer, 1023);
> [snip]
>
> I was thinking more along the lines of making the idle_loop use
> PRPoll()/poll() itself - it already has to figure out what type of fd to
> create for the cancel_idle_fd anyway.
I have it more or less working, with one more problem remaining (and I
already tried fixing it, check the "line_buffer" stuff in this patch)
being that the read returns me a block of bytes, not lines.
Yet I need to feed the rest of the current IDLE handling code exact
lines (untagged lines that start with '*').
So my plan is to use strrchr(buffer, '\n') and with the result calculate
where the last full line was. Then add that to what we stored last time.
And store the remainder in a buffer for the next loop.
Then with the two pieces together, strtok_r on \n and get the full lines
ouf of that.
I already started this, but I'm going to sleep now :)
Attached ... (it's again on Tinymail's trunk, not on Camel upstream)
> > Afaics It's not true that you don't need to change any API:
> >
> > The camel_stream_read() will block until all n bytes are read.
>
> no it doesn't. it returns as soon as any number of bytes are read or an
> error occurs.
>
> > This is
> > something you don't know during IDLE (how many bytes you can expect to
> > receive), and on top of that you want to continue immediately after
> > select returns, not keep retrying until you have n bytes (to simulate a
> > blocking read).
>
> sure, but that's why you poll yourself. once the socket has data, /then/
> you call camel_stream_read() on it.
>
> > You also want a dedicated stop fildescriptor, not the
> > standard cancel_fd as then any cancel will interfere with this (while
> > you want strict control over this to let other threads start and stop
> > IDLE).
>
> right, but if you add a CamelStream API, then you have to add 1 for unix
> fds and 1 for PRFileDesc
>
> plus it keeps the code simpler (keeps all read() logic in the
> camel_stream_read() implementations rather than duplicating it).
>
>
> This actually brings me to another thought I've had for a few years now
> but haven't bothered pushing for it... but it might be best if all of
> the sockets used PRFileDesc rather than unix fd for raw sockets and
> PRFileDesc for SSL sockets. If this is done, I think it'd be possible to
> get rid of CamelTcpStreamRaw and move the implementation into
> CamelTcpStream and make CamelTcpStreamSSL a simple subclass of
> CamelTcpStream, replacing the CamelTcpStream's PRFileDesc with an
> SSL-wrapped fd as appropriate and calling parent_class->read/write/etc
> instead of implementing them all itself /again/.
>
>
> Jeff
>
>
--
Philip Van Hoof, software developer
home: me at pvanhoof dot be
gnome: pvanhoof at gnome dot org
http://www.pvanhoof.be/blog
Index: libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-store.c
===
--- libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-store.c (revision 2884)
+++ libtinymail-camel/camel-lite/camel/providers/imap/camel-imap-store.c (working copy)
@@ -197,46 +197,31 @@
static void
let_idle_die (CamelImapStore *store, gboolean connect_buz)
{
-
- idle_debug ("let_idle_die starts\n");
-
- g_static_rec_mutex_lock (store->idle_prefix_lock);
g_static_rec_mutex_lock (store->idle_lock);
- store->idle_kill = TRUE;
- store->idle_cont = FALSE;
-
- if (store->idle_prefix)
+ idle_debug ("let_idle_die starts\n");
+ if (store->in_idle && store->idle_thread)
{
- gchar *resp = NULL;
- int nwritten = 0;
- CamelException ex = CAMEL_EXCEPTION_INITIALISER;
+ IdleResponse *idle_resp;
+ store->idle_cont = FALSE;
+ if (store->current_idle)
+ camel_operation_cancel (store->current_idle);
+ idle_resp = g_thread_join (store->idle_thread);
+ if (store->current_idle)
+ camel_operation_uncancel (store->current_idle);
- idle_debug ("Sending DONE in let_idle_die\n");
- CAMEL_SERVICE_REC_LOCK (store, connect_lock);
- nwritten = camel_stream_printf (store->ostream, "DONE\r\n");
store->in_idle = FALSE;
- if (nwritten != -1) {
- resp = NULL;
- while ((camel_imap_command_response_idle (store, &resp, &ex)) == CAMEL_IMAP_RESPONSE_UNTAGGED) {
-idle_debug ("(.., ..) <- %s | in let_idle_die\n", resp);
-g_free (resp); resp=NULL;
- }
- if (resp) {
-idle_debug ("(.., ..) <- %s\n", resp);
-g_free (resp);
- }
- }
- CAMEL_SERVICE_REC_UNLOCK (store, connect_lock);
- g_free (store->idle_prefix)
Re: [Evolution-hackers] Let the porting begin
On Wed, 2007-10-24 at 11:58 -0400, Jeffrey Stedfast wrote:
> I took a look at the IDLE implementation last night and felt it went
> about it the wrong way.
Yes, you are right. I think the right fix is to create a new API called
camel_tcp_stream_wait that works like this:
int bytes = camel_tcp_stream_wait (tcp_stream, stop_fd,
store->idle_sleep, line_buffer, 1023);
I need the amount of read-read bytes, to know where the buffer stops, so
I return it. I need a stop_fd to get the select() to early-stop. I need
a timeout for in case of connection problems. I need the buffer to put
the newly read data in and I need a size to tell it how large my buffer
max is.
.. idle_thread () ..
{
start_idle (store);
line_buffer = malloc (1024)
memset (line_buffer, 0, sizeof (*line_buffer));
...
while (store->idle_cont) {
char *lasts = NULL;
int bytes = camel_tcp_stream_wait (tcp_stream, stop_fd,
store->idle_sleep, line_buffer, 1023);
if (bytes > 0) {
char *resp;
line_buffer[bytes+1] = '\0';
for (resp = strtok_r (line_buffer, "\n", &lasts); resp; resp =
strtok_r (NULL, "\n", &lasts)) {
if (resp && strlen (resp) > 1 && resp[0] == '*') {
if (!idle_resp)
idle_resp = idle_response_new (folder);
consume_idle_line (store, folder, resp,
idle_resp);
lines++;
}
}
printf ("IDLE: read %d bytes %s\n", bytes, line_buffer);
}
if (!store->idle_cont || (idle_resp && idle_resp->exists_happened)) {
idle_done (store, folder, idle_resp);
stopped = TRUE;
if (!store->idle_cont)
retval = idle_resp;
}
if (store->idle_cont) {
if (idle_resp) {
process_idle_response (idle_resp);
idle_response_free (idle_resp);
idle_resp = NULL;
printf ("Processed %d lines\n", lines);
lines = 0;
}
if (stopped)
idle_start (store);
}
memset (line_buffer, 0, sizeof (*line_buffer));
}
}
That "wait" is implemented like this:
static ssize_t
stream_wait (CamelTcpStream *stream, int stop_fd, int timeout, char *buffer,
size_t max_len)
{
CamelTcpStreamRaw *raw = CAMEL_TCP_STREAM_RAW (stream);
int fd = raw->sockfd;
ssize_t nread;
int fdmax;
fd_set rdset;
u_long yes = 1;
struct timeval tv;
int res;
//ioctlsocket (fd, FIONBIO, &yes);
fdmax = MAX (fd, stop_fd) + 1;
FD_ZERO (&rdset);
FD_SET (fd, &rdset);
FD_SET (stop_fd, &rdset);
tv.tv_sec = timeout;
tv.tv_usec = 0;
nread = -1;
res = select (fdmax, &rdset, 0, 0, &tv);
if (res > 0 && !FD_ISSET (stop_fd, &rdset))
nread = recv (fd, buffer, max_len, 0);
else
nread = -1;
return nread;
}
Afaics It's not true that you don't need to change any API:
The camel_stream_read() will block until all n bytes are read. This is
something you don't know during IDLE (how many bytes you can expect to
receive), and on top of that you want to continue immediately after
select returns, not keep retrying until you have n bytes (to simulate a
blocking read). You also want a dedicated stop fildescriptor, not the
standard cancel_fd as then any cancel will interfere with this (while
you want strict control over this to let other threads start and stop
IDLE).
Well ... let me put it this way: I tried it from a few angles, I didn't
succeed with the normal camel_stream_read :-(
I have reworked the IDLE implementation. You can apply this patch to
Tinymail's upstream (trunk). You most likely can't apply it to upstream
Camel. (http://svn.tinymail.org/svn/tinymail/trunk)
There's still one problem: sometimes camel_imap_folder_stop_idle is not
returning late enough (the socket is still in IDLE state when it
returns).
Other than that, it usually works.
I'm trying to fix this last problem right now, and after that there will
most likely be a few days of testing before I put this in Tinymail's
repository.
> You should simply poll() on the socket descriptors (and a pipe fd used
> for telling the IDLE thread's I/O loop to finish and send DONE).
Yep
> Jeff
>
> On Mon, 2007-10-08 at 00:41 +0200, Philip Van Hoof wrote:
> > On Sun, 2007-10-07 at 14:15 +0200, Philip Van Hoof wrote:
> > > Hi there,
> >
> > > Using this changeset you can follow the changes to camel-lite:
> > >
> > > http://tinymail.org/trac/tinymail/changeset/2823
> > >
> >
> > This changeset are a bunch of compilation warnings for Matthew's Base64
> > patch to Camel: http://t
Re: [Evolution-hackers] Let the porting begin
On Fri, 2007-10-26 at 03:25 +0200, Philip Van Hoof wrote: > On Wed, 2007-10-24 at 11:58 -0400, Jeffrey Stedfast wrote: > > I took a look at the IDLE implementation last night and felt it went > > about it the wrong way. > > Yes, you are right. I think the right fix is to create a new API called > camel_tcp_stream_wait that works like this: > > > int bytes = camel_tcp_stream_wait (tcp_stream, stop_fd, >store->idle_sleep, line_buffer, 1023); [snip] I was thinking more along the lines of making the idle_loop use PRPoll()/poll() itself - it already has to figure out what type of fd to create for the cancel_idle_fd anyway. > > Afaics It's not true that you don't need to change any API: > > The camel_stream_read() will block until all n bytes are read. no it doesn't. it returns as soon as any number of bytes are read or an error occurs. > This is > something you don't know during IDLE (how many bytes you can expect to > receive), and on top of that you want to continue immediately after > select returns, not keep retrying until you have n bytes (to simulate a > blocking read). sure, but that's why you poll yourself. once the socket has data, /then/ you call camel_stream_read() on it. > You also want a dedicated stop fildescriptor, not the > standard cancel_fd as then any cancel will interfere with this (while > you want strict control over this to let other threads start and stop > IDLE). right, but if you add a CamelStream API, then you have to add 1 for unix fds and 1 for PRFileDesc plus it keeps the code simpler (keeps all read() logic in the camel_stream_read() implementations rather than duplicating it). This actually brings me to another thought I've had for a few years now but haven't bothered pushing for it... but it might be best if all of the sockets used PRFileDesc rather than unix fd for raw sockets and PRFileDesc for SSL sockets. If this is done, I think it'd be possible to get rid of CamelTcpStreamRaw and move the implementation into CamelTcpStream and make CamelTcpStreamSSL a simple subclass of CamelTcpStream, replacing the CamelTcpStream's PRFileDesc with an SSL-wrapped fd as appropriate and calling parent_class->read/write/etc instead of implementing them all itself /again/. Jeff ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Let the porting begin
On Wed, 2007-10-24 at 11:58 -0400, Jeffrey Stedfast wrote: > I took a look at the IDLE implementation last night and felt it went > about it the wrong way. Note that the IDLE implementation got changed (reimplemented) this morning. Although the same problems that you mention here still apply. I also added some comments. The poll() idea is indeed something that at some point I should refactor the loop to. > Firstly, the added camel_stream_[read,write]_nb() and > camel_stream_read_idle() functionality is totally unnecessary and just > makes the camel stream API gross (not to mention duplicating a lot of > code as there's no real difference from the normal read/write > implementations other than the timeout). > > You should simply poll() on the socket descriptors (and a pipe fd used > for telling the IDLE thread's I/O loop to finish and send DONE). > > Jeff > > On Mon, 2007-10-08 at 00:41 +0200, Philip Van Hoof wrote: > > On Sun, 2007-10-07 at 14:15 +0200, Philip Van Hoof wrote: > > > Hi there, > > > > > Using this changeset you can follow the changes to camel-lite: > > > > > > http://tinymail.org/trac/tinymail/changeset/2823 > > > > > > > This changeset are a bunch of compilation warnings for Matthew's Base64 > > patch to Camel: http://tinymail.org/trac/tinymail/changeset/2827 > > > > > > ps. Adding Matthew in CC. > > > -- Philip Van Hoof, software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://www.pvanhoof.be/blog ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Let the porting begin
I took a look at the IDLE implementation last night and felt it went about it the wrong way. Firstly, the added camel_stream_[read,write]_nb() and camel_stream_read_idle() functionality is totally unnecessary and just makes the camel stream API gross (not to mention duplicating a lot of code as there's no real difference from the normal read/write implementations other than the timeout). You should simply poll() on the socket descriptors (and a pipe fd used for telling the IDLE thread's I/O loop to finish and send DONE). Jeff On Mon, 2007-10-08 at 00:41 +0200, Philip Van Hoof wrote: > On Sun, 2007-10-07 at 14:15 +0200, Philip Van Hoof wrote: > > Hi there, > > > Using this changeset you can follow the changes to camel-lite: > > > > http://tinymail.org/trac/tinymail/changeset/2823 > > > > This changeset are a bunch of compilation warnings for Matthew's Base64 > patch to Camel: http://tinymail.org/trac/tinymail/changeset/2827 > > > ps. Adding Matthew in CC. > ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Let the porting begin
On Sun, 2007-10-07 at 14:15 +0200, Philip Van Hoof wrote: > Hi there, > Using this changeset you can follow the changes to camel-lite: > > http://tinymail.org/trac/tinymail/changeset/2823 > This changeset are a bunch of compilation warnings for Matthew's Base64 patch to Camel: http://tinymail.org/trac/tinymail/changeset/2827 ps. Adding Matthew in CC. -- Philip Van Hoof, software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://www.pvanhoof.be/blog ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Let the porting begin
On Sun, 2007-10-07 at 14:15 +0200, Philip Van Hoof wrote: > Hi there, > Using this changeset you can follow the changes to camel-lite: > > http://tinymail.org/trac/tinymail/changeset/2823 More such commits to Tinymail's camel-lite happened today. Nearly all of those are merging work in action. After today, using KDiff3 or any other tool that can recursively compare two directories should be painless and straight forward on ./libtinymail-camel/camel-lite/camel vs. ./evolution-data-server/camel -- Philip Van Hoof, software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://www.pvanhoof.be/blog ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
[Evolution-hackers] Let the porting begin
Hi there, I got notified that some Evolution hackers are interested in bringing the features of camel-lite to camel. This is of course excellent news! To ease this process, I pushed all the recent changes that happened to camel, to camel-lite. These changes include the copyright's address changes, the changes Matthew Barnes did on the BASE64 decoding, most of the compiler-warning changes (although a lot of them had already been put in camel-lite, of course), Kjartan Maraas's 0 vs. NULL changes, Milan Crha's changes (set-label, compiler warnings, etc). Using this changeset you can follow the changes to camel-lite: http://tinymail.org/trac/tinymail/changeset/2823 Note about compiler warnings: camel-lite contains fixes for -pedantic warnings. When making differences, you might find compiler-warning fixes that you wouldn't expect with -Wall. Camel-lite right now has two or three things that are problematic and not yet fixed for -pedantic: - Enumerations that exceed the size of an int, which are not allowed in ANSI C (or C99, I don't remember exactly). - Casting function pointers to (void*) which also isn't allowed (but I have no idea how else I can store them in a GHashTable, which is what Camel is doing at some locations) Some of the changes to camel-lite might not be welcomed to camel. Among them I expect the changes to camel-object not to be interested (some of these changes cut one or two bytes from the CamelObject's size and put some more and also less locking-on-signal-emissions in place). Others are more clear like features that are just not interesting. And yet others are just not done in a clean way. On future plans for camel-lite: I'm interested in implementing QRSYNC next to the CONDSTORE support. Although only one IMAP server already supports this (Isode's Mbox). -- Philip Van Hoof, software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://www.pvanhoof.be/blog ___ Evolution-hackers mailing list Evolution-hackers@gnome.org http://mail.gnome.org/mailman/listinfo/evolution-hackers
