Re: POP3d locking

[Alain Magloire]

> 
> 
> There was an error in proposed mbox_is_updated() (c'est l'esprit
> d'escalier 8-) It should read:
> 
> static int
> mbox_is_updated (mailbox_t mailbox)
> {
>   off_t size = 0;
>   mbox_data_t mud = mailbox->data;
>   if (mud->size == 0 || stream_size (mailbox->stream, &size) != 0)
>     return 0;

Why for mud->size == 0?
If the original mailbox was 0 and we check if the mailbox is uptodate
and it is still 0, why should it return FALSE? AFAICS  the mailbox
is updated and should return TRUE.

No?

> --- 264,273 ----
>         buf = pop3_readline (ifile);
>         cmd = pop3_cmd (buf);
>         arg = pop3_args (buf);
> !       
> !       if (state == TRANSACTION && !mailbox_is_updated (mbox)) 
> !     pop3_abquit(ERR_MBOX_SYNC);
> !        

I've check in a fix for this.  I still do not like it though
and pushing for an entire locking (see other email).

> 
> But for it to work we have to scan the mailbox immediately after
> entering transaction state. Otherwise after receiving the very first
> command in transaction state mbox_is_updated() returns 0 and pop3d
> exits. How do you think, can we afford doing mailbox scan immediately
> after successfull authentication? It will be scanned anyway, sooner
> or later, so it doesn't seem to make a difference...

In the fix, the first pass accept the size of the mailbox and saves
it(assuming we just enter the TRANSACTION state).  The next rounds
will do the comparaison, so ho about this:

static off_t mailbox_size;
...
if (state == TRANSACTION && (!mailbox_is_updated (mbox) || !mailbox_size)
  {
    off_t newsize = 0;
    mailbox_get_size (mbox, &newsize);
    /* Did we shrink?  */
    if (!mailbox_size)
      mailbox_size = newsize;
    else if (newsize < mailbox_size)
      pop3d_abquit (ERR_MBOX_SYNC);
  }

A bit different then what I commited last night.

> One more thing: there seems to be an inconsistency in mbox_expunge().
> Namely:


WOW!! I could have stare at this for two weeks and never see it.
Good catch!

> 
> Index: mailbox/mbx_mbox.c
> ===================================================================
> RCS file: /cvs/mailutils/mailbox/mbx_mbox.c,v
> retrieving revision 1.43
...
> !           status = mbox_get_message (mailbox, i, &msg);
> --- 703,713 ----
...
> !           status = mbox_get_message (mailbox, i+1, &msg);

> The reason for this is:
> 
> Immediately after `if (mum->message == 0)' block the following is
> executed:
> 
>     status = mbox_append_message0 (tmpmailbox, mum->message,
>                                        &total, 1, (i == save_imapbase));
> 
> i.e. it assumes that mbox_get_message() will place the pointer to
> message into mum->message member. But mbox_get_message() relies on
> 1-based message numbers, whereas `mum' pointer is obtained using
> 0-based indexing:

Ok.

-- 
au revoir, alain
----
Aussi haut que l'on soit assis, on n'est toujours assis que sur son cul !!!