apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject RE: cvs commit: apr-util/dbm/sdbm sdbm.c
Date Sat, 20 Jan 2001 22:23:24 GMT
Ok, this is a sparkling demonstration of the weakness behind
any validation scheme.  Let's refactor it a few times.

> wrowe       01/01/20 13:40:23
> 
>   diff -u -r1.3 -r1.4
>   --- sdbm.c	2000/12/12 08:54:30	1.3
>   +++ sdbm.c	2001/01/20 21:40:23	1.4
>   @@ -177,7 +177,8 @@
>            /*
>             * need the dirfile size to establish max bit number.
>             */
>   -        if ((status = apr_getfileinfo(&finfo, db->dirf)) != APR_SUCCESS)
>   +        if (((status = apr_getfileinfo(&finfo, APR_FINFO_SIZE, db->dirf)) 
>   +              != APR_SUCCESS) || !(finfo.valid & APR_FINFO_SIZE))
>                goto error;

Simple, and elegant.  Of course this would be simpler;

>   +        if (((status = apr_getfileinfo(&finfo, APR_FINFO_SIZE, db->dirf)) 
>   +              != APR_SUCCESS))

Now the underlying issue gets more complex when we ask

   if (((status = apr_getfileinfo(&finfo, APR_FINFO_NORM, db->dirf)) 
          != APR_SUCCESS))

We expect most everything, but some platforms will be missing something
(AIX has no group, correct?)  Rather than keep -lying- to the application,
apr ought to be honest and leave those bits unset.  But we do -not- want
to fail in this case.  What if we add a bit of cruft?

   if (((status = apr_getfileinfo(&finfo, APR_FINFO_NORM, db->dirf)) 
          != APR_SUCCESS) && status != APR_INCOMPLETE)

This is ugly as well, now we have two tests where 99% of the time we
just don't care.  If we would 'like' to do something about the user
of the file later, we still need to do this...

   if (finfo.valid & APR_FINFO_USER)
       something(finfo.user);

So what's the pain v.s. gain of reporting APR_INCOMPLETE?

If we want to be notified, I'd suggest the high bit of the wanted arg
is passed as APR_FINFO_FAIL.  When we -must- have a user, we can do
this instead;

   if (((status = apr_getfileinfo(&finfo, APR_FINFO_USER | APR_FINFO_FAIL,
                                  db->dirf)) != APR_SUCCESS))

Pretty gosh darned straightforward.  But so is the current;

   if (((status = apr_getfileinfo(&finfo, APR_FINFO_USER, db->dirf))
          != APR_SUCCESS) || !(finfo.valid & APR_FINFO_USER))

Thoughts, folks?




Mime
View raw message