[Box Backup-dev] Reviewing code -- help!

[Chris Wilson]

Hi Ben,

>> >  WinNamedPipe should be an IOStream derived class, then use 
>> >  IOStreamGetLine not PipeGetLine.

Now done. At least, it compiles with MinGW, not tested yet (and still 
compiles on Linux).

>> >  WinNamedPipe::Write may not work in edge cases where it doesn't write 
>> >  all the bytes given.

According to MSDN, if I read it correctly, WriteFile will not do partial 
writes, but block until it's written everything (or an error occurs). 
There's no way to get the number of bytes written back from it.

[http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/fs/writefile.asp]

>> >  The .bat, config and installer files should not be in lib/win32 as 
>> >  they're not generic win32 stuff. I suggest bin/bbackupd/win32

Ok, done.

> They're not really documents either. Surely the build system will be 
> packaging them up? Perhaps they could be included in the parcel making 
> script?

Done as well, with some small changes to makeparcels.pl to allow including 
files ONLY on some platforms.

> I think some of the structure definitions in emu.h must have come from 
> somewhere.

Sorry, not my department. You need to go to Information Retrieval. 
</brazil>

>> >  Not keen on the global and member function names beginning with lower 
>> >  case letters (and other style "violations").

OK, I think I've taken care of the thread method in BackupDaemon and the 
WinNamedPipe (now called WinNamedPipeStream).

> I think this was in the threading code. Maybe I was imagining it, there's a 
> lot there.

There were quite a few global functions and variables in emu.cpp, 
bbwinservice.cpp and ServiceBackupDaemon.cpp with such names. I've fixed 
all of those that I can see and which I don't think are emulations of 
system library functions.

I don't know of any others, but please point them out if you want me to 
fix them.

>> >  -AX_CHECK_MOUNT_POINT(, [AC_MSG_ERROR([[cannot work out how to discover 
>> >  mount points on your platform]])])
>> >  +AX_CHECK_MOUNT_POINT([],[])
>> > 
>> >  I'm not an autoconf sort of person, but that looks worrying to me? But 
>> >  otherwise there's only minor things.
>> 
>>  If you mean the fact that I disabled the fatal error - Win32 has no mount 
>>  points, as far as I know, so if autoconf bombs out then you can't build.
>
> But doesn't that break other platforms?

As Martin pointed out, all it means (I think) is that Box now configures 
on platforms where statfs does not return a f_mntonname. With the latest 
changes in 06-code-cleanups, Win32 now uses Nick's statfs function again, 
and platforms without f_mntonname will try to work it out from mtab.

>>  I couldn't see an easy way at the time to disable this check just for 
>>  Win32, although I'm learning and I have some ideas now.
>> 
>>  If you mean the empty strings [], then I don't know. I thought it looked 
>>  better than entirely missing arguments. However, I don't know much about 
>>  Autoconf's quoting rules, perhaps someone can correct me.
>
> I hope Martin will come up with the answer.

I've disabled the check and removed the GPL code from SVN. Now I just 
don't even check for nanosleep on Win32, as we never call it or even 
compile the code that uses it.

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 |