httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: [PATCH] Warning clean-ups....
Date Thu, 21 Sep 2000 15:12:35 GMT

> > Index: src/modules/standard/mod_include.c
> > ===================================================================
> > RCS file: /cvs/apache/apache-2.0/src/modules/standard/mod_include.c,v
> > retrieving revision 1.56
> > diff -u -d -b -r1.56 mod_include.c
> > --- mod_include.c	2000/09/14 18:42:56	1.56
> > +++ mod_include.c	2000/09/15 15:54:23
> > @@ -338,7 +338,7 @@
> >  static char *get_tag(apr_pool_t *p, ap_bucket *in, char *tag, int tagbuf_len, int
dodecode, apr_off_t *offset)
> >  {
> >      ap_bucket *dptr = in;
> > -    const char *c;
> > +    const char *c = NULL;
> 
> This function looks uncool... It *deserves* a warning :)  (No, I'm
> just too lazy to look at it in sufficient detail right now.)
> 
> Following the thread of variable c to see if it is really used before
> being set points out some other issues to research, which probably
> means it is worthwhile to get a warning.  Setting c to NULL wouldn't
> resolve any issues but would make them less noticeable.
> 
> Some of the issues:
> 
>   the while condition on the first loop looks bad; it should perhaps
>   be "while (1) {" or "while (dptr != sentinel-value) {".  "while
>   (dptr)" acts the same as "while (1)" but is confusing.
> 
>   what happens if there is a zero-len bucket somewhere?  we don't
>   check for that after dptr->read(); maybe we are guaranteed not to
>   get such a bucket in this function?  with Ryan's changes to the
>   split routines, we can get zero-length buckets in more situations
> 
> (Ryan: how much of mod_include were you planning to rewrite, anyway?
> More than the main parsing loop which creates a brigade with the tag?)

The whole damn thing.  :-)  It needs to be a real parser.  That would
solve all of these problems.  I am hoping to get back to work on it
soon-ish, but I have some other stuff on my plate right now.  This module
has always been one of the bigger hacks in Apache IMO, and we need to just
re-write the thing to solve them.  Trying to hack in more solutions will
just make the problem worse.

> 
> >      const char *str;
> >      apr_ssize_t length; 
> >      char *t = tag, *tag_val, term;
> > @@ -2267,7 +2267,7 @@
> >      int conditional_status;
> >      ap_bucket *dptr = AP_BRIGADE_FIRST(bb);
> >      ap_bucket *tagbuck, *dptr2;
> > -    ap_bucket *endsec;
> > +    ap_bucket *endsec = NULL;
> 
> I think that a change Ryan agreed to this weekend would take care of this
> warning (look for "wet noodle" and related thread in the archives :) ).
> I think that Ryan is going to rewrite this code soon, but if that
> other change doesn't hurt and it gets rid of the warning I'll put it in.

Feel free to put that change in.  I am unlikely to get to the re-write
immediately, and even once I start, it will probably take at least a week
to do it right.

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------


Mime
View raw message