|
On Tue, Apr 24, 2007 at 08:40:02AM -0700, Roberto Nibali wrote:
> > I think this simplifies things somewhat.
>
> I thought about the exact same thing when I went through this :). We
> kernel people somehow love the goto.
Indeed.
> > Cc: Roberto Nibali <ratz@xxxxxxxxxxxx>
> > Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>
> > Index: heartbeat/ldirectord/ldirectord.in
> > ===================================================================
> > --- heartbeat.orig/ldirectord/ldirectord.in 2007-04-24 18:17:00.000000000
> > +0900
> > +++ heartbeat/ldirectord/ldirectord.in 2007-04-24 18:17:05.000000000
> > +0900
> > @@ -2490,17 +2490,16 @@ sub check_sql
> > require DBI;
> > my ($v, $r, $dbd, $dbname) = @_;
> > my $port=(defined $$v{checkport}?$$v{checkport}:$$r{port});
> > - my ($dbh, $sth, $query, $rows, $result); # Local variables
> > + my $dbh, $sth, $query, $rows, $result = 1;
> > +
> > $query = $$r{request};
> > $query =~ s#^/##;
> > unless ($$v{login} && $query) {
> > - service_set($v, $r, "down");
> > &ld_log("Error: Must specify a login and request string " .
> > "for MySQL and PostgreSQL checks. " .
> > "Not adding $$r{server}.\n");
> > - return 1;
> > + goto err_down;
> > }
> > - $result=2; # Set result flag. Only ok if ends up at zero.
> > &ld_debug(2, "Checking $$v{server} server=$$r{server} port=$port\n");
> > $dbh = DBI->connect("dbi:$dbd:$dbname=$$v{database};" .
> > "host=$$r{server};port=$port", $$v{login},
> > @@ -2508,17 +2507,13 @@ sub check_sql
> > unless ($dbh) {
> > &ld_debug(4, "Failed to bind to $$r{server} with " .
> > $dbh->errstr);
> > - service_set($v, $r, "down");
> > - return 1;
> > + goto err_down;
> > }
> > - $result--;
> > $sth = $dbh->prepare($query);
> > unless ($sth) {
> > service_set($v, $r, "down");
> > &ld_log("Error preparing statement:" . $dbh->errstr() . "\n");
> > - $dbh->disconnect();
> > - service_set($v, $r, "down");
> > - return 1;
> > + goto err_disconnect;
> > }
> > $rows = $sth->execute;
> > ld_debug(4, "Database search returned $rows rows");
> > @@ -2526,20 +2521,20 @@ sub check_sql
> > # * Disallows query of an empty table.
> > # * Only works with select and update statements, however most
> > # people will want to use select statements for health checks.
> > - if ($rows gt 0) {
> > - $result--;
> > - } else {
> > + if ($rows == 0) {
>
> The only thing I'm a bit worried about is the fact that $rows is
> initialised with the value of 1. What if the previous execute()
> statement failed? Is $rows set to 0 (good) or is it left untouched by Perl?
I think you might be misreading the syntax, or perhaps I am.
But I don't see where $row is initialised. In any case $rows will
be set to something by "$rows = $sth->execute;". Apparently in the case
of error it will be undef. Which will definately break the if($rows..
thing. I'll fix that up.
Looking a bit futher into the DBI man page, I see that $sth->execute
will actually return 0 on a SELECT because it just starts the select at
this point and returns a "true value". To get the number of rows
one needs to use a fetch method.
I'll try and fix the logic up to reflect this. I think the original
code kind of worked in this regards. Though perhaps only with select
statements.
> > &ld_log("Prior executing statement returned $rows rows: " .
> > "not adding $$r{server}.\n");
> > - service_set($v, $r, "down");
> > - $sth->finish();
> > - $dbh->disconnect();
> > - return 1;
> > + goto err_finish;
> > }
> > - service_set($v, $r, "up");
> > +
> > + $result = 0;
> > +err_finish:
> > $sth->finish();
> > +err_disconnect:
> > $dbh->disconnect();
> > - return 0;
> > +err_down:
> > + service_set($v, $r, $result == 0 ? "up" : "down");
> > + return $result;
> > }
> >
> >
> >
>
> Otherwise: nice cleanup!
>
> >From skimming over the logics for a short while, this change looks ok.
>
> Acked-by: Roberto Nibali <ratz@xxxxxxxxxxxx>
>
> --
> I'm traveling the world: http://our-worldtrip.blogspot.com
--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/
|
