<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Comments inline.<br>
    <div class="moz-cite-prefix"><br>
      On Tuesday 21 October 2014 06:28 PM, Maximiliano Curia wrote:<br>
    </div>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">¡Hola Rahul!

The review process involves checking and fixing the packaging, and checking
upstream code for possible errors/incompatibilities with the way things are
done in the distribution. It takes time from both of us.</pre>
    </blockquote>
    Totally understand and appreciate this. I didn't think that a
    package in Ubuntu mainstream would need so much review.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

My ultra motive to offer you to review the package is to have more members
engaged in the team, not to push things that are not up to the quality expected
in Debian.</pre>
    </blockquote>
    Agreed. But it would be great if we can have this in Debian Jessie.
    Is it still possible?
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">
There are a couple of fixes in the upstream git, last commit is 2014-05-08,
you might want to include those.</pre>
    </blockquote>
    Done.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">
To be under the kde team umbrella the package should be something like:
Maintainer: Debian/Kubuntu Qt/KDE Maintainers <a class="moz-txt-link-rfc2396E" href="mailto:debian-qt-kde@lists.debian.org"><debian-qt-kde@lists.debian.org></a>
or:
Maintainer: Debian KDE Extras Team <a class="moz-txt-link-rfc2396E" href="mailto:pkg-kde-extras@lists.alioth.debian.org"><pkg-kde-extras@lists.alioth.debian.org></a>
or:
Maintainer: Debian Krap Maintainers <a class="moz-txt-link-rfc2396E" href="mailto:debian-qt-kde@lists.debian.org"><debian-qt-kde@lists.debian.org></a>

The field: XSBC-Original-Maintainer is not considered valid in Debian
packages.

Add add yourself to the Uploaders list.</pre>
    </blockquote>
    Done.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

In the debian/copyright file:
Source: <url://example.com>
Please update the template to point to the upstream git repository.</pre>
    </blockquote>
    Done.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

Also in the debian/copyright file, the debian/* path is licensed under a more
restrictive license than the upstream code (GPL, and LGPL respectively), this
kind of licensing could block patches in the debian package from ever be
applied upstream and should be avoided. I pinged Rohan about this.</pre>
    </blockquote>
    Ok.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

In Debian the pam modules are named libpam-$module, please rename the binary
package.</pre>
    </blockquote>
    Done.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

The description provides almost no information, please extend it. Consider
using the kwalletmanager description, and adding a paragraph about the pam
module (ala libpam-gnome-keyring).</pre>
    </blockquote>
    Done. I also added a README file describing the prerequisites and
    necessary configuration.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

It's a good idea to set the build dependencies versions to (at least) the ones
listed in the CMakeLists.txt, in this case cmake (>= 2.8.8) and
libgcrypt11-dev (>= 1.5.0).</pre>
    </blockquote>
    Done.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

In the code I don't see any obvious errors, but I'm not an expert in pam
modules, some comments though:
In kwallet_hash, after the call to error = gcry_kdf_derive(..) it's not
checking in error returned something.

In prompt_for_password, the memset in the lines:
    struct pam_response *response = NULL;
    memset (&response, 0, sizeof(response));
is redundant.
</pre>
    </blockquote>
    I have not reviewed the upstream code (not sure if I'll be able to
    understand it also). Also, I prefer to leave upstream code unchanged
    unless it breaks something or has some security or performance
    issues.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">
Also, the normal review process is done via mentors.debian.net, where you
could upload the package and send a RFS, I prefer using a git repository where
I can see the changes made, and afterwards integrate the changes in a repository
for the package, either one is fine, or even an uri where I can fetch the
package source (I don't care about the binary file).</pre>
    </blockquote>
    You can get the source at <b><a class="moz-txt-link-freetext" href="https://github.com/amaramrahul/pam-kwallet">https://github.com/amaramrahul/pam-kwallet</a></b><br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

In any case, I would prefer not to have the packages as attachments, specially
in bugs and the team mailing lists, so, unless you can't publish the files
somewhere else, please avoid sending them like so. And if you really have to
send the files as attachments, please send them via direct mail, without
copies.</pre>
    </blockquote>
    Point noted.<br>
    <blockquote cite="mid:20141021125826.GB25395@gnuservers.com.ar"
      type="cite">
      <pre wrap="">

Thanks,
</pre>
    </blockquote>
    <br>
    Looking forward to your response.<br>
    <br>
    Thanks,<br>
    Rahul.<br>
  </body>
</html>