GSOC report 2: Improvements in smbclient backup mode

David Disseldorp ddiss at suse.de
Sun Jul 14 17:35:56 MDT 2013


Hi Aurélien,

On Fri, 5 Jul 2013 18:43:56 +0200
Aurélien Aptel <aurelien.aptel at gmail.com> wrote:

> This week I've written more tests for my test suite which now covers
> most usage of smbclient tar feature. The code is fully documented in
> the source and the documentation can be exported as a manpage or as
> HTML[1].
> 
> The script is available on my public repo on the gsoc_test_tarmode
> branch[2].

I've finally had some time to look at your test_smbclient_tarmode.pl
script.
First off, I'm very impressed by the code quality and level of coverage
so far. Furthermore, the documentation you've added should make it easier
to maintain in future.

Your comments about it being a little over-engineered do seem relevant,
but I don't find the wrapper or result-reporting code overly long or
complex, so would be happy to see it left as-is. Though, preferably with
the result output colouring removed.

As discussed earlier, for this to be used as part of automated
regression testing it needs to be triggered by Samba's selftest
test-harness, which can be done by adding it to
source3/selftest/tests.py. Once added, the script will be executed as a
part of "make test" and run against all future changes pushed to the
master branch.

I don't quite think it's ready to replace test_smbclient_tarmode.sh yet,
as the system-tar vs smbclient tar checks have been useful in
identifying differences between the archive formats produced and
consumed by the two implementations. test_smbclient_tarmode.pl deals
only with smbclient archives IIUC.

Finally, I've a few minor comments regarding coding style, etc.:
- Please add a space between if/for/while/etc. and round brackets. e.g:
	if($USER xor $PW) {
- Should be:
	if ($USER xor $PW) {

- Don't split if/else clauses. Looks like this only occurs in one place:
	if($SINGLE_TEST == -1) {
	    run_test(@TESTS);
	}

	elsif(0 <= $SINGLE_TEST && $SINGLE_TEST < @TESTS) {
...

- Use mkdtemp(), rather than hardcoding the TMP path.

Cheers, David


More information about the samba-technical mailing list