cochise added inline comments.
INLINE COMMENTS
> dfaure wrote in jobtest.cpp:118
> typo in Xatr, and in "foud". I suggest:
>
> qWarning() << "Neither getfattr, getextattr nor xattr was found";
>
> Although I have to wonder why this looks for all three, to then only use
> getfattr and skip tests in all other cases....
The check is to build infrastructure for anyone with access to the platforms
that want to add their test.
Not sure if this would ever happens, but it's here.
> dfaure wrote in file_unix.cpp:164
> Can this *ever* return an empty list, because keylist was empty?
>
> (Then last() will assert on the next line)
Not because if the file don't have a xattr the function returns on 153. The
returned keylist should have at least one key.
Removing last, if empy is done because in my tests the keylist includes a empty
attribute that is preserved, and appended after each copy.
if (listlen == 0) {
qCDebug(KIO_FILE) << "file " << src_fd << " don't have any xattr";
return false;
}
> bruns wrote in file_unix.cpp:164
> Why a temporary list at all?
>
> off_t offset = 0; size_t keyLen;
> while (offset < keylist.size()) {
> #if BSD
> keyLen = static_cast<unsigned char>(keylist[offset]);
> offset++;
> #elif LINUX
> keyLen = strlen(keylist[offset]
> #endif
> key = keylist.mid(offset, keyLen);
> /* copy */
> ...
> #if BSD
> offset += keyLen;
> #elif LINUX
> offset += keyLen + 1;
> #endif
> }
As the system functions need individual keys to get values and a individual key
value pair to set, the list is to make more easy to iterate over keys. We can
iterate over the buffer, reading until '\0', but this is a little more bug
prone.
> bruns wrote in file_unix.cpp:164
> This is wrong for the BSD implementation:
>
> > extattr_list_file() returns a list of attributes present in the requested
> > namespace. Each list entry consists of a **single byte containing the
> > length of the attribute name**, followed by the attribute name. The
> > attri-
> > bute name is **not terminated by ASCII 0 (nul).**
Well... This will need some platform specif code to read the buffer and place
in the m_keyList, ad the .split() will not work. Using the list is mandatory
now, as we have different format buffers.
Committing other fixes, ad they are really small and postponed this one.
> bruns wrote in file_unix.cpp:180
> Allocate outside the loop
The value change on every pass and we only get the size inside the loop.
Allocating outside the loop is more performant than resizing for each pass?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D17816
To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho,
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane,
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh
