error: defined but not used [-Werror=unused-function]

[DimitriPapadopoulos]

I am attempting to package openfortivpn for Debian. I get these error messages at compile-time:

src/io.c:49:22: error: 'thread_id' defined but not used [-Werror=unused-function]
 static unsigned long thread_id()
                      ^~~~~~~~~
src/io.c:42:13: error: 'lock_callback' defined but not used [-Werror=unused-function]
 static void lock_callback(int mode, int type, char *file, int line)
             ^~~~~~~~~~~~~

It looks like the packaging environment forces unused-function compiler warnings into errors.

[sandrotosi]

I'm preparing a package for debian and i've been biten by this just now

[DimitriPapadopoulos]

I was supposed to be preparing the package for Debian, but please proceed...

[sandrotosi]

oh now i see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=848138 but it's still marked as RFP (you sent a followup message as ITP but you forgot to retitle the original bug). do you have a package ready? i need this really quickly that's why i'm working on it but i can sponsor your work an/or comaintaing the package

[DimitriPapadopoulos]

I was not sure how to change the title of the bug, this is not documented here:
https://www.debian.org/Bugs/Developer
I've tried changing the title of the mail itself.

I'm more used to bug tracking systems with a user interface, such as Bugzilla. I must admit I find the learning curve of the Debian bug tracking system rather steep.

[DimitriPapadopoulos]

I do have packages for Ubuntu 14.04 or 16.04. They seem to be properly packaged and working well. I would love to get them reviewed.

Since I'm on Ubuntu, I tried building a Debian package using pbuilder. Again I find the learning curve rather steep: there are multiple documented ways for the most basic operations, I haven't found a straightforward and reasonably short manual yet - one that would explain how to easily run package building operations step by step, manually, to debug errors such as this one. That said I'm pretty sure this is a compiler bug, but I do want to open a bug report and work around the bug.

Then I tried installing Debian in a VirtualBox VM instead of using pbuilder. Unfortunately there are known issues with installing Debian testing (or unstable). See for example:
Can't install Debian Stretch in VirtualBox
I probably need to switch from gdm to lightgdm but haven't found the time yet.

Right now I can provide Ubuntu packages, or the files that I have used to build the package. I would also like to add these files in a public version control system, but I haven't found a standard system for the control files of Debian packages yet, at least one I have access to. Could you suggest a server running a version control system? Is GitHub OK for that?

[sandrotosi]

