[Box Backup-dev] win32 review

[Ben Summers]

On 12 Feb 2006, at 16:06, Chris Wilson wrote:

> 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?

bool can only take true and false as a value, but BOOL is an int (on  
MS platforms anyway) which means that TRUE and FALSE are only two  
possible values.

>
>> 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).

Leave it in for now. Let it get revisited when logging is sorted out.

>
>> 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.

OK.

>
>> bin/bbackupd/BackupClientContext.cpp
>> 	-- if Protocol doesn't log it, it wasn't sent.
>
> OK, removed the extra logging.

But I wonder if it's actually working then. Was the problem only  
reported under Win32?


>
>> 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.

Thanks.

>
>> 	-- 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.

I do see your point. However, it means that it'll only print things  
out a line at a time, which may make things nicer for stuff reading  
the data through a pipe. Although it's not a huge thing, I suppose.

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

I'm sure I would have put them in if I were writing it. They're new  
according to the diff.

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

Maybe.

>
>> 	-- 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.

Oh, I see. Isn't Windows fun?

>
>> 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.

Well if they're needed, they shouldn't be in contrib, I suppose.

>
> 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?

I make multiple distributions from a single tree. It would get very  
messy if things were done differently.

>
>> 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.

:-)

Ben