> On Feb. 15, 2015, 10:37 nachm., David Faure wrote: > > Nice, love unittests. > > > > Most of the new code in kzip.cpp is a copy of the contents of the previous > > while loop, though. > > Is there any chance for extracting this into a helper method, to avoid the > > duplication? It would facilitate maintenance in the long run, especially if > > more changes around this are needed.
I resisted any refactoring... but if you ask for it... ;) Updated the diff to include some for the code that is affected by duplication. Should also bring a fix the `compr_size > dev->size()` branch, as that one so far did not do the check if the checked 4-byte block was not across another token start. So at least in theory that could have resulted in missed header tokens, no? Also improved that logic slightly and only do seeking back as much as needed if another `P` was found. Did not go the extra mile though to also cover the code for getting to start of file if the file does not start with any ZIP header, as that one only PK\003\004. Improvement with seeking only as much as needed if another `P` is found is left for a add-on commit, if this patch is okay-ed. - Friedrich W. H. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122579/#review76084 ----------------------------------------------------------- On Feb. 16, 2015, Mitternacht, Friedrich W. H. Kossebau wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122579/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2015, Mitternacht) > > > Review request for KDE Frameworks and David Faure. > > > Repository: karchive > > > Description > ------- > > Currently the parsing code of KZip assumes that there are only data > descriptors behind the file data if bit 3 of the general purpose bit flag in > the local file header is set. But, the spec does not forbid the data > descriptors also to be used when that bit is not set, see the "SHOULD" (not > "MUST") in 4.3.9.1 of the ZIP spec > (https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT): > > 4.3.9.1 This descriptor MUST exist if bit 3 of the general > purpose bit flag is set (see below). It is byte aligned > and immediately follows the last byte of compressed data. > This descriptor SHOULD be used only when it was not possible to > seek in the output .ZIP file, e.g., when the output .ZIP file > was standard output or a non-seekable device. For ZIP64(tm) format > archives, the compressed and uncompressed sizes are 8 bytes each. > > This patch fixes that by testing for a data descriptor behind the file data. > It tests both for data descriptors with and without the PK78 signature, as > 4.3.9.3 of the ZIP spec recommends it: > > 4.3.9.3 Although not originally assigned a signature, the value > 0x08074b50 has commonly been adopted as a signature value > for the data descriptor record. Implementers should be > aware that ZIP files may be encountered with or without this > signature marking data descriptors and SHOULD account for > either case when reading ZIP files to ensure compatibility. > > The patch also comes with a unit test and two files where such redundant data > descriptors are used, once with and once without signature (hand crafted, > using "zip" and Okteta :) ). > > Motivation: > Currently Calligra Words cannot open ODT files as created by the ODT export > of DokuWiki, while at least LibreOffice can and also all the zip tools have > no problem with the file. You can create such ODT files e.g. on > http://plugtest.opendocsociety.org/ > > I have also started to prepare a patch against kdelibs 4.14 and will complete > it, once this RR has passed review. > > > Diffs > ----- > > autotests/data/redundantDataDescriptorsNoSignature.zip PRE-CREATION > autotests/data/redundantDataDescriptorsWithSignature.zip PRE-CREATION > autotests/karchivetest.h 8c4f980 > autotests/karchivetest.cpp 4dc016e > src/kzip.cpp fd9a5e0 > > Diff: https://git.reviewboard.kde.org/r/122579/diff/ > > > Testing > ------- > > All KArchive tests pass, Calligra can load the ODT files created by DokuWiki > and still other ODT files. > > > Thanks, > > Friedrich W. H. Kossebau > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
