[libvirt] [PATCH] tests: do not ignore virInitialize failure
Simple...
From f5ee09ed08473478b3ea3135d51125fbf687e402 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 18 May 2010 12:32:39 +0200 Subject: [PATCH] tests: do not ignore virInitialize failure
* tests/nodeinfotest.c (mymain): Do not ignore virInitialize failure. Most other callers of virInitialize test for failure. --- tests/nodeinfotest.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index ff056b9..d3c500d 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -106,7 +106,8 @@ mymain(int argc, char **argv) return(EXIT_FAILURE); } - virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE; for (i = 0 ; i < ARRAY_CARDINALITY(nodeData); i++) if (virtTestRun(nodeData[i], 1, linuxTestNodeInfo, nodeData[i]) != 0) -- 1.7.1.250.g7d1e8
On 05/18/2010 04:33 AM, Jim Meyering wrote:
Simple...
From f5ee09ed08473478b3ea3135d51125fbf687e402 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 18 May 2010 12:32:39 +0200 Subject: [PATCH] tests: do not ignore virInitialize failure
* tests/nodeinfotest.c (mymain): Do not ignore virInitialize failure. Most other callers of virInitialize test for failure. --- tests/nodeinfotest.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index ff056b9..d3c500d 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -106,7 +106,8 @@ mymain(int argc, char **argv) return(EXIT_FAILURE); }
- virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE;
Shouldn't we be adding ATTRIBUTE_RETURN_CHECK to virInitialize in the appropriate header, to let the compiler enforce us to do the checking? ACK with that addition. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/18/2010 04:33 AM, Jim Meyering wrote:
Simple...
From f5ee09ed08473478b3ea3135d51125fbf687e402 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 18 May 2010 12:32:39 +0200 Subject: [PATCH] tests: do not ignore virInitialize failure
* tests/nodeinfotest.c (mymain): Do not ignore virInitialize failure. Most other callers of virInitialize test for failure. --- tests/nodeinfotest.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index ff056b9..d3c500d 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -106,7 +106,8 @@ mymain(int argc, char **argv) return(EXIT_FAILURE); }
- virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE;
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., #ifndef VIR_DEPRECATED /* The feature is present in gcc-3.1 and newer. */ # if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 1) # define VIR_DEPRECATED __attribute__((__deprecated__)) # else # define VIR_DEPRECATED /* nothing */ # endif #endif /* VIR_DEPRECATED */ Since my change does address the coverity-spotted problem, I'd prefer to keep this small and defer the more proactive (but riskier) change to a separate change set.
On Tue, May 18, 2010 at 05:25:20PM +0200, Jim Meyering wrote:
Eric Blake wrote:
On 05/18/2010 04:33 AM, Jim Meyering wrote:
Simple...
From f5ee09ed08473478b3ea3135d51125fbf687e402 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 18 May 2010 12:32:39 +0200 Subject: [PATCH] tests: do not ignore virInitialize failure
* tests/nodeinfotest.c (mymain): Do not ignore virInitialize failure. Most other callers of virInitialize test for failure. --- tests/nodeinfotest.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index ff056b9..d3c500d 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -106,7 +106,8 @@ mymain(int argc, char **argv) return(EXIT_FAILURE); }
- virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE;
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 ? Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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
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.
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jim Meyering