[Pkg-utopia-maintainers] Bug#807310: RFS: network-manager-ssh/0.9.4-1 [ITP] - ssh vpn for network manager

[Lennart Weller]

Thank you for the extensive review. I went through your suggestions/comments 
and either fixed them or mentioned them here.

> I don't think the package descriptions should describe NetworkManager,
> that is a job for the network-manager package description. Instead,
> they should describe the network-manager-ssh packages.

I know it's weird and somewhat unusual but all the other nm plugins do the same. So I went with it:
http://anonscm.debian.org/cgit/pkg-utopia/network-manager-openvpn.git/tree/debian/control
http://anonscm.debian.org/cgit/pkg-utopia/network-manager-pptp.git/tree/debian/control
http://anonscm.debian.org/cgit/pkg-utopia/network-manager-vpnc.git/tree/debian/control

> It would be great if there were a DEP-8 test:
> 
> http://dep.debian.net/deps/dep8
> http://ci.debian.net

This would definitely be a nice-to-have but it would require a decent test in 
the source package or writing one myself. As of now the test under test/ really
doesn't do much. It basically just setups the env to actually start testing. 
So I'd say its more of a long term goal.

> I always wonder if screenshots like images/*.png should be generated
> at build time so they never get out of date.

Screenshots don't change all that often and even if they are slightly out-of-date 
most people won't care. I'd say many just check the screenshots to check if its
a GTK1 application last maintained in the 90s.

> I wonder what "PCF" in the icon means, probably it would be better to
> have "SSH" in there?

The icon seems to be from nm-pptp. But even there it doesn't really make sense.
So I guess the icon went x -> pptp -> openvpn -> ssh. 

> Upstream's po files look like they have poorly maintained or
> unmaintained copyright headers.

Most of them seem alright to me. A few have missing source package information 
but that information is available in debian/copyright anyhow. The translators seem
to be attributed in all of them.

> Upstream's test suite is using an IP address belonging to the USA
> Department of Defence. I would strongly suggest using a proper private
> IP address according to RFC 6761.
> 
> http://tools.ietf.org/html/rfc6761

inetnum:        1.1.1.0 - 1.1.1.255
netname:        APNIC-LABS
descr:          Research prefix for APNIC Labs
address:        South Brisbane, QLD 4101
address:        Australia
e-mail:         abuse at apnic.net

I know you are not supposed to use these. But the test is not executed and 
won't be unless upstream changes it as it is useless right now.

> Please add some upstream metadata:
> 
> https://wiki.debian.org/UpstreamMetadata

Following the links on the page it seems to be mostly used by scientific 
libraries to reference algorithms used in the packages. All the basic 
repository/issue/copyright information is already present in other files
in my current version.

> Automated checks:
> 
> check-all-the-things:
> $ cppcheck -j1 --quiet -f . | grep -vF 'cppcheck: error: could not
> find or open any of the paths given.'
> [properties/nm-ssh.c:1287]: (error) Possible null pointer dereference: gateway
> [properties/nm-ssh.c:1289]: (error) Possible null pointer dereference: remote_ip
> [properties/nm-ssh.c:1290]: (error) Possible null pointer dereference: local_ip
> [properties/nm-ssh.c:1291]: (error) Possible null pointer dereference: netmask
> [properties/nm-ssh.c:1316]: (error) Possible null pointer dereference:
> preferred_authentication
> $ flawfinder -Q -c .
> <lots>

I'll report these upstream as these check seems to be of valid concern.
Not security wise but to avoid errors.

Thanks again,

Lennart Weller