4:02 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:10 a.m.
New subject: [Libvir] [patch 3/3] Do not inline xstrtol functions
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.
7:39 a.m.
New subject: [Libvir] [patch 3/3] Do not inline xstrtol functions
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:55 a.m.
New subject: [Libvir] [patch 3/3] Do not inline xstrtol functions
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.
8:33 a.m.
New subject: [Libvir] [patch 3/3] Do not inline xstrtol functions
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
4:11 a.m.
New subject: [Libvir] [patch 3/3] Do not inline xstrtol functions
Daniel Veillard <veillard(a)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.
6132
days inactive
6132
days old
5 comments
3 participants
participants (3)
-
Daniel Veillard -
Jim Meyering -
Mark McLoughlin