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. 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. But I also agree with Jim's sentiment that adding it is a separate patch; therefore, ACK to the tests/nodeinfotest.c change regardless of whether we modify libvirt.h.in in a separate patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org