httpd-mbox-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jus...@erenkrantz.com>
Subject Re: svn commit: r160602 [1/3] - in httpd/mod_mbox/trunk: ./ config/ m4/ module-2.0/
Date Sat, 16 Apr 2005 07:23:12 GMT
On Fri, Apr 08, 2005 at 08:30:54PM -0000, pquerna@apache.org wrote:
> Modified: httpd/mod_mbox/trunk/module-2.0/mbox_cache.c
> URL: http://svn.apache.org/viewcvs/httpd/mod_mbox/trunk/module-2.0/mbox_cache.c?view=diff&r1=160601&r2=160602
> ==============================================================================
> --- httpd/mod_mbox/trunk/module-2.0/mbox_cache.c (original)
> +++ httpd/mod_mbox/trunk/module-2.0/mbox_cache.c Fri Apr  8 13:30:50 2005
> @@ -30,6 +30,8 @@
>      return APR_SUCCESS;
>  }
>  
> +#if 0
> +/* handy debugging function */
>  static void dump_dbm(apr_dbm_t* d)
>  {
>      apr_status_t status;
> @@ -41,6 +43,7 @@
>          status = apr_dbm_nextkey(d, &key);
>      }
>  }
> +#endif 

Please don't add if 0's to the code.  Just leave the function in there.

>      /* We store the entire structure in a single entry. */
> -    vlen = sizeof(msgc->location) + sizeof(msgc->date) + sizeof(msgc->cte)
+ \
> +    vlen = (sizeof(msgc->body_start) * 3) + sizeof(msgc->date) + sizeof(msgc->cte)
+ \
>             (sizeof(tlen) * 5) + \
>             sstrlen(msgc->from) + \
>             sstrlen(msgc->subject) + \

Should we be better off just adding in body_start, msg_start, and body_end
sizeof?  FWIW, this really seems like a lot of black voodoo now that is
just rife for errors.  What's tlen?

The store_cstring has a scoping error: it refers to 'tlen' when that's not in
scope.  (A comment explaining what that goofy macro does would be good.)
I'm guessing it writes out the length of the string and the string; but
that's not real obvious from its name...

(Note: fetch_cstring has similar scoping errors.)

Now that I've read it through, I think it'd be far clearer if the tlen were
grouped with the strings as in:

sstrlen(msgc->from) + sizeof(tlen) + \

I believe this makes it much clearer that the tlen size is logically grouped
with the string fields.

