[Box Backup-dev] [PATCH] Win32 merge [03] Additional commands
for bbackupctl
Martin Ebourne
boxbackup-dev@fluffy.co.uk
Mon, 07 Aug 2006 13:27:22 +0100
Chris Wilson <chris@qwirx.com> wrote:
> The attached patch adds some new commands to bbackupctl. They are
> "wait-for-end" and "sync-and-wait". The patch includes documentation
> for them. They are useful in bbackupd unit tests (still to come).
There's beginning to be quite a confusing array of commands in =20
bbackupctl now. I wonder if some of them could be rationalised to make =20
it simpler?
The command execution code is something of an ifelse mess at the =20
moment. The last loop has essentially no shared code between the =20
commands. It would be a lot better to implement each of these commands =20
in a separate function and call that instead. You didn't start the =20
code down this route, so I'm not blaming you!
> =09// Is the command the "wait for sync to start" command?
> =09bool areWaitingForSync =3D false;
> -=09if(::strcmp(argv[0], "wait-for-sync") =3D=3D 0)
> +=09bool areWaitingForSyncEnd =3D false;
> +
> +=09if(::strcmp(argv[0], "wait-for-sync") =3D=3D 0 ||
> +=09 ::strcmp(argv[0], "wait-for-end") =3D=3D 0)
> =09{
> -=09=09// Check that it's not in non-automatic mode, because then it'll =
=20
> never start
> +=09=09// Check that it's not in non-automatic mode, +=09=09// because the=
n =20
> it'll never start
> =09=09if(!autoBackup)
> =09=09{
> -=09=09=09printf("ERROR: Daemon is not in automatic mode -- sync will =20
> never start!\n");
> +=09=09=09printf("ERROR: Daemon is not in automatic mode -- "
> +=09=09=09=09"sync will never start!\n");
> =09=09=09return 1;
> =09=09}
>
> -=09=09// Yes... set the flag so we know what we're waiting for a sync to =
start
> -=09=09areWaitingForSync =3D true;
> +=09=09// Yes... set the flag so we know that +=09=09// we're waiting for =
a =20
> sync to start/end
> +=09=09if(::strcmp(argv[0], "wait-for-sync") =3D=3D 0)
> +=09=09{
> +=09=09=09areWaitingForSync =3D true;
> +=09=09}
> +=09=09else if (::strcmp(argv[0], "wait-for-end") =3D=3D 0)
> +=09=09{
> +=09=09=09areWaitingForSyncEnd =3D true;
> +=09=09}
> =09}
At the very least the original boolean state flag (areWaitingForSync) =20
should have been replaced by an enum for command and the if further =20
down replaced by a switch though. This comparing twice for the same =20
string is bad and should be avoided. Not for efficiency here but for =20
consistency. If those strings ever manage to differ then that will be =20
hard to debug.
> +=09=09=09else if(line =3D=3D "finish-sync" && syncIsRunning)
> +=09=09=09{
> +=09=09=09=09if (!quiet) printf("Sync finished.\n");
> +=09=09=09=09// Send a quit command to finish nicely
> +=09=09=09=09connection.Write("quit\n", 5);
> +
> +=09=09=09=09// And we're done
> +=09=09=09=09break;
> +=09=09=09}
> +=09=09=09else if(line =3D=3D "finish-sync")
> +=09=09=09{
> +=09=09=09=09if (!quiet) printf("Previous sync finished.\n");
> +=09=09=09}
This testing the same condition would be much better as a nested if.
Something along the lines of the following would be better if separate =20
functions were not used (I'm not attempting to indent this properly in =20
the email program):
enum Command
{
Default,
WaitForSync,
WaitForSyncEnd,
SyncAndWait
};
Command command =3D Default;
std::string commandName(argv[0]);
if (commandName =3D=3D "wait-for-sync")
{
command =3D WaitForSync;
}
else if (commandName =3D=3D "wait-for-end")
{
command =3D WaitForEnd;
}
else if (commandName =3D=3D "sync-and-wait")
{
command =3D SyncAndWait;
}
switch (command)
{
case WaitForSync:
case WaitForSyncEnd:
{
// Check that it's in automatic mode, because
// otherwise it'll never start
if(!autoBackup)
{
printf("ERROR: Daemon is not in automatic mode -- "
"sync will never start!\n");
return 1;
}
}
break;
case SyncAndWait:
{
// send a sync command
std::string cmd("force-sync\n");
connection.Write(cmd.c_str(), cmd.size());
connection.WriteAllBuffered();
if (currentState !=3D 0)
{
printf("Waiting for current sync/error state "
"to finish...\n");
}
}
break;
default:
{
// Normal case, just send the command given plus a quit command=
.
std::string cmd(argv[0]);
cmd +=3D "\nquit\n";
connection.Write(cmd.c_str(), cmd.size());
}
}
// Read the response
std::string line;
bool syncIsRunning =3D false;
bool finished =3D false;
while(!finished && !getLine.IsEOF() && getLine.GetLine(line))
{
switch (command)
{
case WaitForSync:
{
// Need to wait for the state change...
if(line =3D=3D "start-sync")
{
// Send a quit command to finish nicely
connection.Write("quit\n", 5);
// And we're done
finished =3D true;
}
}
break;
case WaitForSync:
case SyncAndWait:
{
if(line =3D=3D "start-sync")
{
if (!quiet) printf("Sync started...\n");
syncIsRunning =3D true;
}
else if(line =3D=3D "finish-sync")
{
if (syncIsRunning)
{
if (!quiet) printf("Sync finished.\n");
// Send a quit command to finish nicely
connection.Write("quit\n", 5);
// And we're done
finished =3D true;
}
else
{
if (!quiet) printf("Previous sync finished.\n");
}
// daemon must still be busy
}
}
break;
default:
{
// Is this an OK or error line?
if(line =3D=3D "ok")
{
if(!quiet)
{
printf("Succeeded.\n");
}
finished =3D true;
}
else if(line =3D=3D "error")
{
printf("ERROR. (Check command spelling)\n");
returnCode =3D 1;
finished =3D true;
}
}
}
}
Cheers,
Martin.