[isf-wifidog] Re: [wirelesstoronto-discuss] code contribution to wifidog auth server

[Benoit Grégoire]

> >At first glance, I don't get it.  We already have a custom DB abstraction
> >layer.  We are open to replacing it with an off the shelf one or improving
> >it, but I don't see what replacing it with different but still custom one
> >would buy anyone?  But I guess I'll understand when I see the code.
>
1> * printing obscure error messages in the middle of the html page
2> * throwing errors that aren't necessarily caught
3> * gets a new (albiet hopefully cached) db connection with every sql
> statement
4> * printing debug messages in the middle of the html page
5> * profiling info not collected so much as debugged
6> * not extensible by inheritance or wrapping
7> * too much cutting and pasting of code, the big methods are mostly
> identical but for a couple of lines.
8> * no objective wrapping of the result set, so all calls must return the
> entire result set in an array


1, 2 were unavoidable before we had a proper logging and error display 
abstraction, there have nothing to do with the db abstraction class design 
itself.

3 was done this way because that is how postgress PHP driver works (returns 
the same connexion if the parameters are the same, so there was simply no 
reason to manage keeping a pointer to the connexion.

4- If you found a way to implement it so that it doesn't print in the middle 
of the page but also works everywhere and has no UI dependencies, I'm all for 
it!

5: Sorry, I don't understand your statement.

6: I think it appears so because the really abstract AbstractDb class was 
pulled from wifidog because at one point there was a transition from MySql 
and we needed both to be distinct, but that's just a historical artifact.

7:  Agreed, but why not just fix it.

8:  True, but taht was by design.  We were not trying to re-create one of the 
heavyweight abstraction layers, just to simplify the calling conventions and 
add easy to use debuging and profiling information.  Web applications seldom 
operate on resultsets large enough that there is significant overhead in 
pulling it all at once (doesn't mean the queries cannot be super-costly, but 
it's very rare to pull megabytes of data in an array).  Supporting it meant 
the db calls were no longer identical.

> >It's seems unlikely that the only changes needed in the db abstraction
> > layer are in common.php, unless you kept the exact same API (but then why
> > change the abstraction layer?).
>
> as to why, see above.  as to how, I made AbstractDb into a wrapper
> around my stuff.  If the method isn't found it passes the call to my stuff.

Well, now we have an extra indirection layer around code that is still custom, 
why not just fix what is already there?  The people who wrote the current 
code spent a lot of time on it.  I'm sure they won't mind having their code 
repalced by a standard and externally maintained db abstraction layer.  Or 
even have their code partly rewritten to be better, or completely replaced by 
something that does stuff that neither the current abstraction nor a standard 
one could never do.  But replacing all that code by a still custom 
abstraction layer with an extra indirection to aparently do the exact same 
thing with the same API as the class current class is a little hard to 
accept.

I'd much rather we merge your code in the current one.

> >>if you folks on the other hand find that these changes are not useful,
> >>then i'll have to find a way to use wifidog-auth's UI from the install
> >>script without making changes to wifidog-auth's internals.
> >
> >See above
> >
> >>however, i've made some security improvements to the database stuff i
> >>hope you'll accept -
> >>
> >>* database connections using the unix socket (more secure)
> >
> >?!?!? That was already supported and documented in the INSTALL file.
>
> I didn't find anything when I started this.  i just grepped the entire
> tree for unix (case independent) and found nothing relevant.  it's great
> if it's in there, but it would be nice if that piece of help was easier
> to find.

Sorry, aparently the person who did the install script deleted it from the 
documentation and didn't move it into the install script.

> the pg_connect call made in AbstractDbPostgres.php only supports the
> unix socket if you supply the full slash path to the postgres unix
> socket as the hostname.  AbstractDbPostgres will not omit the "host=xxx"
> keyword from the connection string and so you cannot use the server's
> unix socket by omitting the "host=xxx" keyword which is what I'd prefer
> to do, not really knowing where the unix socket is.

Well, I can assure you it used to work, you just had to leave 
CONF_DATABASE_HOST blank.  In any case, that is not a DB abstraction layer 
issue.

> >Well, I may be missing something, but I find what you propose extremely
> >dangerous.  When you upgrade a live server, the schema update must happen
> > as soon as the code is updated, no matter what.
> >
> >Otherwise, you have new code running that may or may not work with the old
> >schema (and may even accidentally corrupt data in a way that will break
> > the assumptions of the upgrade script).  If you instead checkout in a
> > sandbox (but with your real database) to have your schema update before
> > update the code on the live server, you have the opposite problem:  old
> > code running that may or may not be compatible with the new schema. 
> > That's why schema validate is always called (not just from the install
> > script), and why the wifidog user must have table creation privileges.
>
> shouldn't updates to the code (via cvs update, or copy) be done while
> the server is offline?  otherwise, a user could get in there while the
> update is still progressing.  who knows what would happen.  suppose they
> got in before schema_validate was updated but after all the other code
> was updated?  or visa versa?
>
> from later postings to this thread i see that Max has already started on
> separating the roles of maintainer from user, which is i think exactly
> what i'm calling for.

All I can say is that ISF strongly disagrees.  

First we have hotspots that are open (and have people using them) 24h a day.  
The only semi-reasonnable maintenance window would be between 4 à 5 am, which 
is a lot to ask from volunteers.  

Second, creating an offline mode would be very complicated.  The amount of 
special treatement while offline and after coming back online is probably 
quite a bit larger than what you thing.  As an example, you'd have to do 
special treatement when the server comes back online to not send bogus emails 
to hotspot tech support that their hotspot went down, but you can't just 
reset the last successfull heartbeat, else you will disrupt the notification 
mecanism for hotspots that actually are down.  So you'd have to keep a log in 
the database of your maintenance windows, so you can do all you special 
casing.  

Third, we've fixed bugs or even added features during daytime very frequently 
in the past.  We would now loose this ability.  Taking the whole network down 
to fix a minor bug just seems unreasonnable, even more now that many groups 
use it.  Unexpected problems may sometimes apear because one group made well 
tested changes but introduced a bug in a code path they never use.

The only valid reason I see to do this change is that indeed, code updates 
can't be done entirely atomically and it's possible that a request could 
corrupt data in a specific record in the few seconds that pass.  But that's 
not very likely, and if it does happen the consequences are far less than 
interrupting all active user sessions and taking 75 nodes instantly offline 
for everyone.  Not to mention the cost of having to do it at night on a 
weekly basis and incurring the overhead of coding, testing, maintaining and 
documenting an offline mode.  This is one of the reason I'd personally prefer 
to drop future support for MySql (I have doubt's that people are ever going 
to step up to finish it and support it and that all developpers will 
systematically test their changes with both databases).  Not maintaining 
complete database independance would allow us to have the schema completly 
enforce it's semantics trough triggers.  That way just about any request that 
would corrupt data would simply fail.

Having end user application using databases may not have revolutionned how 
databases work.  But they tought us a few tricks along the way on hab they 
can be administered.  One of which is that transparent schema updates work 
just fine.

If you are not willing to take the risk and consider the risk of a bug or 
human error in the offline mode to actually be lower, you are welcome to 
implement it in the mainline, as long as it's an optional process.


-- 
Benoit Grégoire, http://benoitg.coeus.ca/
-------------- section suivante --------------
Une pièce jointe non texte a été nettoyée...
Nom: non disponible
Type: application/pgp-signature
Taille: 189 octets
Desc: non disponible
Url: http://listes.ilesansfil.org/pipermail/wifidog/attachments/20051116/cb26f104/attachment.pgp