yeah github is fine, please upload your package there please (quick version: create a source package, create a git repo, go in the repo, gbp import-dsc --pristine-tar /patch/to/file.dsc ; git push --all) i can handle the debian side (i'm a debian developer)

[sandrotosi]

anyway, the solution to this error is to ignore the warning (which are treated as error in debian on purpose):

export DEB_CFLAGS_MAINT_APPEND = -Wno-unused-function in the debian/rules file

[DimitriPapadopoulos]

Just created this git repo:
https://github.com/DimitriPapadopoulos/openfortivpn-debian

Will attempt to populate it real soon.

[mrbaseman]

maybe adding a #pragma clang diagnostic ignored "-Wno-unused-function" in io.c could be a workaround for this too?

[DimitriPapadopoulos]

This looks like a compiler bug that will hopefully disappear in upcoming versions — I haven't experienced this bug with any recent Fedora or Ubuntu LTS release.

It's probably better to work around the compiler bug in the package specific to Debian, rather than in upstream source code. Unless you were thinking of patching io.c in the Debian package, not upstream?

[mrbaseman]

If it is a generic bug in clang (in several, widely used versions) patching it upstream might make sense. But if the bug is specific to a specific clang version used in Debian, or even specific to the clang package on Debian, you are right. Then, it makes more sense to fix it in the Debian package.

[sandrotosi]

it is not a compiler bug, it is a debian decision to treat every warning as an error.

[DimitriPapadopoulos]

The warning itself looks like a compiler bug, not the fact that Debian treats warnings as errors.

Even when compiling with -Wall on other platforms, GCC does not emit warnings.

[mrbaseman]

right. thread_id and lock_callback in fact are used. They are not called directly, but function pointers to them are handed over to openssl and clang doesn't recognize that and complains by mistake.

[DimitriPapadopoulos]

@sandrotosi Can you give it a try?
https://github.com/DimitriPapadopoulos/openfortivpn-debian

I'm not sure about debian/rules yet. For example should I add uncomment export DEB_BUILD_MAINT_OPTIONS = hardening=+all`?

By the way, I was able to build a sid binary using pbuilder after all, after adding -Wno-unused-function. I'm still not sure how to debug build errors within pbuilder—that I will learn next time.

[cablespaghetti]

Watching eagerly. If you get this working I'll see if I can make an Ubuntu ppa.

[Centuriondan]

I have a working build in Devuan now: https://git.devuan.org/devuan-packages/openfortivpn
built for experimental and also working on building for jessie-backports

Note: I'm using source format "3.0 (git)" which isn't supported in Debian... no idea about Ubuntu, but just changing the debian/source/format to "3.0 (quilt)" should be sufficent as I haven't had to patch the upstream source for this build.

[sandrotosi]

@DimitriPapadopoulos the copyright file is really poor and misses a lot of the copyright/license notices. I have a working package, I'll polish it up, add you to uploaders, uploade it to collab-main git.debian.org repo and upload it

[Centuriondan]

@sandrotosi I did also wonder about the special exception in LICENSE for linking against libssl
ping us here when it's uploaded so I can sync the Devuan package with it.

[sandrotosi]

@DimitriPapadopoulos @Centuriondan i've just uploaded it to debian experimental (due to the upcoming freeze): repo at https://anonscm.debian.org/cgit/collab-maint/openfortivpn.git

let's see what the ftp masters thinka bout the license.

i'm also not sure if the binary should have the setuid bit set, as to setup the tunnel it requires root permissions

[Centuriondan]

@sandrotosi
can we make that install time configurable using debconf? It may not be necessary if using the (yet to be packaged) plugin for network-manager

[sandrotosi]

oh look at that - i just started looking at nm-fortisslvpn! ;) yeah that's why i'm unsure, we'll see when i played a bit with the NM plugin

[Centuriondan]

@sandrotosi - well if your working on it the nm-fortisslvpn plugin I will follow your work for Devuan ;-)

[DimitriPapadopoulos]

@sandrotosi I also had a working package (perhaps with changes to apply to copyright) and I was supposed to package the software. Now my work has been discarded, you have used the fact that you're a Debian developer to push your own work, and I have disappeared from the copyright.
Anyway... I'm interested too in getting this package out so let's proceed this way.

[DimitriPapadopoulos]

@sandrotosi I too believe it's better not to have the setuid bit set.
I've seen other software manage this through /etc/sudoers and an openfortivpn group, as suggested by the author. This might be preferred in some deployments to mitigate this risk: #54.
Besides I too would rather use an nm-fortisslvpn plugin instead!

[DimitriPapadopoulos]

On Ubuntu nm-fortisslvpn won't work of course, because of Unity... I would have loved to write a plugin for Ubuntu but that's too much work.

[adrienverge]

Thanks for your work on this guys.

About the setuid bit: I agree it is not needed and could be a security concern in case of a flaw in openfortivpn.

In case this can be of any help, here is the spec file to build the Fedora / CentOS packages: https://src.fedoraproject.org/cgit/rpms/openfortivpn.git/tree/openfortivpn.spec

[simonvcodes]

Is there to the current date any workaround for using openfortivpn with debian testing?
I am also getting the described warning/error:

CC src/openfortivpn-config.o
CC src/openfortivpn-hdlc.o
CC src/openfortivpn-http.o
CC src/openfortivpn-io.o
src/io.c:62:22: error: ‘thread_id’ defined but not used [-Werror=unused-function]
static unsigned long thread_id()
^~~~~~~~~
src/io.c:55:13: error: ‘lock_callback’ defined but not used [-Werror=unused-function]
static void lock_callback(int mode, int type, char *file, int line)
^~~~~~~~~~~~~
cc1: all warnings being treated as errors
Makefile:520: die Regel für Ziel „src/openfortivpn-io.o“ scheiterte
make: *** [src/openfortivpn-io.o] Fehler 1

