[Box Backup-dev] win32 review

[Chris Wilson]

Hi Ben,

> lib/win32/WinNamedPipeStream.cpp
> 	mWriteClosed = TRUE;
> 	-- mWriteClosed is of type "bool", so setting it to TRUE rather than 
> true is not strictly correct. Similarly for all the other changes.

Changed as requested, but I thought that TRUE and true were the same?

> lib/server/Daemon.cpp
> 	-- not keen on all the #ifdefs for logging on Win32. Not sure there's 
> a neater way if you really want to do this, apart from defining another 
> function to do it, and calling that instead. Soon to be done away with with 
> the iostream for logging stuff, I think.

Can I leave it in then? Or can we syslog on all platforms? (would seem to 
make sense to me).

> lib/common/FdGetLine.cpp
> 	-- eeek! But whatever, shouldn't catch anyone unawares, but still, 
> eek!

Sorry, I didn't like it either, but on Windows with the console in unicode 
mode, read() returns zero (i.e. doesn't work) if the user enters any 
unicode character.

> bin/bbackupd/BackupClientContext.cpp
> 	-- if Protocol doesn't log it, it wasn't sent.

OK, removed the extra logging.

> bin/bbackupquery/BackupQueries.cpp
> +		char* buffer = ConvertConsoleToUtf8(args[0].c_str());
> +               if(!buffer) return;
> +               std::string storeDirEncoded(buffer);
> +               delete [] buffer;
> 	-- ugly. Why doesn't ConvertConsoleToUtf8() return std::string?

OK, changed ConvertConsoleToUtf8 (and vice versa) to take a std::string 
ref as a parameter, and return a bool.

> 	-- why have we moved from storing a line in a std::string to using 
> printf() (and without the :: prefix?)

I was originally using wprintf for the filename, until I found that it 
didn't work properly. I can probably revert most of that now, if you like. 
It seemed like a lot of trouble to add everything to a string, if all you 
were going to do was print it out at the end.

Almost none of the printfs in BackupQueries have ::. Do you want me to add 
them?

Also, almost none of the error messages are sent to stderr, perhaps that 
should be fixed?

> 	-- please #define const_iterator iterator on Win32 only, rather than 
> removing this type check on all platforms

Done. MSVC doesn't like to call erase() with a const argument.

> bin/bbackupquery/bbackupquery.cpp
> 	-- I would have done unicode by default, with a flag to switch it off

I don't think it's a good idea, since the default console font can't 
handle unicode, but can handle the user's native codepage. The 
non-unicode option will work better for most people.

> distribution/boxbackup/DISTRIBUTION-MANIFEST.txt
> 	-- add your new directories, and check you have everything in the 
> archive created by
> 		infrastructure/makedistribution.pl boxbackup

I thought about this, but I came up against a problem. I include these 
files in the client parcel, but their location changes from SVN checkouts 
to release tarballs. That means I can't rely on their location in the 
parcels.txt, which makes things very complicated.

Why do we have this strange distribution system where the location of the 
contrib directory changes? Can it be fixed? Otherwise, can I put contrib 
stuff somewhere other than under distribution/boxbackup?

> Usual mumblings about style. eg "if(command == "quit" || command == "")" 
> CHANGED to "if (command == "quit" || command == "")", it's not even new 
> code!

Sorry, I'd forgotten about those, I've changed them back.

Cheers, Chris.
-- 
_ ___ __     _
  / __/ / ,__(_)_  | Chris Wilson <0000 at qwirx.com> - Cambs UK |
/ (_/ ,\/ _/ /_ \ | Security/C/C++/Java/Perl/SQL/HTML Developer |
\ _/_/_/_//_/___/ | We are GNU-free your mind-and your software |