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

[Paul Wise]

On Mon, Dec 7, 2015 at 4:49 PM, Lennart Weller wrote:

> I am looking for a sponsor for my package "network-manager-ssh"

I don't intend to sponsor this but here is a review:

This might be a blocker, depends on if ftpmasters notice:

debian/copyright doesn't acknowledge Dan Williams although some source files do.

Here are some issues that would be nice to fix:

Why do the last two overrides in debian/rules have two tab characters
instead of one?

The upstream README file is not useful, please install README.md
instead. However README.md includes install instructions and usage
instructions for people on non-Debian distros, which aren't useful to
users of Debian binary packages so I would sed those parts out during
`debian/rules build`.

The upstream NEWS file is not useful, please don't install it.

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.

It would be great if there were a DEP-8 test:

http://dep.debian.net/deps/dep8/
http://ci.debian.net/

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

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

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

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

Please add some upstream metadata:

https://wiki.debian.org/UpstreamMetadata

If you contact upstream, please mention the Debian upstream guide:

https://wiki.debian.org/UpstreamGuide

Automated checks:

check-all-the-things:

$ find -type f -iname '*.sh' -exec checkbashisms {} +
could not find any possible bashisms in bash script ./test/nm-ssh-server.sh

$ codespell --quiet-level=3
./src/nm-ssh-service.c:907: arguements  ==> arguments

$ find -type f \( -iname '*.sh' -o -iname '*.bash' -o -iname '*.zsh'
\) -exec shellcheck {} +

In ./test/nm-ssh-server.sh line 27:
    local external_interface=`_get_external_interface`
              ^-- SC2155: Declare and assign separately to avoid
masking return values.
                                 ^-- SC2006: Use $(..) instead of legacy `..`.


In ./test/nm-ssh-server.sh line 28:
    iptables -t nat -I POSTROUTING -o $external_interface -j MASQUERADE
                                          ^-- SC2086: Double quote to
prevent globbing and word splitting.

$ 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

$ find \( -name .git -o -name .svn -o -name .bzr -o -name CVS -o -name
.hg -o -name _darcs -o -name _FOSSIL_ -o -name .sgdrawer \) -prune -o
-empty -print
./ChangeLog
./NEWS
./intltool-extract.in
./intltool-update.in
./intltool-merge.in

$ flawfinder -Q -c .
<lots>

$ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec
POFileChecker {} +
<lots>

$ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec POFileSpell
{} +
<lots>

$ find -type f \( -iname '*.po' -o -iname '*.pot' -o -iname '*.mo' -o
-iname '*.gmo' \) -exec i18nspector {} +
<lots>

$ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec msgfmt
--check --check-compatibility --check-accelerators
--output-file=/dev/null {} \;                       <lots>

$ grep -riE 'fixme|todo|hack|xxx' .
<lots>

-- 
bye,
pabs

https://wiki.debian.org/PaulWise