MESSAGE quota resource implemention

Greg Banks gnb at fastmail.fm
Thu Sep 1 04:15:12 EDT 2011


Julien Coloos wrote:
> Hi,
>
> As discussed on IRC last week, I worked on implementing MESSAGE quota 
> resource in cyrus (see RFC 2087; STORAGE resource already being handled).
> I created a branch based on Greg's 'annotate' one on github, since his 
> work on annotation storage management made mine a lot easier.
>
> Details on the changes I made:
>
> cyrus-imapd:
>   - added MESSAGE quota resource management
>     -> updated cunit test
>     -> added 'quotawarnmsg' option which behaviour is similar to 
> 'quotawarnkb'; warning message shown when selecting folder in IMAP 
> tells which resource limit was triggered
>     -> added 'autocreatequotamsg' option, to set MESSAGE limit 
> (unlimited by default) when user auto-creates its mailbox
>   - xfer transfers all non-unlimited resources
>   - added helper function to compute annotation storage usage for a 
> given mailbox
>   - changed the way quota entries are read/written
>     -> resources presence in fetched entry is remembered
>     -> when writing entry, only present resources are written
>     -> when setting resources limits, entries which were not present 
> upon fetching are now marked as present and their current usage is 
> computed
>   - quota utility lists and computes (-f) all resources associated to 
> mailbox
> The branch 'quotamessage/gnb/annotate' is available here: 
> git://github.com/worldline-messaging/cyrus-imapd.git. It is based on 
> Greg's 'annotate' branch on github.
>

Reviewed:

commit 2d4fb0 "Added quota MESSAGE resource (RFC 2087) management."

imap/quota.h

Why #include <stdint.h>?  It's not like we're using any of the typedefs 
there.  There's a good argument to be made that we *should* do so, but 
until then the #include doesn't seem useful.

I'm unconvinced about the need for quota.sets[].  After a lot of reading 
it seems to me that (quota.sets[x]) and (quota.limits[x] != 
QUOTA_UNLIMITED) are identical or should be.  Also, are you setting 
sets[] anywhere except when loading from the db?

I like quota_update_useds(), it does seem necessary to be able to update 
several usages together.

Otherwise, looks good.

imap/quota_db.c

In quota_write(), your sanity check should goto the buf_free() at the 
end of the function rather than return early.  Otherwise, looks good.

imap/append.c

Looks good.

imap/append.h

Looks good

imap/cyr_virusscan.c

Looks good

imap/imap_err.et

Looks good

imap/imapd.c

In the 1st hunk in cmd_append(), at this point in the code I believe 
totalsize = 0, so you could easily pass 0 for the new last argument as well.

Otherwise looks good.

BTW, reading your code I realised a number of places I failed to account 
for X-ANNOTATION-STORAGE quota properly :(  I think I'll have to change 
a number of those new pairs of quota_t arguments to a quota_t 
quotadiff[QUOTA_NUMRESOURCES].

imap/index.c

Looks good

imapd/lmtpd.c

Looks good

imap/lmtpengine.c

Looks good

imap/lmtpengine.h

Looks good


imap/mailbox.c

I'm not sure it's a good idea to set mailbox->i.exists = 0 in 
mailbox_delete().  Perhaps it would be better to check for 
(mailbox->i.options & OPT_MAILBOX_DELETED) when sampling 
mailbox->i.exists in mailbox_commit_quota().

Otherwise, looks good.

imap/mailbox.h

Looks good

imap/mbdump.c

Looks good

imap/nntpd.c

Looks good

lib/imapoptions

Looks good.

commit 0e613c "Removed unused code."

Looks good.

commit c8aeef "Compute each quota resource upon setting it for the first 
time."

commit b30c10 "List and compute all set quota resources."

Oh, here's why you include stdint.h:  you're using PRIdMAX and not 
QUOTA_REPORT_FMT, which is used only in imapd.c.

I'm thinking that there's now three places in the code which take a 
mailbox* and fill out an array of quota diffs, interpreting the contents 
of the struct mailbox.  That should really be centralised.


-- 
Greg.



More information about the Cyrus-devel mailing list