Eric Blake wrote:
On 05/18/2010 09:35 AM, Daniel P. Berrange wrote:
Shouldn't we be adding ATTRIBUTE_RETURN_CHECK to virInitialize in the appropriate header, to let the compiler enforce us to do the checking?
That would be nice, but the declaration of virInitialize is in libvirt.h.in, and I am leery of adding new symbols in such an exposed header, in spite of the few that are already there, e.g.,
Arguably every single function in the public libvirt.h should have an ATTRIBUTE_RETURN_CHECK annotation. The downside is that we could cause compile errors for existing apps using libvirt if they have been sloppy. I'm in two minds as to whether that's acceptable or not. Perhaps we could turn it on only if the symbol FORTIFY_SOURCE was defined, or something similar ?
ATTRIBUTE_RETURN_CHECK only causes a warning, unless you are compiling with -Werror. If users were sloppy, either they fix their coding, or they disable -Werror.
If we go forward with this, let's avoid using that particular name and instead use something more namespace-friendly like glibc's __wur or __attribute_warn_unused_result__: /usr/include/sys/cdefs.h:# define __wur __attribute_warn_unused_result__ /usr/include/sys/cdefs.h:# define __wur /* Ignore */
I see no problem with adding ATTRIBUTE_RETURN_CHECK globally (witness how recent glibc has been adding it to a lot of standard interfaces, without regards to FORTIFY_SOURCE). I see it as a service to our users.
I agree.