[DimitriPapadopoulos]

@sandrotosi Can I help update the openfortivpn package in experimental to version 1.3.0 and perhaps apply change 9f8a6d65?
I've started reading Debian New Maintainers' Guide and I understand I should have the rights to push an updated package, using for example dupload.
But what about the sources of the updated package? Should I fork https://anonscm.debian.org/cgit/collab-maint/openfortivpn.git, update my fork, and request a pull? Any documentation on which order to update branches pristine-tar, upstream and master?

[ckujau]

@groovejunkk see the comment from @sandrotosi above: add -Wno-unused-function to the build flags to ignore that error:

aclocal && autoconf && automake --add-missing && CFLAGS="-Wno-unused-function" ./configure && make

For the sake of completeness: current Ubuntu/17.04 (gcc 6.3.0 20170406) or openSUSE/Tumbleweed (gcc 6.3.1 20170202) systems are not affected, but Fedora 26 (gcc 7.1.1 20170503), Arch Linux (gcc 6.3.1 20170306) and Debian/unstable (gcc 6.3.0 20170516) still need that workaround to build.

GCC #28901 discusses this warning too and someone suggested another workaround for that, although I can't tell if that's just papering over a real problem in the code:

diff --git a/src/io.c b/src/io.c
index a2b7710..72d1fae 100644
--- a/src/io.c
+++ b/src/io.c
@@ -52,14 +52,14 @@ typedef dispatch_semaphore_t        sem_t;
 #define PKT_BUF_SZ 0x1000
 
 static pthread_mutex_t *lockarray;
-static void lock_callback(int mode, int type, char *file, int line)
+static void __attribute__((unused)) lock_callback(int mode, int type, char *file, int line)
 {
        if (mode & CRYPTO_LOCK)
                pthread_mutex_lock(&(lockarray[type]));
        else
                pthread_mutex_unlock(&(lockarray[type]));
 }
-static unsigned long thread_id()
+static unsigned long __attribute__((unused)) thread_id()
 {
        return (unsigned long) pthread_self();
 }

With that applied, openfortivpn compiles without using -Wno-unused-function.

[mrbaseman]

this would be an alternative to the solution with #pragma that I mentioned above (#81 (comment)). I think the same arguments still hold: It is a decision to treat all warnings as errors, but it's a compiler bug in clang that it complains about unused functions which actually are registered to openssl. The approach with __attribute__((unused)) however allows to specifically mark these two functions whereas the #pragma would generally disable this class of warnings for the current source file.

[DimitriPapadopoulos]

It appears this will be best fixed by removing -Werror from the default build instructions.

See Pull Request #151 and fc91349.

But then the Debian package building system might put back the -Werror. I really don't know...

[DimitriPapadopoulos]

At this point, this looks like a Debian packaging issue.

The -Werror option has been removed from the default build instructions (see acfca24). When packaging for systems on which the compiler emits bogus warnings and the -Werror option is added back by the packaging infrastructure, it's up to the packager to work around these issues.

[ckujau]

I still think that marking certain variables with __attribute__((unused)) is better than turning off -Werror globally, regardless of distribution specifics. That way the build will notice if this will happen again for a future variable, and then it can be decided again if this is a real problem or just a false positive. Without -Werror, code problems may creep in unnoticed. I think of it as the equivalent of use strict and use warnings in Perl :-)

[DimitriPapadopoulos]

@ckujau -Werror is not gone, it has been moved to the "make distcheck" target.

See the comment from a packager in fc91349:

It is not a nice things to do to packagers. Behavior of both are
strongly dependent on compiler version, are known to produce false
positives and will needlessly break build.

A maintainer can easily add -Werror too if needed. In fact, this patch adds
this to "make distcheck" target.