Bug 9667 - smbclient tar mode should use libarchive
smbclient tar mode should use libarchive
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: Other
unspecified
All All
: P5 enhancement
: ---
Assigned To: Samba QA Contact
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-18 11:31 UTC by David Disseldorp
Modified: 2015-12-11 19:03 UTC (History)
4 users (show)

See Also:


Attachments
v4-1-test rebase of libarchive smbclient tarmode changes in master (146.34 KB, patch)
2014-02-24 11:38 UTC, David Disseldorp
no flags Details
v4-1-test rebase of libarchive smbclient tarmode and test-suite changes in master (185.92 KB, patch)
2014-02-24 15:38 UTC, David Disseldorp
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2013-02-18 11:31:52 UTC
smbclient currently includes a home-made tar archive creation and extraction implementation. There have been a number of bugs which can be directly attributed to this implementation:

https://bugzilla.samba.org/show_bug.cgi?id=102

https://bugzilla.samba.org/buglist.cgi?quicksearch=smbclient+tar


Furthermore, smbtorture test failures are common when tested against recent versions of GNU tar:

UNEXPECTED(failure): samba3.blackbox.smbclient_tarmode (s3dc).test_tarmode_extraction(s3dc)
REASON: _StringException: _StringException: Domain=[SAMBA-TEST] OS=[Unix] Server=[Samba 4.1.0pre1-DEVELOPERBUILD]
tarmode is now full, system, hidden, noreset, verbose
this tar file appears to contain some kind of link other than a GNUtar Longlink - ignoring
Skipping \PaxHeaders.7449\tarmode...
restore directory \tarmode\
this tar file appears to contain some kind of link other than a GNUtar Longlink - ignoring
Skipping \tarmode\PaxHeaders.7449\file.10...
restore tar file \tarmode\file.10 of size 142336 bytes
this tar file appears to contain some kind of link other than a GNUtar Longlink - ignoring
Skipping \tarmode\PaxHeaders.7449\file.7...
checksums don't match 0 65688
abandoning restore, -1 from read tar header

