Re: [Libvir] [patch 3/3] Do not inline xstrtol functions
On Wed, Feb 06, 2008 at 11:07:55PM +0000, Mark McLoughlin wrote:
Our strtol() variants are all marked "static inline" and with gcc 4.3 we get:
internal.h:272: error: inlining failed in call to 'xstrtol_i': call is unlikely and code size would grow
This patch renames them to virStrToLong() and exports them from the library as private symbols.
Hum, I don't like too much adding more exported symbols
Alternative is to not build with -Winline.
That sounds a weak way to try to avoid a problem, we should not rely on just compiler options to get the code to compile and link. My preference would be to use the patch to make them real internal APIs without exporting all the functions, I think only xstrtol_i is used by external programs (virsh and qemud), and maybe we can add only that one to the list of exported symbols. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, 2008-02-07 at 05:02 -0500, Daniel Veillard wrote:
On Wed, Feb 06, 2008 at 11:07:55PM +0000, Mark McLoughlin wrote:
Alternative is to not build with -Winline.
That sounds a weak way to try to avoid a problem, we should not rely on just compiler options to get the code to compile and link.
My preference would be to use the patch to make them real internal APIs without exporting all the functions, I think only xstrtol_i is used by external programs (virsh and qemud), and maybe we can add only that one to the list of exported symbols.
Yeah, good point. Like this? Cheers, Mark.
On Thu, Feb 07, 2008 at 10:10:34AM +0000, Mark McLoughlin wrote:
On Thu, 2008-02-07 at 05:02 -0500, Daniel Veillard wrote:
On Wed, Feb 06, 2008 at 11:07:55PM +0000, Mark McLoughlin wrote:
Alternative is to not build with -Winline.
That sounds a weak way to try to avoid a problem, we should not rely on just compiler options to get the code to compile and link.
My preference would be to use the patch to make them real internal APIs without exporting all the functions, I think only xstrtol_i is used by external programs (virsh and qemud), and maybe we can add only that one to the list of exported symbols.
Yeah, good point. Like this?
Yup fine by me, maybe add a #include to allow using the function name without the __ within the library code, allowing for more uniform code. But we don't have a libvrty code internal header, both internal.h and util.h are for example used by virsh.c so I don't know where to put this.
Index: libvirt/src/nodeinfo.h =================================================================== --- libvirt.orig/src/nodeinfo.h 2008-02-07 10:05:32.000000000 +0000 +++ libvirt/src/nodeinfo.h 2008-02-07 10:05:46.000000000 +0000 @@ -24,7 +24,7 @@ #ifndef __VIR_NODEINFO_H__ #define __VIR_NODEINFO_H__
-#include "internal.h" +#include "libvirt/libvirt.h"
#ifdef __cplusplus extern "C" {
Seems a bit unrelated to the initial issue, but that's fine, I just didn't spot it the first time. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, 2008-02-07 at 08:39 -0500, Daniel Veillard wrote:
On Thu, Feb 07, 2008 at 10:10:34AM +0000, Mark McLoughlin wrote:
On Thu, 2008-02-07 at 05:02 -0500, Daniel Veillard wrote:
On Wed, Feb 06, 2008 at 11:07:55PM +0000, Mark McLoughlin wrote:
Alternative is to not build with -Winline.
That sounds a weak way to try to avoid a problem, we should not rely on just compiler options to get the code to compile and link.
My preference would be to use the patch to make them real internal APIs without exporting all the functions, I think only xstrtol_i is used by external programs (virsh and qemud), and maybe we can add only that one to the list of exported symbols.
Yeah, good point. Like this?
Yup fine by me, maybe add a #include to allow using the function name without the __ within the library code, allowing for more uniform code.
Did you miss this: #define virStrToLong_i(s,e,b,r) __virStrToLong_i((s),(e),(b),(r)) Or ...? Sorry, I'm not following.
Index: libvirt/src/nodeinfo.h =================================================================== --- libvirt.orig/src/nodeinfo.h 2008-02-07 10:05:32.000000000 +0000 +++ libvirt/src/nodeinfo.h 2008-02-07 10:05:46.000000000 +0000 @@ -24,7 +24,7 @@ #ifndef __VIR_NODEINFO_H__ #define __VIR_NODEINFO_H__
-#include "internal.h" +#include "libvirt/libvirt.h"
#ifdef __cplusplus extern "C" {
Seems a bit unrelated to the initial issue, but that's fine, I just didn't spot it the first time.
Yeah, it looks a bit bogus ... but it is related. internal.h was only needed for xstrtol() use in nodeinfo.c, so I would have replace it with util.h, except util.h is not needed by the header itself so I included the appropriate header in nodeinfo.h and util.h in nodeinfo.c. Cheers, Mark.
On Thu, Feb 07, 2008 at 01:55:58PM +0000, Mark McLoughlin wrote:
On Thu, 2008-02-07 at 08:39 -0500, Daniel Veillard wrote:
On Thu, Feb 07, 2008 at 10:10:34AM +0000, Mark McLoughlin wrote:
On Thu, 2008-02-07 at 05:02 -0500, Daniel Veillard wrote:
On Wed, Feb 06, 2008 at 11:07:55PM +0000, Mark McLoughlin wrote:
Alternative is to not build with -Winline.
That sounds a weak way to try to avoid a problem, we should not rely on just compiler options to get the code to compile and link.
My preference would be to use the patch to make them real internal APIs without exporting all the functions, I think only xstrtol_i is used by external programs (virsh and qemud), and maybe we can add only that one to the list of exported symbols.
Yeah, good point. Like this?
Yup fine by me, maybe add a #include to allow using the function name without the __ within the library code, allowing for more uniform code.
Did you miss this:
#define virStrToLong_i(s,e,b,r) __virStrToLong_i((s),(e),(b),(r))
Or ...? Sorry, I'm not following.
Yes I missed it :-) , but I looked really !
Index: libvirt/src/nodeinfo.h =================================================================== --- libvirt.orig/src/nodeinfo.h 2008-02-07 10:05:32.000000000 +0000 +++ libvirt/src/nodeinfo.h 2008-02-07 10:05:46.000000000 +0000 @@ -24,7 +24,7 @@ #ifndef __VIR_NODEINFO_H__ #define __VIR_NODEINFO_H__
-#include "internal.h" +#include "libvirt/libvirt.h"
#ifdef __cplusplus extern "C" {
Seems a bit unrelated to the initial issue, but that's fine, I just didn't spot it the first time.
Yeah, it looks a bit bogus ... but it is related. internal.h was only needed for xstrtol() use in nodeinfo.c, so I would have replace it with util.h, except util.h is not needed by the header itself so I included the appropriate header in nodeinfo.h and util.h in nodeinfo.c.
okay, okay, +1 -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> wrote:
On Wed, Feb 06, 2008 at 11:07:55PM +0000, Mark McLoughlin wrote:
Our strtol() variants are all marked "static inline" and with gcc 4.3 we get:
internal.h:272: error: inlining failed in call to 'xstrtol_i': call is unlikely and code size would grow
This patch renames them to virStrToLong() and exports them from the library as private symbols.
Hum, I don't like too much adding more exported symbols
Nor I, hence my preference to not display the warning by omitting -Winline.
Alternative is to not build with -Winline.
That sounds a weak way to try to avoid a problem, we should not rely on just compiler options to get the code to compile and link.
Um... omitting -Winline would just suppress the warning. There's no compile or link failure here.
My preference would be to use the patch to make them real internal APIs without exporting all the functions, I think only xstrtol_i is used by external programs (virsh and qemud), and maybe we can add only that one to the list of exported symbols.
participants (3)
-
Daniel Veillard -
Jim Meyering -
Mark McLoughlin