Review Request 116798: Rewrite KUser/KUserGroup Widows implementation + introduce KUserId/KGroupId

[Michael Pyne]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116798/#review52955
-----------------------------------------------------------


Looks OK overall. I'll admit to getting coincidentally tired right as I got to kuser_win.cpp, so I hope that all works (it's been a very long time since I coded for Windows, although I'm surprised how familiar it looks).

There are some minor issues that would need to be fixed before the KDE and UNIX parts can go in. Especially on the documentation side, our docs are bad enough as it is without adding more undocumented exported library classes. :)

Hopefully someone else can look as well though. I'm only reviewing this because I promised ervin I'd maintain kcoreaddons. :P


autotests/kusertest.cpp
<https://git.reviewboard.kde.org/r/116798/#comment37263>

    The year seems a bit off here (and in other files).



src/lib/util/kuser.h
<https://git.reviewboard.kde.org/r/116798/#comment37264>

    Same issue with the year (copy 'n' paste is the best, no?) ;)



src/lib/util/kuser.h
<https://git.reviewboard.kde.org/r/116798/#comment37265>

    I think you can also add @internal or a similar Doxygen tag to note the same effect.



src/lib/util/kuser.h
<https://git.reviewboard.kde.org/r/116798/#comment37266>

    The TODO is appropriate, but this would actually need to be documented before it goes in. ;)



src/lib/util/kuser.h
<https://git.reviewboard.kde.org/r/116798/#comment37267>

    Ditto.



src/lib/util/kuser.h
<https://git.reviewboard.kde.org/r/116798/#comment37268>

    Should this be deprecated in favor of the new userId()? Or are they both appropriate for applications to use?



src/lib/util/kuser_unix.cpp
<https://git.reviewboard.kde.org/r/116798/#comment37269>

    I know this one isn't your fault, but there should probably be a call to setpwent() here to reset the /etc/passwd file pointer if some other code had called getpwent() earlier.



src/lib/util/kuser_unix.cpp
<https://git.reviewboard.kde.org/r/116798/#comment37270>

    I think you can just do "return d->users.mid(0, (int)maxCount)" here. (You'd want to check that (int)maxCount isn't negative first, of course).
    
    Although ::mid() uses int instead of uint for the length, it's also true that QList itself doesn't seem to hold more than int elements if you look at the return type of QList<T>::size().



src/lib/util/kuser_unix.cpp
<https://git.reviewboard.kde.org/r/116798/#comment37271>

    Again, might want to put a setgrent() here.



src/lib/util/kuser_unix.cpp
<https://git.reviewboard.kde.org/r/116798/#comment37272>

    And a setgrent() here.


- Michael Pyne


On March 14, 2014, 2:30 p.m., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116798/
> -----------------------------------------------------------
> 
> (Updated March 14, 2014, 2:30 p.m.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> This was all started in order to get KIO to compile on Windows (uses uid_t/gid_t, getpwent, getpwuid, getuid,  etc.)
> 
> This patchset introduces KUserId/KGroupId which is a no-overhead wrapper around uid_t/gid_t and implicitly shares a SID on Windows.
> 
> Also introduced a maxCount paramter to all listing functions of KUser/KUserGroup so that the manual calls to getpwent() in KIO can be replaced
> 
> Finally added a unit test for KUser, KUserGroup, KUserId, KGroupId to ensure that these changes are safe
> 
> Windows only changes:
> - KUser::homeDirectory() works properly, before it always returned the home directory of the current user
> - KUser::faceIconPath() is now implemented on Windows
> - Use scoped pointers everywhere -> no memory leaks (at least one was fixed)
> 
> 
> This is actually a series of commits, if you think that is easier to review I can push them somewhere.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 30ac97822a77e4b12b0e81a4a5c93fe9a768d915 
>   autotests/kusertest.cpp PRE-CREATION 
>   src/lib/util/kuser.h 42c81156831d0269c89435c76c3572dcf2e085b5 
>   src/lib/util/kuser_unix.cpp aa2f9073015c7f9feb8f74e3646928d2fa2de007 
>   src/lib/util/kuser_win.cpp 06759afa34ea7015a44cf9b38f541f944f8126d4 
> 
> Diff: https://git.reviewboard.kde.org/r/116798/diff/
> 
> 
> Testing
> -------
> 
> Wrote new unit test, passes on Linux and Windows, KIO is closer to compiling on windows
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140315/d988c041/attachment-0001.html>