> @@ -762,6 +705,12 @@
>      if (fi.size != (apr_size_t)fi.size)
>          return APR_EGENERAL;
>  
> +    if (fi.size == 0) {
> +        OPEN_DBM(r, msgDB, APR_DBM_RWCREATE, MSGID_DBM_SUFFIX, temp, status);
> +        apr_dbm_close(msgDB);
> +        return OK;
> +    } 
> +
>      status = apr_mmap_create(&b.mm, f, 0, (apr_size_t)fi.size, APR_MMAP_READ, r->pool);
>  
>      if (status != APR_SUCCESS)
> @@ -782,14 +731,31 @@
>  
>      mbox_fillbuf(&b);
>  
> +    msgID = NULL;
>      apr_pool_create(&tpool, r->pool);
>      /* When we reach the end of the file, b is NULL.  */
>      while (b.b)
>      {
> +#ifdef APR_HAS_MMAP
> +        msgc.body_end = b.b - b.sb;
> +#else
> +        msgc.body_end = b.totalread - b.len + b.b - b.rb;
> +#endif        
>          if (b.b[0] == 'F' && b.b[1] == 'r' && 
>              b.b[2] == 'o' && b.b[3] == 'm' &&
>              b.b[4] == ' ')
>          {
> +            if (msgID) {
> +                store_msgc(tpool, msgDB, msgID, &msgc, indexer, list, domain);
> +                msgID = NULL;
> +            }
> +            apr_pool_clear(tpool);

This is a very odd bit of code and use of pools.  If I read it correctly, you
are delaying updating the index until you read the beginning of the next
message.  If that is the case, then a comment explaining that would go a
long way to making it clear.

> +            
> +#ifdef APR_HAS_MMAP
> +            msgc.msg_start = b.b - b.sb;
> +#else
> +            msgc.msg_start = b.totalread - b.len + b.b - b.rb;
> +#endif

This is screaming to be a macro of some sort.

> Modified: httpd/mod_mbox/trunk/module-2.0/mbox_parse.h
> URL: http://svn.apache.org/viewcvs/httpd/mod_mbox/trunk/module-2.0/mbox_parse.h?view=diff&r1=160601&r2=160602
> ==============================================================================
> --- httpd/mod_mbox/trunk/module-2.0/mbox_parse.h (original)
> +++ httpd/mod_mbox/trunk/module-2.0/mbox_parse.h Fri Apr  8 13:30:50 2005
> @@ -35,6 +35,7 @@
>  #include "apr_strings.h"
>  #include "apr_mmap.h"
>  
> +#include "mbox_search.h"
>  #include <stdio.h>

Why are we adding the include dependency here?  What does searching have to
do with parsing?  Shouldn't we just have updated the files that need the
search APIs directly?

> @@ -150,17 +153,19 @@
>  /* 
>   * Generates the DBM file.
>   */
> -apr_status_t generate_index(request_rec *r, apr_file_t * f);
> +apr_status_t mbox_generate_index(request_rec *r, apr_file_t * f,
> +                            mbox_indexer_t *indexer, const char* list,
> +                            const char* domain);

Is the include from above present because of this prototype change?  What does
the Lucene index have to do with the generation of the mod_mbox index?  Why
can't the indexing for the search be done separately?  Why tie it into the
generation of master index?  At a quick glance, Lucene should be able to
separately generate its own indexes after mod_mbox has created its index.

(I'd think it should be kept separate: especially considering how specific it
is to Lucene.)

> Added: httpd/mod_mbox/trunk/module-2.0/mbox_search.c
> URL: http://svn.apache.org/viewcvs/httpd/mod_mbox/trunk/module-2.0/mbox_search.c?view=auto&rev=160602
> ==============================================================================
> --- httpd/mod_mbox/trunk/module-2.0/mbox_search.c (added)
> +++ httpd/mod_mbox/trunk/module-2.0/mbox_search.c Fri Apr  8 13:30:50 2005
> @@ -0,0 +1,225 @@
> +/* Copyright 2001-2005 The Apache Software Foundation or its licensors, as
> + * applicable.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
...snip...

Most of the other mod_mbox files have a small comment block after the license
block that says what this file does.  I have no idea what this file does.  It
seems that it spawns a java process.  But, how that relates to searching I'm
not sure at all.  (I have a guess that it creates the index.)

> Modified: httpd/mod_mbox/trunk/module-2.0/mod-mbox-util.c
> URL: http://svn.apache.org/viewcvs/httpd/mod_mbox/trunk/module-2.0/mod-mbox-util.c?view=diff&r1=160601&r2=160602
> ==============================================================================
> --- httpd/mod_mbox/trunk/module-2.0/mod-mbox-util.c (original)
> +++ httpd/mod_mbox/trunk/module-2.0/mod-mbox-util.c Fri Apr  8 13:30:50 2005

Not sure when this file was created, but it should likely be renamed to
mod_mbox_util.c.  But, having one file that uses a - vs a _ is badness.

> @@ -94,14 +103,14 @@
>  
>      temp = r->filename;
>      r->filename = absfile;
> -    rv = generate_index(r, f);
> +    rv = mbox_generate_index(r, f, indexer, list, domain);
>      r->filename = temp;
>  
>      if (rv != APR_SUCCESS) {
>          apr_file_printf(errfile, "Error: Index Generation for '%s' failed: %s" NL,
>                          absfile,
>                          apr_strerror(rv, errbuf, sizeof(errbuf)));
> -        return EXIT_FAILURE;
> +        return 0;
>      }

Um.  Are you sure changing the exit code is right?  (It's likely not.)

Also, whatever editor you are using for your commits is adding tons of
whitepsace on blank lines and adds loads of trailing whitespace.  Can you
please fix up your editor to not do that any more?

Thanks!  -- justin

Mime
View raw message