I propose removing the hand-rolled archival implementation and using libarchive instead, which alleviates the burden of cross platform/version format support.
Comment 1 Björn Jacke 2013-02-19 15:06:23 UTC
+1 to optionally support libarchive or make the tar feature optionally build depend if libarchive is available. Importing libarchive into the samba tree is probably too heavy. Have you already patches for that?
Comment 2 David Disseldorp 2013-02-19 15:14:50 UTC
(In reply to comment #1)
> +1 to optionally support libarchive or make the tar feature optionally build
> depend if libarchive is available. Importing libarchive into the samba tree is
> probably too heavy. Have you already patches for that?

Agreed, I'm definitely against importing libarchive into the tree. I haven't done any work towards implementing this so far, and can't promise I'll get to it. I expect there are very few people using smbclient tar functionality.
Comment 3 Wagner Caixeta Rodrigues 2014-02-13 17:24:09 UTC
I agree!

There is a lot of others project depending of this.
AmandaBackup, Bacula, BackupPC.

smbclient does not work to exclude very well, even with "-r" flag.
Comment 4 Wagner Caixeta Rodrigues 2014-02-13 17:28:27 UTC
I found a branch with this fixed 

https://bitbucket.org/knarf/samba/commits/branch/gsoc_clitar_libarchive

it is not complete yet, i will try.
Comment 5 Wagner Caixeta Rodrigues 2014-02-13 17:29:50 UTC
More info: http://diobla.info/doc/gsoc13/journal
Comment 6 David Disseldorp 2014-02-13 17:32:48 UTC
(In reply to comment #4)
> I found a branch with this fixed 
> 
> https://bitbucket.org/knarf/samba/commits/branch/gsoc_clitar_libarchive
> 
> it is not complete yet, i will try.

Aurélien's GSoC work was successfully completed.
His test suite has already been merged upstream, the remaining smbclient tarmode changes shouldn't be far off. I just need to finish reviewing everything.
Comment 7 David Disseldorp 2014-02-21 17:19:25 UTC
Aurélien's libarchive smbclient tarmode changes are now all in master:

913b2a172d74b60bb5a7396eb09ad4af4b748021 clitar: don't panic, propagate talloc errors upwards
83a653fadbdb3167ac53b51b2694d2fd6baf962b clitar: propagate make_remote_path() talloc errors
55de6d60ef72b105883117b36963a7a145268223 clitar: return allocation errors from is_subpath()
852259773b4a2ab63ed6df2b41fa73eb22ba81c4 clitar: add error return to tar_path_in_list()
6d5b56dc7ab8b957af58c584d91ac4864156ced2 clitar: add error return to tar_extract_skip_path()
4d9e1b68b7b636a435f55c25df9ab8f51922b36e clitar: add error return to tar_create_skip_path()
385f0c9ea05a0f37388cf549102fc13ef9e6691a clitar: check for path_base_name() allocation errors
...
497f0327a08fbfa444308c90a418ccb6b45b96d6 build: check for libarchive version via pkgconfig
43227c3d9fda806dc510fd552e340127192b9424 cli: do not dump libarchive absence warning on startup
67590086664abcae2fe1478a58ab45d5ff5594ec s3-clitar: Fix identation.
995118484f7c1ef92b5539567cf878acc1c48b0a s3-clitar: Simplify is_subpath().
3b207dc0f3ef642ec5f16bc1dbce7c018c89cf55 s3-clitar: Improve readabilty of fix_unix_path().
ef150e7dfa17bd0b6432ac8b5e1a6afd055edb17 s3-clitar: Improve readabilty of max_token().
4f6552c63ba0cc293463f38ed08cd30399fdad65 s3-clitar: Improve readabilty of make_remote_path().
98ede411e3baeb4da8854450e002e4478c32ddd3 s3-clitar: Improve readabilty of tar_path_in_list().
2ccba45d301997042e9b3ba07bd157d02b7e7b94 s3-clitar: Improve readabilty of tar_read_inclusion_file().
f955bc4b5de449a6c6ffcbefa807b207d754adf8 s3-clitar: Improve readabilty of tar_set_newer_than().
75dbb27115ff2d32a69215075cc79bdfe956160f s3-clitar: Improve readabilty of tar_send_file().
f58ef3878e0718cd95e09ffbba5dbaa374c2c2f0 s3-clitar: Improve readabilty of tar_extract().
f8ac29d1acb528ae2471ac1300d134588ccaf990 s3-clitar: Improve readabilty of get_file_callback().
4dd84bb8300e987acaad6d887bf32438e668f847 s3-clitar: Improve readabilty of tar_create_from_list().
87c5258fa38bcef3776c42abe636291d16ec65b0 s3-clitar: Improve readabilty of tar_parse_args().
034da62e6bfa006974aac345d5140344c136a2d5 s3-clitar: Improve readabilty of cmd_setmode().
7bb9836c94e5659a2b9d6cca3becdd1dfc205932 s3-clitar: Improve readabilty of cmd_tar().
a65f557c6b588b725d1e4900f1a0bf5772d9c892 s3-clitar: Improve readabilty of cmd_block().
3707fba088873988427875c60ae379dc6e218eda s3-clitar: Use ARRAY_SIZE macro.
8524cf69d683107a488945bf39c55ceb57ed19a9 build: use configure var for libarchive depenency
8dc6f0fb39647e37a444ac582b5b33e27b40b3dc clitar: get tar context handle via helper function
14c6e9b6b8c0f67a0cd85508c94413fb42ac20f7 s3: fix --with-libarchive bug, remove useless DEFINE()
a66942df05ea536cdb2680f658b3d0d5683cceef clitar.c: fix segfault in cmd_block()
95f9e60ef70a1925a463038f41476400b8990d5c s3: add --with-libarchive to build configuration
92356e1524b97abc7e8f8fb5c7e625dc200de277 s3/selftest/tests.py: add test_smbclient_tarmode.pl to test suite
1be1303f1bfdd99bd13e3270fb87bbe02dc2b69a manpages/smbclient: remove trailing whitespace
2155b5bb40403132117fb5fcb054ef036a4fedc1 manpages/smbclient: update tarmode documentation
b8844fcdaecdcaefe73b0455ed61da4d93c74793 clitar.c: check all allocations return value
68305d90b3cfaa85d64321486e12c70eb2837111 clitar.c: create and free talloc context properly
08f3c4e942ea3ff1d18ae4f39cce3eb271ad901e clitar: remove unnecessary public function prototypes
44265721bc7137b7382c720f2b896653085e43f1 clitar.c: add prototype, rearrange definition for easier reading.
5cd72b39b35efc5129596ed258187def57f2f209 clitar.c: documentation
82dce8f7a8d340a39e08dcc12fb38b8ed254dc64 clitar.c: honor regex flag, emulate old behaviour (and quirks), add tests
fc05ff32fd7e8484f00e02a27d000e05748cdf74 clitar.c: blocksize in block unit, fix error handling in tar_create().
cae67383ddd4373dcf8fc1f82a41dc30b1b7d299 client: fix tar_parse_args() compiler warning
3a7efaa009d0d171b7c5920ccdf1fc43099d63c7 clitar.c: when returning int, always use 0 for success
5fbe36cbaf2421327f25576e1b2c76322b765479 clitar.h: delete file
9cb90772339c4beb87d85229b4010e7d36d12205 clitar.c: default block size was a multiple of TBLOCK
7c973de80218a1f6f45c2aeeec4f485690d163bd clitar.c: only list <include list> instead of whole share, handle wildcards
679cd1627021daf96ddc4c1dd8f71a2067dbaa99 clitar.c: implement interactive command + respective test
3348b139d2a73c62181ad71be10973ec6e3a0366 clitar.c: cosmetic changes
5f99b0915da12a2455555de4e9ac27d06fc92346 clitar.c: operation INCLUDE_LIST same as INCLUDE
6d0ff6d2f0366e2b8f6d5dc3b15ddb3abb57163a clitar.c: fix creation exclusion, don't reverse in exclusion mode
b8258540055e01fcb2d26776be4b5914b00b5a8d clitar.c: fix include mode bug
cbad767ded9fa50bb3045809e6af3287a59c69b4 clitar.c: implement reset mode
e2b0092e1dd7c5a9657960a2d80970dd9d04332e clitar: implement exclusion filter for extraction
d96e2b205298b637ba0d7eb7b2d47a818e64737d clitar.c: implement dry mode, add total_size and fix write bug
d23074cece79432bbeeac1d364300a9ccb0d5f76 clitar.c: add line in DBG macro
39a21cdce3ba6deeb7c787de68f41ba88b87e88b clitar.c: implement nosystem, nohidden and include filter.
9032fc7eec40dcd42b853ccd95e3c69c069b0b58 clitar.c: implement basic tar creation
ec1583ebd48d0077aa48149bcabd47fd87c65c83 clitar.c: don't create file when processing dir
b7f6fd26b6346eac7ec37cf6742d49cd179df960 clitar.c: implement basic full extraction
c9e23fd1a6c7595413dc0ea93db2c9936051b51d clitar.c: fix_unix_path() now replace / with \
dfd627b902c8b81346fb8c5fd5f666a70762286a clitar.c: factor path_list handling in a function, remove tar_fd
5af65f87b563690be773e79f1061225ab60d5d32 clitar.c: update TAR_DEFAULT_BLOCK_SIZE comment
29542d1cb5d94b5edf952210f800db1e7b0d210b clitar.c: start processing tar files in extraction mode
ed9d22097d2595fb096646f3a63ec434c4f20b60 client.c: fix negation in tar_to_process test
0babaf7e9de4d700afb465d93d55c9ee42547dc3 s3/wscript, s3/wscript_build: add libarchive dependency.
50d069637d9a95c5728069600f9cd9e945725a42 client.c, clitar.c: rename process_tar to tar_process, prepare code
60eec86ceb08814dabb986219f756d1645efb82c clitar.c, client.c: add flag to know if there's a tar operation to do.
b753900b09e6b5001042e45388b72ad36e8a0093 clitar.c: fix path name when adding them
3a8e3264d0da83817629def666c483b558d8d49d clitar: use lowercase for bool value
113aa6f3d4b784e707bc8cc7084dd912f6a77252 clitar.c: add tar_dump() debug function, fix bugs
431483e6b9210c396ddf119ded603ecc95c1ea98 client.c: remove unused extern declaration
45a45c8edac20469e5b9e7b30a55e3a591b62f79 implement argument parsing, split client_proto.h
eebd378b6cbc4b19c35b670cd5e493ef575c19cb client.c: remove trailing whitespace
342d38eb4b872c341cf25884f795ae38b04fca0f clitar.c: start of argument parsing
b9b5bc433d07376ae0bd96de3704162c67684734 clitar.c: add doc, remove _t suffix in enum name
a896f046fecde4d6601d4789ad2a7057a1d0c8da clitar.c: add cmd_setmode(), remove typedef
1d142c6237ded9994e4846fccb5c2ea085fb31ee clitar.c: expand context structure and implement cmd_block()
2945596011cc31df938692bdbad04e2feaee6fbb clitar.c: fresh new compilable file.

Given the added libarchive dependency, I expect the patch set is not suitable for 4.1.
Comment 8 David Disseldorp 2014-02-24 11:38:56 UTC
Created attachment 9713 [details]
v4-1-test rebase of libarchive smbclient tarmode changes in master

Although mostly isolated to the smbclient tar code-path, this patch-set adds a new libarchive dependency, so may not be suitable for a maintenance release.

Andreas, Björn and Andrew: what do you think?
Comment 9 Björn Jacke 2014-02-24 12:06:52 UTC
On 2014-02-24 at 11:38 +0000 samba-bugs@samba.org sent off:
> Although mostly isolated to the smbclient tar code-path, this patch-set adds a
> new libarchive dependency, so may not be suitable for a maintenance release.
> 
> Andreas, Björn and Andrew: what do you think?

new features are not allowed in stable release branches by policy in general.
especially if library dependencies change this is a not a good thing. All we
can do is provide a clean patch set so that distributions can add the patch to
their packages on their own risk.
Comment 10 David Disseldorp 2014-02-24 13:32:40 UTC
(In reply to comment #9)
...
> > Andreas, Björn and Andrew: what do you think?
> 
> new features are not allowed in stable release branches by policy in general.
> especially if library dependencies change this is a not a good thing. All we
> can do is provide a clean patch set so that distributions can add the patch to
> their packages on their own risk.

Sounds sensible. Closing accordingly.
Comment 11 David Disseldorp 2014-02-24 15:38:12 UTC
Created attachment 9714 [details]
v4-1-test rebase of libarchive smbclient tarmode and test-suite changes in master
Comment 12 Alan Bowler 2015-12-11 17:44:01 UTC
The new TAR with libarchive seems to ignore the tarmode verbose mode.
BackUpPC expects this output.  Is there some way to turn it on?
Comment 13 Andrew Bartlett 2015-12-11 19:03:03 UTC
(In reply to Alan Bowler from comment #12)
Please file a new bug for this regression (it can 'block' this bug to keep a reference).

Make sure to include steps to reproduce (the command line BackupPC uses) and the difference in the output.

Thanks!