4:26 p.m.
This patch has the includes need to build on Solaris.
I've been using ifdef linux & ifndef linux to distinguish
between solaris and linux at this point.
I'm not sure what the preference is...
MRJ
5:33 p.m.
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
This patch has the includes need to build on Solaris.
I've been using ifdef linux & ifndef linux to distinguish
between solaris and linux at this point.
Looks ok aside from
+#ifndef __linux__
+#define NAME_MAX 14
+#endif
The #ifndef should test against NAME_MAX itself, rather than __linux__
Also, NAME_MAX is intended to be the maximum length of an unqualified
filename, so 14 characters is rather too small. 255 is what Linux has
it defined as, so I'd go for that unless Solaris has a different named
constant for maximum filename length ? I know BSD uses MAXNAMLEN,
but NAME_MAX is POSIX
I'm not sure what the preference is...
I don't think it really matters one way or the other when we're only
having to worry about a #ifdef choice between 2 platforms. If someone
ports libvirt to BSD then we can adapt to whichever results in smaller
code as needed.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6:05 p.m.
On 6/14/07, Daniel P. Berrange <berrange(a)redhat.com> wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
> This patch has the includes need to build on Solaris.
> I've been using ifdef linux & ifndef linux to distinguish
> between solaris and linux at this point.
Looks ok aside from
> +#ifndef __linux__
> +#define NAME_MAX 14
> +#endif
The #ifndef should test against NAME_MAX itself, rather than __linux__
Also, NAME_MAX is intended to be the maximum length of an unqualified
filename, so 14 characters is rather too small. 255 is what Linux has
it defined as, so I'd go for that unless Solaris has a different named
constant for maximum filename length ? I know BSD uses MAXNAMLEN,
but NAME_MAX is POSIX
Yep, I agree with both... Do yo want me to re-submit the patch
or do you want to make those changes?
> I'm not sure what the preference is...
I don't think it really matters one way or the other when we're only
having to worry about a #ifdef choice between 2 platforms. If someone
ports libvirt to BSD then we can adapt to whichever results in smaller
code as needed.
great. Thanks!
Marj
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
6:07 p.m.
On Thu, Jun 14, 2007 at 07:05:49PM -0400, Mark Johnson wrote:
On 6/14/07, Daniel P. Berrange <berrange(a)redhat.com> wrote:
>On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
>> This patch has the includes need to build on Solaris.
>> I've been using ifdef linux & ifndef linux to distinguish
>> between solaris and linux at this point.
>
>Looks ok aside from
>
>> +#ifndef __linux__
>> +#define NAME_MAX 14
>> +#endif
>
>The #ifndef should test against NAME_MAX itself, rather than __linux__
>
>Also, NAME_MAX is intended to be the maximum length of an unqualified
>filename, so 14 characters is rather too small. 255 is what Linux has
>it defined as, so I'd go for that unless Solaris has a different named
>constant for maximum filename length ? I know BSD uses MAXNAMLEN,
>but NAME_MAX is POSIX
Yep, I agree with both... Do yo want me to re-submit the patch
or do you want to make those changes?
No need - its quick enough to change this when its applied to CVS.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
3:06 a.m.
Daniel P. Berrange wrote:
On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
> This patch has the includes need to build on Solaris.
> I've been using ifdef linux & ifndef linux to distinguish
> between solaris and linux at this point.
Looks ok aside from
[..]
No, I don't agree. We should use configure.in to test for the presence
of header files and then do things like:
#ifdef HAVE_STRINGS_H
#include <strings.h>
#endif
See the current configure.in, AC_CHECK_HEADERS.
In fact we already have the HAVE_STRINGS_H symbol defined.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
4:21 a.m.
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
>On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
>>This patch has the includes need to build on Solaris.
>>I've been using ifdef linux & ifndef linux to distinguish
>>between solaris and linux at this point.
>
>Looks ok aside from
[..]
No, I don't agree. We should use configure.in to test for the presence
of header files and then do things like:
#ifdef HAVE_STRINGS_H
#include <strings.h>
#endif
See the current configure.in, AC_CHECK_HEADERS.
In fact we already have the HAVE_STRINGS_H symbol defined.
Agreed. And in the case of system specific header use the system specific
macro to guard e.g.:
+
+#ifndef __linux__
+#include <iso/limits_iso.h>
+#endif
should probably be rewritten as
#ifdef __sun__
#include <iso/limits_iso.h>
#endif
or with whatever the macro should be on your platform, so that if someone
tries to compile on say AIX it doesn't break on an inexistant header but
rather if there is a missing reference in the code itself.
thanks,
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 a.m.
On 6/15/07, John Levon <levon(a)movementarian.org> wrote:
On Fri, Jun 15, 2007 at 05:21:30AM -0400, Daniel Veillard wrote:
> +#ifndef __linux__
> +#include <iso/limits_iso.h>
> +#endif
This looks like an odd header to include anyway. Mark why isn't limits.h
enough?
I'll check in a little bit.. I'll change this to sun (assuming there's a
good
reason for the include).
As far a strings.h goes, I'm not sure how HAVE_STRINGS_H would
help us. Not that I understand configure* very well...
In this case, both linux and solaris have string.h and strings.h. There
are some things defined in string.h in linux which are in strings.h
in solaris.
Mark.
7:59 a.m.
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
Daniel P. Berrange wrote:
>On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
>>This patch has the includes need to build on Solaris.
>>I've been using ifdef linux & ifndef linux to distinguish
>>between solaris and linux at this point.
>
>Looks ok aside from
[..]
No, I don't agree. We should use configure.in to test for the presence
of header files and then do things like:
#ifdef HAVE_STRINGS_H
#include <strings.h>
#endif
For strings.h I don't see the point in making it conditional really, unless
we're going to do the same for every single other header we include. The
strings.h header is always present on Linux. In recent times stuff that was
previously in strings.h has moved to string.h, but they're still in the
original header too. So we should always include both string.h & strings.h
for maximum portability.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
8:03 a.m.
On Fri, Jun 15, 2007 at 01:59:41PM +0100, Daniel P. Berrange wrote:
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
> Daniel P. Berrange wrote:
> >On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
> >>This patch has the includes need to build on Solaris.
> >>I've been using ifdef linux & ifndef linux to distinguish
> >>between solaris and linux at this point.
> >
> >Looks ok aside from
> [..]
>
> No, I don't agree. We should use configure.in to test for the presence
> of header files and then do things like:
>
> #ifdef HAVE_STRINGS_H
> #include <strings.h>
> #endif
For strings.h I don't see the point in making it conditional really, unless
we're going to do the same for every single other header we include. The
strings.h header is always present on Linux. In recent times stuff that was
previously in strings.h has moved to string.h, but they're still in the
original header too. So we should always include both string.h & strings.h
for maximum portability.
Hum, I don't think they are really the same. In libxml2 I do a configure
test for HAVE_STRINGS_H but string.h is included without checks in a lot of
places.
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/
8:08 a.m.
On Fri, Jun 15, 2007 at 09:03:42AM -0400, Daniel Veillard wrote:
On Fri, Jun 15, 2007 at 01:59:41PM +0100, Daniel P. Berrange wrote:
> On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
> > Daniel P. Berrange wrote:
> > >On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
> > >>This patch has the includes need to build on Solaris.
> > >>I've been using ifdef linux & ifndef linux to distinguish
> > >>between solaris and linux at this point.
> > >
> > >Looks ok aside from
> > [..]
> >
> > No, I don't agree. We should use configure.in to test for the presence
> > of header files and then do things like:
> >
> > #ifdef HAVE_STRINGS_H
> > #include <strings.h>
> > #endif
>
> For strings.h I don't see the point in making it conditional really, unless
> we're going to do the same for every single other header we include. The
> strings.h header is always present on Linux. In recent times stuff that was
> previously in strings.h has moved to string.h, but they're still in the
> original header too. So we should always include both string.h & strings.h
> for maximum portability.
Hum, I don't think they are really the same. In libxml2 I do a configure
test for HAVE_STRINGS_H but string.h is included without checks in a lot of
places.
They're not the same - I string.h is a superset of strings.h on Linux, but
on other places they're disjoint sets.
Comment in strings.h...
/* We don't need and should not read this file if <string.h> was already
read. The one exception being that if __USE_BSD isn't defined, then
these aren't defined in string.h, so we need to define them here. */
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
8:16 a.m.
Daniel P. Berrange wrote:
On Fri, Jun 15, 2007 at 09:06:55AM +0100, Richard W.M. Jones wrote:
> Daniel P. Berrange wrote:
>> On Thu, Jun 14, 2007 at 05:26:39PM -0400, Mark Johnson wrote:
>>> This patch has the includes need to build on Solaris.
>>> I've been using ifdef linux & ifndef linux to distinguish
>>> between solaris and linux at this point.
>> Looks ok aside from
> [..]
>
> No, I don't agree. We should use configure.in to test for the presence
> of header files and then do things like:
>
> #ifdef HAVE_STRINGS_H
> #include <strings.h>
> #endif
For strings.h I don't see the point in making it conditional really, unless
we're going to do the same for every single other header we include. The
strings.h header is always present on Linux. In recent times stuff that was
previously in strings.h has moved to string.h, but they're still in the
original header too. So we should always include both string.h & strings.h
for maximum portability.
<string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard
said you can include the former without checks, but the latter should be
defended with an #ifdef HAVE_STRINGS_H.
On BSD, <strings.h> has the legacy functions like bcopy and index. Are
we using those? Shouldn't we instead replace any instances with memcpy
/ memmove / strchr / strrchr?
There are apparently some platforms where you can't include both ...
urrgh: http://gcc.gnu.org/ml/gcc-patches/1998-08/msg00317.html
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
8:32 a.m.
As johnlev said, I can fix the limits_iso.h with limits.h
<string.h> is in ISO C, but <strings.h> is not. So as
Daniel Veillard
said you can include the former without checks, but the latter should be
defended with an #ifdef HAVE_STRINGS_H.
On BSD, <strings.h> has the legacy functions like bcopy and index. Are
we using those? Shouldn't we instead replace any instances with memcpy
/ memmove / strchr / strrchr?
It's needed because of index().
There are apparently some platforms where you can't include both
...
urrgh: http://gcc.gnu.org/ml/gcc-patches/1998-08/msg00317.html
That can break on BSD and solaris since they have both string.h and
strings.h which don't overlap.
8:34 a.m.
On 6/15/07, Mark Johnson <johnson.nh(a)gmail.com> wrote:
As johnlev said, I can fix the limits_iso.h with limits.h
> <string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard
> said you can include the former without checks, but the latter should be
> defended with an #ifdef HAVE_STRINGS_H.
>
> On BSD, <strings.h> has the legacy functions like bcopy and index. Are
> we using those? Shouldn't we instead replace any instances with memcpy
> / memmove / strchr / strrchr?
It's needed because of index().
I guess not anymore :-)
MRJ
> There are apparently some platforms where you can't include
both ...
> urrgh: http://gcc.gnu.org/ml/gcc-patches/1998-08/msg00317.html
That can break on BSD and solaris since they have both string.h and
strings.h which don't overlap.
8:35 a.m.
On Fri, Jun 15, 2007 at 09:32:13AM -0400, Mark Johnson wrote:
As johnlev said, I can fix the limits_iso.h with limits.h
><string.h> is in ISO C, but <strings.h> is not. So as Daniel Veillard
>said you can include the former without checks, but the latter should be
>defended with an #ifdef HAVE_STRINGS_H.
>
>On BSD, <strings.h> has the legacy functions like bcopy and index. Are
>we using those? Shouldn't we instead replace any instances with memcpy
>/ memmove / strchr / strrchr?
It's needed because of index().
please use strchr() let's get rid of strings.h , that will be way simpler.
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/
8:39 a.m.
OK, I'll make another stab at this patch with all the comments and
send it out shortly.
Thanks,
Mark
10 a.m.
Mark Johnson wrote:
updated header file patch attached. This one is much nicer. :-)
I think that's fine, except that I will change:
+#ifdef __linux__
#include <paths.h>
+#endif
to:
+#ifdef HAVE_PATHS_H
#include <paths.h>
+#endif
and add the appropriate detection to configure.in. If everyone else is
happy with that then I can commit it.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
10:09 a.m.
On Fri, Jun 15, 2007 at 04:00:01PM +0100, Richard W.M. Jones wrote:
Mark Johnson wrote:
>updated header file patch attached. This one is much nicer. :-)
I think that's fine, except that I will change:
+#ifdef __linux__
#include <paths.h>
+#endif
to:
+#ifdef HAVE_PATHS_H
#include <paths.h>
+#endif
and add the appropriate detection to configure.in. If everyone else is
happy with that then I can commit it.
+1
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/
10:26 a.m.
Mark:
Committed. Can you just check that it still fixes Solaris compiles over
there?
Also, thank you for your contribution to libvirt.
Rich.
--
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
England and Wales under Company Registration No. 03798903
6404
days inactive
6405
days old
20 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
John Levon -
Mark Johnson -
Richard W.M. Jones